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@xxxxxxxxxxxxx>
>> ---
>> 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 */
>>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
