[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Add qcow2 cache configuration



On Wed, Aug 30, 2017 at 04:07:16PM +0800, Liu Qing wrote:
Random write IOPS will drop dramatically if qcow2 l2 cache could not
cover the whole disk. This patch give libvirt user a chance to adjust
the qcow2 cache configuration.


Thanks for the patch, but it has no documentation, no capability
checking, no RNG schema adjustments and no tests.  Ideally the patch
should be a series of patches, each introducing part of the
functionality.  For example:

 [PATCH 1/3] conf, docs: Add support for bla bla bla

   Add stuff to docs/schemas/*.rng, docs/*.html.in, src/conf/*.c, add
   parsing tests to tests/qemuxml2xmltest.c (and possibly
   qemuxml2argvtest.c if there is some negative test that should fail
   parsing).  Code needs to compile cleanly and tests need to pass after
   this patch.  Documentation should cleanly state the reasoning and
   rules for the possible values so that users know *if* and *why* they
   need to set this up and to *what* values.  It is also good to think
   about why QEMU doesn't use such values as default and whether or not
   (or why/not) libvirt should default to such values without making
   the user do so.

 [PATCH 2/3] qemu: Add capability checking for bla bla bla

   Here you would check that we properly probe qemu for the possibility
   of setting such tunables in src/qemu/qemu_capabilities.[hc].  Code
   needs to compile cleanly and tests need to pass after this patch.

 [PATCH 3/3] qemu: Add support for bla bla bla

   Here you would check if the emulator has the required capabilities,
   format them on the command line and add positive tests to
   tests/qemuxml2argvtest.c.  Code needs to compile cleanly and tests
   need to pass after this patch.

In rare cases where the functionality and required tests are minimal,
patches [2/3] and [3/3] could be merged together, but they can always be
separate, IMHO.

All of this ^^ is only about the way the patch is supposed to be sent.
Whether or not it makes sense to expose such tunables is left as an
exercise to all readers (and possibly a discussion on v2 of this patch).

Have a nice day,
Martin

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]