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

[John Ferlan]

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.

> +            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 */
>