[libvirt] [PATCH v2 3/4] qemu: support metadata-cache-size for blockdev

[John Ferlan]

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 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(-)
> 
> 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.

> +    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).

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