On 09/14/2017 01:08 AM, Liu Qing wrote:
> Random write IOPS will drop dramatically if qcow2 l2 cache could not
> cover the whole disk. This patch gives libvirt user a chance to adjust
> the qcow2 cache configuration.
>
> Three new qcow2 driver parameters (l2-cache-size, refcount-cache-size
> and cache-clean-interval) are added as attributes to a new <qcow2>
> subelement for a <driver name='qemu' type='qcow2'...> of a <disk>
> element. The QEMU source docs/qcow2-cache.txt provides the guidelines
> for defining/configuring values for each.
>
> Signed-off-by: Liu Qing <liuqing@xxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 41 +++++++++
> docs/schemas/domaincommon.rng | 35 ++++++++
> src/conf/domain_conf.c | 96 ++++++++++++++++++++--
> src/qemu/qemu_driver.c | 5 ++
> src/util/virstoragefile.c | 3 +
> src/util/virstoragefile.h | 6 ++
> .../qemuxml2argv-disk-drive-qcow2-cache.xml | 43 ++++++++++
> .../qemuxml2xmlout-disk-drive-qcow2-cache.xml | 43 ++++++++++
> tests/qemuxml2xmltest.c | 1 +
> 9 files changed, 268 insertions(+), 5 deletions(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml
>
OK - so I had my head in other code during the v4 review comments from
Peter. So I'll just "restart" that a bit here. If I can summarize the
comments... There's a concern that we're creating a very specific set of
parameters for which there isn't great guidance available other than
going to a source file in the qemu tree and attempting to decipher it.
Something I did note at one time as a possible issue and why I also
asked for the "qcow2" subelement. It makes it painfully obvious that
there was something very specific. I figure seeing a "qcow2" element
name would fairly easily point out to anyone that specificity.
So, based on what I read in the responses it seems that the purpose of
the values is to provide a mechanism to "allow" qemu to use a couple of
different algorithms to perform operations, but qemu wants the user to
decide whether they are willing to sacrifice one thing over another. If
you want high IOPS, then you have to sacrifice something else; whereas,
if you don't care about performance, then you sacrifice something else.
In any case, I think what could be useful is to determine if there is a
way to "automate" something such that the "dials" would be automatically
set based on the algorithms. That is - rather than supplying the
specific values, supply a named attribute that would then perform the
calculations and set the values, such as "high_performance="yes" or
"large_capacity=yes". Where you cannot set both, but can set one or the
other. Or can you set both? Is there value in partially turning a knob?
If so maybe the attribute is "performance={default, ..., best}" and
"capacity={default, ..., large}". Then based on the value of the
attribute, code you add to libvirt would perform the calculations.
Or do you really think that's something some supplier (such as what
you're trying to do) would want to have - the ability to control the
algorithm for the various knobs.
Hopefully this makes sense...
One minor nit below which is easily fixable...
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8ca7637..4574e3a 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3036,6 +3036,47 @@
> set. (<span class="since">Since 3.5.0</span>)
> </li>
> </ul>
> + The <code>driver</code> element may contain a <code>qcow2</code> sub
> + element, which to specifying further details related to a qcow2 disk.
> + For recommended setting guidelines refer to the QEMU source file
> + <code>docs/qcow2-cache.txt</code>.
> + <span class="since">Since 3.8.0</span>
> + <ul>
> + <li>
> + The optional <code>l2_cache_size</code> attribute controls how much
> + memory will be consumed by qcow2 l2 table cache in bytes. This
> + option is only valid when the driver type is qcow2. The default
> + size is 2097152.
> + <span class='since'>Since 3.8.0</span>
> +
> + <b>In general you should leave this option alone, unless you
> + are very certain you know what you are doing.</b>
> + </li>
> + <li>
> + The optional <code>refcount_cache_size</code> attribute controls
> + how much memory will be consumed by qcow2 reference count table
> + cache in bytes. This option is only valid when the driver type is
> + qcow2. The default size is 262144.
> + <span class='since'>Since 3.8.0</span>
> +
> + <b>In general you should leave this option alone, unless you
> + are very certain you know what you are doing.</b>
> + </li>
> + <li>
> + The optional <code>cache_clean_interval</code> attribute defines
> + an interval (in seconds). All cache entries that haven't been
> + accessed during that interval are removed from memory. This option
> + is only valid when the driver type is qcow2. The default
> + value is 0, which disables this feature. If the interval is too
> + short, it will cause frequent cache write back and load, which
> + impact performance. If the interval is too long, unused cache
> + will still consume memory until it's been written back to disk.
> + <span class='since'>Since 3.8.0</span>
> +
> + <b>In general you should leave this option alone, unless you
> + are very certain you know what you are doing.</b>
> + </li>
> + </ul>
> </dd>
> <dt><code>backenddomain</code></dt>
> <dd>The optional <code>backenddomain</code> element allows specifying a
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index c9a4f7a..0e25f44 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1756,6 +1756,23 @@
> </element>
> </define>
> <!--
> + Parameters for qcow2 driver
> + -->
> + <define name="qcow2Driver">
> + <element name="qcow2">
> + <optional>
> + <ref name="qcow2_l2_cache_size"/>
> + </optional>
> + <optional>
> + <ref name="qcow2_refcount_cache_size"/>
> + </optional>
> + <optional>
> + <ref name="qcow2_cache_clean_interval"/>
> + </optional>
> + </element>
> + </define>
> +
> + <!--
> Disk may use a special driver for access.
> -->
> <define name="diskDriver">
> @@ -1794,6 +1811,9 @@
> <ref name="detect_zeroes"/>
> </optional>
> <ref name="virtioOptions"/>
> + <optional>
> + <ref name="qcow2Driver"/>
> + </optional>
> <empty/>
> </element>
> </define>
> @@ -1889,6 +1909,21 @@
> </choice>
> </attribute>
> </define>
> + <define name="qcow2_l2_cache_size">
> + <attribute name='l2_cache_size'>
> + <ref name="unsignedLong"/>
> + </attribute>
> + </define>
> + <define name="qcow2_refcount_cache_size">
> + <attribute name='refcount_cache_size'>
> + <ref name="unsignedLong"/>
> + </attribute>
> + </define>
> + <define name="qcow2_cache_clean_interval">
> + <attribute name='cache_clean_interval'>
> + <ref name="unsignedLong"/>
> + </attribute>
> + </define>
> <define name="controller">
> <element name="controller">
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index a43b25c..e29ecb5 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5734,6 +5734,30 @@ virDomainDeviceLoadparmIsValid(const char *loadparm)
>
>
> static void
> +virDomainQcow2CacheOptionsFormat(virBufferPtr buf,
> + virDomainDiskDefPtr def)
> +{
> + if (def->src->l2_cache_size == 0 &&
> + def->src->refcount_cache_size == 0 &&
> + def->src->cache_clean_interval == 0)
> + return;
> +
> + virBufferAddLit(buf, "<qcow2");
> +
> + if (def->src->l2_cache_size > 0)
> + virBufferAsprintf(buf, " l2_cache_size='%llu'",
> + def->src->l2_cache_size);
> + if (def->src->refcount_cache_size > 0)
> + virBufferAsprintf(buf, " refcount_cache_size='%llu'",
> + def->src->refcount_cache_size);
> + if (def->src->cache_clean_interval > 0)
> + virBufferAsprintf(buf, " cache_clean_interval='%llu'",
> + def->src->cache_clean_interval);
> + virBufferAddLit(buf, "/>\n");
> +}
> +
> +
> +static void
> virDomainVirtioOptionsFormat(virBufferPtr buf,
> virDomainVirtioOptionsPtr virtio)
> {
> @@ -8572,15 +8596,69 @@ virDomainDiskDefParseValidate(const virDomainDiskDef *def)
> }
> }
>
> + if (def->src->format != VIR_STORAGE_FILE_QCOW2 &&
> + (def->src->l2_cache_size > 0 || def->src->refcount_cache_size > 0 ||
> + def->src->cache_clean_interval > 0)) {
> + virReportError(VIR_ERR_XML_ERROR,
> + _("Setting l2_cache_size, refcount_cache_size, "
> + "cache_clean_interval is not allowed for types "
> + "other than QCOW2"));
> + return -1;
> + }
> +
> return 0;
> }
>
>
> static int
> +virDomainDiskDefQcow2ParseXML(virDomainDiskDefPtr def,
> + xmlNodePtr cur)
> +{
> + char *tmp = NULL;
> + int ret = -1;
> +
> + if ((tmp = virXMLPropString(cur, "l2_cache_size")) &&
> + (virStrToLong_ullp(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;
> + }
> + VIR_FREE(tmp);
> +
> + if ((tmp = virXMLPropString(cur, "refcount_cache_size")) &&
> + (virStrToLong_ullp(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;
> + }
> + VIR_FREE(tmp);
> +
> + if ((tmp = virXMLPropString(cur, "cache_clean_interval")) &&
> + (virStrToLong_ullp(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;
> + }
> + VIR_FREE(tmp);
> +
> + ret = 0;
> +
> + cleanup:
> + VIR_FREE(tmp);
> +
> + return ret;
> +}
> +
> +
> +static int
> virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
> xmlNodePtr cur)
> {
> char *tmp = NULL;
> + xmlNodePtr child;
> int ret = -1;
>
> def->src->driverName = virXMLPropString(cur, "name");
> @@ -8683,6 +8761,13 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
> }
> VIR_FREE(tmp);
>
> + for (child = cur->children; child != NULL; child = child->next) {
> + if (virXMLNodeNameEqual(child, "qcow2") &&
> + virDomainDiskDefQcow2ParseXML(def, child) < 0) {
> + goto cleanup;
> + }
> + }
> +
> ret = 0;
>
> cleanup:
> @@ -21885,6 +21970,7 @@ virDomainDiskDefFormat(virBufferPtr buf,
> const char *discard = virDomainDiskDiscardTypeToString(def->discard);
> const char *detect_zeroes = virDomainDiskDetectZeroesTypeToString(def->detect_zeroes);
> virBuffer driverBuf = VIR_BUFFER_INITIALIZER;
> + virBuffer driverQcow2Buf = VIR_BUFFER_INITIALIZER;
>
> if (!type || !def->src->type) {
> virReportError(VIR_ERR_INTERNAL_ERROR,
> @@ -21966,11 +22052,11 @@ virDomainDiskDefFormat(virBufferPtr buf,
> if (virBufferCheckError(&driverBuf) < 0)
> return -1;
>
FWIW:
The above 3 lines wouldn't be necessary now since virXMLFormatElement
performs the virBufferCheckError.
> - if (virBufferUse(&driverBuf)) {
> - virBufferAddLit(buf, "<driver");
> - virBufferAddBuffer(buf, &driverBuf);
> - virBufferAddLit(buf, "/>\n");
> - }
> + virBufferSetChildIndent(&driverQcow2Buf, buf);
> + virDomainQcow2CacheOptionsFormat(&driverQcow2Buf, def);
> +
> + if (virXMLFormatElement(buf, "driver", &driverBuf, &driverQcow2Buf) < 0)
> + return -1;
>
> if (def->src->auth) {
> if (virStorageAuthDefFormat(buf, def->src->auth) < 0)
> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
> index e956839..c3b81e1 100644
> --- a/src/qemu/qemu_driver.c
> +++ b/src/qemu/qemu_driver.c
> @@ -14318,6 +14318,11 @@ qemuDomainSnapshotDiskDataCollect(virQEMUDriverPtr driver,
> if (!(dd->src = virStorageSourceCopy(snap->def->disks[i].src, false)))
> goto error;
>
> + /* keep the qcow2 cache configuration */
> + dd->src->l2_cache_size = vm->def->disks[i]->src->l2_cache_size;
> + dd->src->refcount_cache_size = vm->def->disks[i]->src->refcount_cache_size;
> + dd->src->cache_clean_interval = vm->def->disks[i]->src->cache_clean_interval;
> +
Oh - right - I seeing src/qemu/qemu_ and didn't even look to see if was
"command" ;-)... My bad on previous v3/v4 comments!
John
> if (virStorageSourceInitChainElement(dd->src, dd->disk->src, false) < 0)
> goto error;
>
> diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c
> index e94ad32..f23390f 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..9b5a5f3 100644
> --- a/src/util/virstoragefile.h
> +++ b/src/util/virstoragefile.h
> @@ -280,6 +280,12 @@ 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 long long l2_cache_size; /* qcow2 l2 cache size */
> + /* qcow2 reference count table cache size */
> + unsigned long long refcount_cache_size;
> + /* clean unused cache entries interval */
> + unsigned long long cache_clean_interval;
> };
>
>
> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml
> new file mode 100644
> index 0000000..3f464db
> --- /dev/null
> +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml
> @@ -0,0 +1,43 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <disk type='block' device='disk'>
> + <driver name='qemu' type='qcow2' cache='none'>
> + <qcow2 l2_cache_size='2097152' refcount_cache_size='524288' cache_clean_interval='900'/>
> + </driver>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
> + <disk type='block' device='cdrom'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/HostVG/QEMUGuest2'/>
> + <target dev='hdc' bus='ide'/>
> + <readonly/>
> + <address type='drive' controller='0' bus='1' target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='none'/>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml
> new file mode 100644
> index 0000000..3f464db
> --- /dev/null
> +++ b/tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml
> @@ -0,0 +1,43 @@
> +<domain type='qemu'>
> + <name>QEMUGuest1</name>
> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid>
> + <memory unit='KiB'>219136</memory>
> + <currentMemory unit='KiB'>219136</currentMemory>
> + <vcpu placement='static'>1</vcpu>
> + <os>
> + <type arch='i686' machine='pc'>hvm</type>
> + <boot dev='hd'/>
> + </os>
> + <clock offset='utc'/>
> + <on_poweroff>destroy</on_poweroff>
> + <on_reboot>restart</on_reboot>
> + <on_crash>destroy</on_crash>
> + <devices>
> + <emulator>/usr/bin/qemu-system-i686</emulator>
> + <disk type='block' device='disk'>
> + <driver name='qemu' type='qcow2' cache='none'>
> + <qcow2 l2_cache_size='2097152' refcount_cache_size='524288' cache_clean_interval='900'/>
> + </driver>
> + <source dev='/dev/HostVG/QEMUGuest1'/>
> + <target dev='hda' bus='ide'/>
> + <address type='drive' controller='0' bus='0' target='0' unit='0'/>
> + </disk>
> + <disk type='block' device='cdrom'>
> + <driver name='qemu' type='raw'/>
> + <source dev='/dev/HostVG/QEMUGuest2'/>
> + <target dev='hdc' bus='ide'/>
> + <readonly/>
> + <address type='drive' controller='0' bus='1' target='0' unit='0'/>
> + </disk>
> + <controller type='usb' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x2'/>
> + </controller>
> + <controller type='ide' index='0'>
> + <address type='pci' domain='0x0000' bus='0x00' slot='0x01' function='0x1'/>
> + </controller>
> + <controller type='pci' index='0' model='pci-root'/>
> + <input type='mouse' bus='ps2'/>
> + <input type='keyboard' bus='ps2'/>
> + <memballoon model='none'/>
> + </devices>
> +</domain>
> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c
> index 0a87ced..fab1e19 100644
> --- a/tests/qemuxml2xmltest.c
> +++ b/tests/qemuxml2xmltest.c
> @@ -461,6 +461,7 @@ mymain(void)
> DO_TEST("disk-drive-cache-v2-none", NONE);
> DO_TEST("disk-drive-cache-directsync", NONE);
> DO_TEST("disk-drive-cache-unsafe", NONE);
> + DO_TEST("disk-drive-qcow2-cache", NONE);
> DO_TEST("disk-drive-network-nbd", NONE);
> DO_TEST("disk-drive-network-nbd-export", NONE);
> DO_TEST("disk-drive-network-nbd-ipv6", NONE);
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
