Re: [libvirt] [PATCH v2 3/4] qemu: support metadata-cache-size for blockdev

On 11/8/18 8:02 AM, Nikolay Shirokovskiy wrote: > Just set l2-cache-size to INT64_MAX for all format nodes of > qcow2 type in block node graph. > > -drive configuration is not supported because we can not > set l2 cache size down the backing chain in this case. > > Note that imlementation sets l2-cache-size and not cache-size. implementation > Unfortunately at time of this patch setting cache-size to INT64_MAX > fails and as guest performance depends only on l2 cache size > and not refcount cache size (which is documented in recent qemu) > we can set l2 directly. More fodder for the let's not take the all or nothing approach. Say nothing of introducing cache-size and refcount-cache-size terminology in a commit message when I believe it'd be better in code... > > Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy(a)virtuozzo.com&gt; > --- > src/qemu/qemu_block.c | 5 ++++- > src/qemu/qemu_command.c | 23 +++++++++++++++++++++++ > src/qemu/qemu_domain.c | 2 ++ > src/util/virstoragefile.c | 1 + > src/util/virstoragefile.h | 1 + > 5 files changed, 31 insertions(+), 1 deletion(-) > > diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c > index 5321dda..8771cc1 100644 > --- a/src/qemu/qemu_block.c > +++ b/src/qemu/qemu_block.c > @@ -1322,7 +1322,6 @@ qemuBlockStorageSourceGetFormatQcow2Props(virStorageSourcePtr src, > * 'pass-discard-snapshot' > * 'pass-discard-other' > * 'overlap-check' > - * 'l2-cache-size' > * 'l2-cache-entry-size' > * 'refcount-cache-size' > * 'cache-clean-interval' > @@ -1331,6 +1330,10 @@ qemuBlockStorageSourceGetFormatQcow2Props(virStorageSourcePtr src, > if (qemuBlockStorageSourceGetFormatQcowGenericProps(src, "qcow2", props) < 0) > return -1; > I think this is where you indicate why l2-cache-size is only being used and what "downside" there is to adding 'cache-size' and/or 'refcount-cache-size'. If I'm reading code, it's a lot "nicer" to find that information when reading rather than having to go find the commit where this was added and find the comment about why something wasn't added.

> + if (src->metadata_cache_size == VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM &&> + virJSONValueObjectAdd(props, "I:l2-cache-size", INT64_MAX, NULL) < 0) QEMU uses "INT_MAX" which is different than INT64_MAX by a few magnitudes - although the math in the code may help in this case.> As for "I"... Maybe "Z" or "Y" would be better choices... or "U"... The json schema can accept an 'int' although read_cache_sizes seems to allow a uint64_t although perhaps limited (I didn't have the energy to follow the math).

The rest of the changes could be different based on the patch1 adjustments. John > + return -1; > +> return 0; > } > > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c > index f59cbf5..12b2c8d 100644 > --- a/src/qemu/qemu_command.c > +++ b/src/qemu/qemu_command.c > @@ -1330,6 +1330,20 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk, > return -1; > } > > + if (disk->metadata_cache_size) { > + if (disk->src->format != VIR_STORAGE_FILE_QCOW2) { > + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", > + _("metadata_cache_size can only be set for qcow2 disks")); > + return -1; > + } > + > + if (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 +1367,15 @@ qemuCheckDiskConfig(virDomainDiskDefPtr disk, > _("detect_zeroes is not supported by this QEMU binary")); > return -1; > } > + > + if (disk->metadata_cache_size && > + !(virQEMUCapsGet(qemuCaps, QEMU_CAPS_BLOCKDEV) && > + virQEMUCapsGet(qemuCaps, QEMU_CAPS_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 && > diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c > index 045a7b4..23d9348 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 > @@ -13244,6 +13245,7 @@ qemuDomainPrepareDiskSourceData(virDomainDiskDefPtr disk, > src->iomode = disk->iomode; > src->cachemode = disk->cachemode; > src->discard = disk->discard; > + src->metadata_cache_size = disk->metadata_cache_size; > > if (disk->device == VIR_DOMAIN_DISK_DEVICE_FLOPPY) > src->floppyimg = true; > diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c > index 94c32d8..9089e2f 100644 > --- a/src/util/virstoragefile.c > +++ b/src/util/virstoragefile.c > @@ -2210,6 +2210,7 @@ virStorageSourceCopy(const virStorageSource *src, > ret->cachemode = src->cachemode; > ret->discard = src->discard; > ret->detect_zeroes = src->detect_zeroes; > + ret->metadata_cache_size = src->metadata_cache_size; > > /* storage driver metadata are not copied */ > ret->drv = NULL; > diff --git a/src/util/virstoragefile.h b/src/util/virstoragefile.h > index 3ff6c4f..8b57399 100644 > --- a/src/util/virstoragefile.h > +++ b/src/util/virstoragefile.h > @@ -331,6 +331,7 @@ struct _virStorageSource { > int cachemode; /* enum virDomainDiskCache */ > int discard; /* enum virDomainDiskDiscard */ > int detect_zeroes; /* enum virDomainDiskDetectZeroes */ > + int metadata_cache_size; /* enum virDomainDiskImageMetadataCacheSize */ > > bool floppyimg; /* set to true if the storage source is going to be used > as a source for floppy drive */ >