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(a)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 */
>