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

On 10.12.2018 20:00, John Ferlan wrote: > > > 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. Well from my POV the reason we use l2-cache-size here is straightforward - we actually need to tune this parameter for performance. refcount cache makes difference only for snapshots (at least this is what I read from qemu docs) and cache-size is a kind of combined cache, a convinience tool. May be still refcount need to be tuned in accordance with l2-cache size but this is not clear from qemu description, so at least to me this place does not seem cryptic or something...

> >> + 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).

It can be any one of these I guess as we have only one value yet. Nikolay > > 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 */ >>