On 09/07/2017 02:09 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 are added. They are l2-cache-size,
> refcount-cache-size and cache-clean-interval.
>
> The following are from qcow2-cache.txt.
> The amount of virtual disk that can be mapped by the L2 and refcount
> caches (in bytes) is:
> disk_size = l2_cache_size * cluster_size / 8
> disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits
>
> The parameter "cache-clean-interval" defines an interval (in seconds).
> All cache entries that haven't been accessed during that interval are
> removed from memory.
Suggestion, rewrite a bit:
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.
...
That is - let's not bother with repeating what's in the document since
it can change, but provide enough information that someone with
curiosity would know where to look.
Question from me: does "cache='none'" need to be set in the <driver>
element for this to work properly? Do any of the cache values make
sense? If it must be none, then we need to check that. If something
doesn't make ense, we need to check that too.
>
> Signed-off-by: Liu Qing <liuqing@xxxxxxxxxx>
> ---
>
> a) add new element qcow2 as drive's child, if any qcow2
> specific configuration exists.
> b) merge the three test cases to one.
> c) update docs. cache size only support bytes.
> d) move the parameters checking to virDomainDiskDefParseValidate
>
> docs/formatdomain.html.in | 43 ++++++++++
> docs/schemas/domaincommon.rng | 35 ++++++++
> src/conf/domain_conf.c | 95 +++++++++++++++++++++-
> src/qemu/qemu_command.c | 6 ++
> 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, 274 insertions(+), 1 deletion(-)
> create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-drive-qcow2-cache.xml
> create mode 100644 tests/qemuxml2xmloutdata/qemuxml2xmlout-disk-drive-qcow2-cache.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8ca7637..245d5c4 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3036,6 +3036,49 @@
> set. (<span class="since">Since 3.5.0</span>)
> </li>
> </ul>
> + The <code>drive</code> element may contain a qcow2 sub element, which
s/drive/driver/
s/qcow2/<code>qcow2</code>/
> + allows to specifying further details related to qcow2 driver type.
s/allows//
s/to qcow2 driver type/to a qcow2 disk./
Then, I'd add:
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. The amount of virtual disk that can be mapped by
> + the L2 caches (in bytes) is:
> + disk_size = l2_cache_size * cluster_size / 8
The one concern I have about providing an example is that it changes in
the qemu source docs and isn't reflected here. Beyond that someone
would have to know how to determine their cluster_size or perhaps
remember from when the volume was created.
> + <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. The amount of virtual disk that
> + can be mapped by the refcount caches (in bytes) is:
> + disk_size = refcount_cache_size * cluster_size * 8 / refcount_bits
Similar here and here you introduce refcount_bits which kind of doesn't
make much sense.
> + <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 06c5a91..ec44e58 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1762,6 +1762,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">
> @@ -1800,6 +1817,9 @@
> <ref name="detect_zeroes"/>
> </optional>
> <ref name="virtioOptions"/>
> + <zeroOrMore>
> + <ref name="qcow2Driver"/>
> + </zeroOrMore>
While multiple <qcow2> sub-elements could be had in the source, only the
last setting for any value is used... Thus:
<driver name='qemu' type='qcow2' cache='none'>
<qcow2 l2_cache_size='2097152' refcount_cache_size='524288'
cache_clean_interval='900'/>
<qcow2 cache_clean_interval='1'/>
</driver>
would conceivably overwrite the previous.... I know bad example, but
supporting multiple <qcow2> sub elements will be "forever"...
> <empty/>
> </element>
> </define>
> @@ -1895,6 +1915,21 @@
> </choice>
> </attribute>
> </define>
> + <define name="qcow2_l2_cache_size">
> + <attribute name='l2_cache_size'>
> + <ref name="unsignedInt"/>
> + </attribute>
> + </define>
> + <define name="qcow2_refcount_cache_size">
> + <attribute name='refcount_cache_size'>
> + <ref name="unsignedInt"/>
> + </attribute>
> + </define>
> + <define name="qcow2_cache_clean_interval">
> + <attribute name='cache_clean_interval'>
> + <ref name="unsignedInt"/>> + </attribute>
> + </define>
These would all be unsignedLong values
> <define name="controller">
> <element name="controller">
> <optional>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 2fc1fc3..c7d4b9b 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -5673,6 +5673,28 @@ virDomainDeviceLoadparmIsValid(const char *loadparm)
>
>
> static void
> +virDoaminQcow2CacheOptionsFormat(virBufferPtr buf,
Domain
Also, I think if you follow the virDomainDiskSourceFormatInternal model,
this changes to take as input a driverQcow2Buf pointer... and the
following changes:
> + virDomainDiskDefPtr def)
> +{
> + virBuffer qcow2Buff = VIR_BUFFER_INITIALIZER;
Remove the above, add:
+ if (def->src->l2_cache_size == 0 &&
+ def->src->refcount_cache_size == 0 &&
+ def->src->cache_clean_interval == 0)
+ return;
+
+ virBufferAddLit(driverQcow2Buf, "<qcow2");
+
> + if (def->src->l2_cache_size > 0)
> + virBufferAsprintf(&qcow2Buff, " l2_cache_size='%llu'",
> + def->src->l2_cache_size);
> + if (def->src->refcount_cache_size > 0)
> + virBufferAsprintf(&qcow2Buff, " refcount_cache_size='%llu'",
> + def->src->refcount_cache_size);
> + if (def->src->cache_clean_interval > 0)
> + virBufferAsprintf(&qcow2Buff, " cache_clean_interval='%llu'",
> + def->src->cache_clean_interval);
Change all the above &qcow2Buff w/ driverQcow2Buf, then alter then
remove all the following and replace with:
+ virBufferAddLit(driverQcow2Buf, "/>\n");
see below [1] for more context
> +
> + if (virBufferUse(&qcow2Buff)) {
> + virBufferAddLit(buf, "<qcow2");
> + virBufferAddBuffer(buf, &qcow2Buff);
> + virBufferAddLit(buf, "/>\n");> + }
> +}
> +
Another blank line
> +static void
> virDomainVirtioOptionsFormat(virBufferPtr buf,
> virDomainVirtioOptionsPtr virtio)
> {
> @@ -8511,15 +8533,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;
> + }
> +
Consider my earlier comment is there any setting of the cache attribute
that needs to also be checked?
It's also dawning on me the strangeness of seeing "cache='none'"
attribute and the various cache attributes for the qcow2 sub element.
> 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);
superfluous, but no big deal
> +
> + 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");
> @@ -8622,6 +8698,12 @@ 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;
> + }
> + }
Add a blank line
> ret = 0;
>
> cleanup:
> @@ -21908,7 +21990,18 @@ virDomainDiskDefFormat(virBufferPtr buf,
[1]
Add:
+ virBuffer driverQcow2Buf = VIR_BUFFER_INITIALIZER;
below driverBuf
and the rest of this changes to:
+ virBufferSetChildIndent(&driverQcow2Buf, buf); //&driverBuf);
+ virDomainQcow2CacheOptionsFormat(&driverQcow2Buf, def);
+
+ if (virXMLFormatElement(buf, "driver", &driverBuf, &driverQcow2Buf)
< 0)
+ return -1;
Slick and compact!
> if (virBufferUse(&driverBuf)) {
> virBufferAddLit(buf, "<driver");
> virBufferAddBuffer(buf, &driverBuf);
> - virBufferAddLit(buf, "/>\n");
> +
> + if (def->src->l2_cache_size == 0 &&
> + def->src->refcount_cache_size == 0 &&
> + def->src->cache_clean_interval == 0) {
> + virBufferAddLit(buf, "/>\n");
> + } else {
> + virBufferAddLit(buf, ">\n");
> + virBufferAdjustIndent(buf, 2);
> + virDoaminQcow2CacheOptionsFormat(buf, def);
> + virBufferAdjustIndent(buf, -2);
> + virBufferAddLit(buf, "</driver>\n");
> + }
> }
>
> if (def->src->auth) {
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index ed8cb6e..391aaba 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1433,6 +1433,12 @@ qemuBuildDriveSourceStr(virDomainDiskDefPtr disk,
> qemuformat = "luks";
> virBufferAsprintf(buf, "format=%s,", qemuformat);
> }
> + if (disk->src->l2_cache_size > 0)
> + virBufferAsprintf(buf, "l2-cache-size=%llu,", disk->src->l2_cache_size);
> + if (disk->src->refcount_cache_size > 0)
> + virBufferAsprintf(buf, "refcount-cache-size=%llu,", disk->src->refcount_cache_size);
> + if (disk->src->cache_clean_interval > 0)
> + virBufferAsprintf(buf, "cache-clean-interval=%llu,", disk->src->cache_clean_interval);
These don't belong in this patch, they belong in the next patch. We're
not adding the .args here.
I can make all the above changes for you - not a problem. But please
let me know your thoughts on whether we should keep the sizing
recommendation in formatdomain.html.in or just point at the qemu source
document. It's a fine line - I like the example, but I also see the
downside.
John
>
> 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..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 97ff36c..1ffb831 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
