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@virtuozzo.com> --- 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);