On Tue, Aug 31, 2021 at 20:50:04 +0800, yuxiating wrote:
> DISCARD and WRITE_ZEROES features for machine type >= 4.0 is enabled by default since
> commit 5c81161f804144b146607f890e84613a4cbad95c
> virtio-blk: add "discard" and "write-zeroes" properties
> Sometimes guestos has bugs DISCARD need to be disabled.
I'm not very persuaded by this commit message, could you please
elaborate how the bugs show themselves?
Specifically this fells like a hack/workaround rather so a proper
justification would be great.
> Signed-off-by: yuxiating <yuxiating@xxxxxxxxxx>
> ---
> src/conf/domain_conf.c | 15 +++++++++++++++
> src/conf/domain_conf.h | 9 +++++++++
> src/conf/domain_validate.c | 6 ++++++
> src/libvirt_private.syms | 3 ++-
> src/qemu/qemu_command.c | 11 +++++++++++
> 5 files changed, 43 insertions(+), 1 deletion(-)
Please split the conf/* changes from the qemu changes.
Also you didn't add any documentation to docs/formatdomain.rst which
describes the XML which is unacceptable.
Additionally you are also missing test cases for qemuxml2argvtest.
>
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 6127513117..bfe4721e60 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -1278,6 +1278,13 @@ VIR_ENUM_IMPL(virDomainDiskDiscard,
> "ignore",
> );
>
> +VIR_ENUM_IMPL(virDomainDiskDiscardEnable,
> + VIR_DOMAIN_DISK_DISCARD_ENABLE_LAST,
> + "default",
> + "off",
> + "on",
NACK to this hunk, 'default' value feels weird, and for the rest we have
the virTristateBool type.
> +);
> +
> VIR_ENUM_IMPL(virDomainDiskDetectZeroes,
> VIR_DOMAIN_DISK_DETECT_ZEROES_LAST,
> "default",
> @@ -8930,6 +8937,10 @@ virDomainDiskDefDriverParseXML(virDomainDiskDef *def,
> if (virXMLPropUInt(cur, "queues", 10, VIR_XML_PROP_NONE, &def->queues) < 0)
> return -1;
>
> + if (virXMLPropEnum(cur, "discard_enable", virDomainDiskDiscardEnableTypeFromString,
> + VIR_XML_PROP_NONZERO, &def->discard_enable) < 0)
> + return -1;
> +
> return 0;
> }
>
> @@ -23416,6 +23427,10 @@ virDomainDiskDefFormatDriver(virBuffer *buf,
> if (disk->queues)
> virBufferAsprintf(&attrBuf, " queues='%u'", disk->queues);
>
> + if (disk->discard_enable)
> + virBufferAsprintf(&attrBuf, " discard_enable='%s'",
> + virDomainDiskDiscardEnableTypeToString(disk->discard_enable));
It even behaves exactly like virTristateBool.
> +
> virDomainVirtioOptionsFormat(&attrBuf, disk->virtio);
>
> if (disk->src->metadataCacheMaxSize > 0) {
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index c7e6df7981..c39694a19e 100644
> --- a/src/conf/domain_conf.h
> +++ b/src/conf/domain_conf.h
[...]
> diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
> index ab8a6c00c3..52a74dd2d5 100644
> --- a/src/libvirt_private.syms
> +++ b/src/libvirt_private.syms
> @@ -1,5 +1,5 @@
> -#
> # General private symbols. Add symbols here, and see src/meson.build for
> +# mainDiskDeviceTypeToString
> # more details.
> #
> # Keep this file sorted by header name, then by symbols with each header.
NACK to this hunk too, you shouldn't need to modify the header here.
> @@ -377,6 +377,7 @@ virDomainDiskDefParseSource;
> virDomainDiskDetectZeroesTypeFromString;
> virDomainDiskDetectZeroesTypeToString;
> virDomainDiskDeviceTypeToString;
> +virDomainDiskDiscardEnableTypeToString;
> virDomainDiskDiscardTypeToString;
> virDomainDiskEmptySource;
> virDomainDiskErrorPolicyTypeFromString;
> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b230314f7f..894c8b17b9 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1739,6 +1739,17 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
> virBufferAsprintf(&opt, ",num-queues=%u", disk->queues);
> }
>
> + if (disk->discard_enable) {
> + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE_DISCARD)) {
This capability was also checked in the validator so you don't need to
do this here.
> + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> + _("virtio-blk discard property isn't supported by this "
> + "QEMU binary"));
> + return NULL;
> + }
> + virBufferAsprintf(&opt, ",discard=%s",
> + virDomainDiskDiscardEnableTypeToString(disk->discard_enable));
> + }
> +
> qemuBuildVirtioOptionsStr(&opt, disk->virtio);
>
> if (qemuBuildDeviceAddressStr(&opt, def, &disk->info) < 0)
> --
> 2.27.0
>
>
