[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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 virtuozzo com>
>> ---
>>  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 */
>>


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]