On 12/11/18 6:24 AM, Nikolay Shirokovskiy wrote:
>
>
> 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...
>
Right, my point was more putting a comment here at least acknowledges
the existence of the other parameters which were specifically not chosen
for particular reasons.
>>
>>> + 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).
Fair enough - Z would be something considerable if you think about my
most recent comment and idea for providing some value for 2.12 and 3.0
from patch 2. Otherwise, 'I' could stay. Using 'J' or 'Y' would be only
if the user provided value was used directly... 'U' has "interesting"
QEMU side effects as described in virJSONValueObjectAddVArgs
Mostly noted to follow up previous comments so that if any other changes
to the algorithm were made that we didn't forget this spot.
John
>
> 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
