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.

Signed-off-by: Liu Qing <liuqing@huayun.com>
---
 src/conf/domain_conf.c    | 30 ++++++++++++++++++++++++++++++
 src/qemu/qemu_command.c   |  6 ++++++
 src/util/virstoragefile.c |  3 +++
 src/util/virstoragefile.h |  4 ++++
 4 files changed, 43 insertions(+)

diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index d97aab4..06ca1de 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -8590,6 +8590,30 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
         VIR_FREE(tmp);
     }
 
+    if ((tmp = virXMLPropString(cur, "l2-cache-size")) &&
+        (virStrToLong_ui(tmp, NULL, 10, &def->src->l2_cache_size) < 0)) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid l2-cache-size attribute in disk driver element: %s"),
+                       tmp);
+        goto cleanup;
+    }
+
+    if ((tmp = virXMLPropString(cur, "refcount-cache-size")) &&
+        (virStrToLong_ui(tmp, NULL, 10, &def->src->refcount_cache_size) < 0)) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid refcount-cache-size attribute in disk driver element: %s"),
+                       tmp);
+        goto cleanup;
+    }
+
+    if ((tmp = virXMLPropString(cur, "cache-clean-interval")) &&
+        (virStrToLong_ui(tmp, NULL, 10, &def->src->cache_clean_interval) < 0)) {
+        virReportError(VIR_ERR_XML_ERROR,
+                       _("Invalid cache-clean-interval attribute in disk driver element: %s"),
+                       tmp);
+        goto cleanup;
+    }
+
     if ((tmp = virXMLPropString(cur, "detect_zeroes")) &&
         (def->detect_zeroes = virDomainDiskDetectZeroesTypeFromString(tmp)) <= 0) {
         virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
@@ -21887,6 +21911,12 @@ virDomainDiskDefFormat(virBufferPtr buf,
         virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread);
     if (def->detect_zeroes)
         virBufferAsprintf(&driverBuf, " detect_zeroes='%s'", detect_zeroes);
+    if (def->src->l2_cache_size > 0)
+        virBufferAsprintf(&driverBuf, " l2-cache-size='%u'", def->src->l2_cache_size);
+    if (def->src->refcount_cache_size > 0)
+        virBufferAsprintf(&driverBuf, " refcount-cache-size='%u'", def->src->refcount_cache_size);
+    if (def->src->cache_clean_interval > 0)
+        virBufferAsprintf(&driverBuf, " cache-clean-interval='%u'", def->src->cache_clean_interval);
 
     virDomainVirtioOptionsFormat(&driverBuf, def->virtio);
 
diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
index 9a27987..7996eed 100644
--- a/src/qemu/qemu_command.c
+++ b/src/qemu/qemu_command.c
@@ -1430,6 +1430,12 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
             qemuformat = "luks";
         virBufferAsprintf(buf, "format=%s,", qemuformat);
     }
+    if (disk->src->format == VIR_STORAGE_FILE_QCOW2 && disk->src->l2_cache_size > 0)
+        virBufferAsprintf(buf, "l2-cache-size=%u,", disk->src->l2_cache_size);
+    if (disk->src->format == VIR_STORAGE_FILE_QCOW2 && disk->src->refcount_cache_size > 0)
+        virBufferAsprintf(buf, "refcount-cache-size=%u,", disk->src->refcount_cache_size);
+    if (disk->src->format == VIR_STORAGE_FILE_QCOW2 && disk->src->cache_clean_interval > 0)
+        virBufferAsprintf(buf, "cache-clean-interval=%u,", disk->src->cache_clean_interval);
 
     ret = 0;
 
diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
index fbc8245..c685331 100644
--- a/src/util/virstoragefile.c
+++ b/src/util/virstoragefile.c
@@ -2038,6 +2038,9 @@ virStorageSourceCopy(const virStorageSource *src,
     ret->physical = src->physical;
     ret->readonly = src->readonly;
     ret->shared = src->shared;
+    ret->l2_cache_size = src->l2_cache_size;
+    ret->refcount_cache_size = src->refcount_cache_size;
+    ret->cache_clean_interval = src->cache_clean_interval;
 
     /* storage driver metadata are not copied */
     ret->drv = NULL;
diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h
index 6c388b1..e7889d9 100644
--- a/src/util/virstoragefile.h
+++ b/src/util/virstoragefile.h
@@ -280,6 +280,10 @@ struct _virStorageSource {
     /* metadata that allows identifying given storage source */
     char *nodeformat;  /* name of the format handler object */
     char *nodestorage; /* name of the storage object */
+
+    unsigned l2_cache_size; /* qcow2 l2 cache size */
+    unsigned refcount_cache_size; /* qcow2 reference count table cache size */
+    unsigned cache_clean_interval; /* clean unused cache entries interval */
 };
 
 
-- 
1.8.3.1

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On Wed, Aug 30, 2017 at 11:12:02AM +0200, Martin Kletzander wrote:
> 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).
Thanks for the guide, I will take time to complete these.
> 
> Have a nice day,
> Martin


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list