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.

Virtio_blk kernel driver has a bug that causes memory corruption in virtblk_setup_discard_write_zeroes(); af822aa68fbd ("block: virtio_blk: fix handling single range discard request") has fix it. However, some operating systems are not fixed and need to disabled on the QEMU side.

...

Signed-off-by: yuxiating <yuxiating@huawei.com> --- 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.

I will add a description to docs/ formatDomain.rst. qemuxml2argvtest will be added in version V2.

...

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.

I find virTristateSwitch more appropriate.

...

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

I'll use virTristateSwitch instead header.

NACK to this hunk too, you shouldn't need to modify the header here.

It was my mistake.

...

@@ -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"));

I'm going to delete this check.

...

+ return NULL; + } + virBufferAsprintf(&opt, ",discard=%s", + virDomainDiskDiscardEnableTypeToString(disk->discard_enable)); + } + qemuBuildVirtioOptionsStr(&opt, disk->virtio);

if (qemuBuildDeviceAddressStr(&opt, def, &disk->info) < 0) -- 2.27.0