[libvirt] [PATCH] Add qcow2 cache configuration

Martin Kletzander mkletzan at redhat.com
Wed Aug 30 09:12:02 UTC 2017


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170830/30585f2f/attachment-0001.sig>


More information about the libvir-list mailing list