[libvirt] [PATCH v3 3/5] qemu: support metadata-cache-size for blockdev

[Nikolay Shirokovskiy]

On 23.01.2019 00:45, John Ferlan wrote:
> 
> 
> On 1/10/19 7:15 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.
>>
>> Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy at 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(-)
>>
> 
> I guess I find it "strange" to have a driver argument be copied into
> storage source, but I guess that just an "implementation detail" that I
> have to live with.
> 
>> diff --git a/src/qemu/qemu_block.c b/src/qemu/qemu_block.c
>> index cbf0aa4..5f98b85 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;
>>  
>> +    if (src->metadata_cache_size == VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM &&
>> +        virJSONValueObjectAdd(props, "I:l2-cache-size", INT64_MAX, NULL) < 0)
>> +        return -1;
>> +
>>      return 0;
>>  }
>>  
>> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
>> index 822d5f8..96d6601 100644
>> --- a/src/qemu/qemu_command.c
>> +++ b/src/qemu/qemu_command.c
>> @@ -1328,6 +1328,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'"));
> 
> Since the only way to get here is if metadata_cache_size is set and the
> only way it's set when it's VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MAXIMUM,
> then I see this as unnecessary...
> 
> If you think the future holds more than "minimum" (0) and "maximum" (1),
> then change the condition into here to be metadata_cache_size >
> VIR_DOMAIN_DISK_METADATA_CACHE_SIZE_MINIMUM
> 
> Even with that I wouldn't bother with this check - leave that for a
> future change.

Yes, this is future proof. So that if one adds one more value to enum we get error immediately if
someone tries to use newly added value. Without the check the qemuBlockStorageSourceGetFormatQcow2Props
will simply ignore new value. We could put the check into qemuBlockStorageSourceGetFormatQcow2Props itself
but looks like checking functionality resides in qemuCheckDiskConfig.

Nikolay

> 
>> +            return -1;
>> +        }
>> +    }
>> +
>>      if (qemuCaps) {
>>          if (disk->serial &&
>>              disk->bus == VIR_DOMAIN_DISK_BUS_SCSI &&
>> @@ -1351,6 +1365,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_CAPPED))) {
> 
> Even though l2-cache-size needs -blockdev (qemu 3.0+) support to add the
> field is only qemu 3.1+, so using BLOCKDEV here just becomes a mechanism
> to note what type of support requires BLOCKDEV.
> 
> IDC if the check stays - it's just superfluous then.
> 
> John
> 
>> +            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 ec6b340..fc5c7c2 100644
>> --- a/src/qemu/qemu_domain.c
>> +++ b/src/qemu/qemu_domain.c
>> @@ -9202,6 +9202,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
>> @@ -13370,6 +13371,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 bd4b027..e5660e6 100644
>> --- a/src/util/virstoragefile.c
>> +++ b/src/util/virstoragefile.c
>> @@ -2208,6 +2208,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 1d6161a..8bf5fe4 100644
>> --- a/src/util/virstoragefile.h
>> +++ b/src/util/virstoragefile.h
>> @@ -329,6 +329,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 */
>>