On Thu, Nov 01, 2018 at 14:32:23 +0300, Nikolay Shirokovskiy wrote:
> The only possible value is 'maximum' which makes l2_cache_size large
> enough to keep all metadata in memory. This will unleash disks
> performace for some database workloads and IO benchmarks with random
> access to whole disk.
>
> Note that imlementation sets l2-cache-size and not cache-size.
> Both *cache-size's is upper limit on cache size value thus instead of
> setting precise limit for disk which involves knowing disk size
> and disk's cluster size we can just set INT64_MAX. Unfortunately
> both old and new versions of qemu fail on setting cache-size to INT64_MAX.
> Fortunately both old and new versions works well on such setting for
> l2-cache-size. As guest performance depends only l2 cache size
> and not refcount cache size (which is documented in recent qemu)
> we can set l2 directly.
>
> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@xxxxxxxxxxxxx>
> ---
> docs/formatdomain.html.in | 7 ++++
> docs/schemas/domaincommon.rng | 10 +++++
> src/conf/domain_conf.c | 17 ++++++++
> src/conf/domain_conf.h | 9 ++++
> src/qemu/qemu_command.c | 26 ++++++++++++
> src/qemu/qemu_domain.c | 1 +
> .../qemuxml2argvdata/disk-metadata_cache_size.args | 34 +++++++++++++++
> .../qemuxml2argvdata/disk-metadata_cache_size.xml | 42 +++++++++++++++++++
> tests/qemuxml2argvtest.c | 2 +
> .../disk-metadata_cache_size.xml | 48 ++++++++++++++++++++++
> tests/qemuxml2xmltest.c | 2 +
> 11 files changed, 198 insertions(+)
> create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.args
> create mode 100644 tests/qemuxml2argvdata/disk-metadata_cache_size.xml
> create mode 100644 tests/qemuxml2xmloutdata/disk-metadata_cache_size.xml
>
> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
> index 8189959..93e0009 100644
> --- a/docs/formatdomain.html.in
> +++ b/docs/formatdomain.html.in
> @@ -3556,6 +3556,13 @@
> virt queues for virtio-blk. (<span class="since">Since 3.9.0</span>)
> </li>
> <li>
> + The optional <code>metadata_cache_size</code> attribute specifies
> + metadata cache size policy. The only possible value is "maximum" to
> + keep all metadata in cache, this will help if workload needs access
> + to whole disk all the time. (<span class="since">Since
> + 4.9.0</span>)
I wanted to complain that we prefer camelCase to underscores generally,
but given that the <driver> element has at least 4 attributes using
underscores that point would be moot.
What's missing though is the description of the default value when the
attribute is not present. Also I think that we should allow to pass
"default" as a valid argument.
> + </li>
> + <li>
> For virtio disks,
> <a href="#elementsVirtio">Virtio-specific options</a> can also be
> set. (<span class="since">Since 3.5.0</span>)
> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
> index 099a949..18efa3a 100644
> --- a/docs/schemas/domaincommon.rng
> +++ b/docs/schemas/domaincommon.rng
> @@ -1990,6 +1990,9 @@
> <ref name="detect_zeroes"/>
> </optional>
> <optional>
> + <ref name="metadata_cache_size"/>
> + </optional>
> + <optional>
> <attribute name='queues'>
> <ref name="positiveInteger"/>
> </attribute>
> @@ -2090,6 +2093,13 @@
> </choice>
> </attribute>
> </define>
> + <define name="metadata_cache_size">
> + <attribute name='metadata_cache_size'>
> + <choice>
> + <value>maximum</value>
Here default should be allowed as well.
> + </choice>
> + </attribute>
> + </define>
> <define name="controller">
> <element name="controller">
> <optional>
[...]
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 1ff593c..b33e6a5 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1330,6 +1330,21 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
> return -1;
> }
>
> + if (disk->metadata_cache_size &&
> + (disk->src->type != VIR_STORAGE_TYPE_FILE ||
Why don't do this for network-based qcow2s?
> + disk->src->format != VIR_STORAGE_FILE_QCOW2)) {
Note that a QCOW2 can also be a part of the backing chain where the top
image format is not qcow2. In such case it would not work. Without
-blockdev support I don't see a possibility to expose that to qemu
though since I expect the -drive command being rejected if the top image
is not a qcow2.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata_cache_size can only be set for qcow2 disks"));
> + return -1;
> + }
> +
> + if (disk->metadata_cache_size &&
This part is common to both of the above conditions.
> + disk->metadata_cache_size != VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("metadata_cache_size can only be set to 'maximum'"));
> + return -1;
> + }
> +
> if (qemuCaps) {
> if (disk->serial &&
> disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
> @@ -1353,6 +1368,14 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk,
> _("detect_zeroes is not supported by this QEMU binary"));
> return -1;
> }
> +
> + if (disk->metadata_cache_size &&
> + !virQEMUCapsGet(qemuCaps, QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE)) {
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("setting metadata_cache_size is not supported by "
> + "this QEMU binary"));
> + return -1;
> + }
> }
>
> if (disk->serial &&
> @@ -1776,6 +1799,9 @@ qemuBuildDriveStr(virDomainDiskDefPtr disk,
> virDomainDiskIoTypeToString(disk->iomode));
Additions to XML should be in a separate patch from actual implementation.
> }
>
> + if (disk->metadata_cache_size == VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM)
> + virBufferAsprintf(&opt, ",l2-cache-size=%ld", INT64_MAX);
> +
> qemuBuildDiskThrottling(disk, &opt);
>
> if (virBufferCheckError(&opt) < 0)
> diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c
> index ba3fff6..896adf3 100644
> --- a/src/qemu/qemu_domain.c
> +++ b/src/qemu/qemu_domain.c
> @@ -9074,6 +9074,7 @@ qemuDomainDiskChangeSupported(virDomainDiskDefPtr disk,
> /* "snapshot" is a libvirt internal field and thus can be changed */
> /* startupPolicy is allowed to be updated. Therefore not checked here. */
> CHECK_EQ(transient, "transient", true);
> + CHECK_EQ(metadata_cache_size, "metadata_cache_size", true);
>
> /* Note: For some address types the address auto generation for
> * @disk has still not happened at this point (e.g. driver
[...]
> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c
> index 39a7f1f..a0a2ff3 100644
> --- a/tests/qemuxml2argvtest.c
> +++ b/tests/qemuxml2argvtest.c
> @@ -3044,6 +3044,8 @@ mymain(void)
> DO_TEST_CAPS_ARCH_LATEST("x86_64-pc-headless", "x86_64");
> DO_TEST_CAPS_ARCH_LATEST("x86_64-q35-headless", "x86_64");
>
> + DO_TEST("disk-metadata_cache_size", QEMU_CAPS_DRIVE_QCOW2_L2_CACHE_SIZE);
Please use DO_TEST_LATEST for this rather than hardcoding caps.
> +
> if (getenv("LIBVIRT_SKIP_CLEANUP") == NULL)
> virFileDeleteTree(fakerootdir);
>
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
