8:31 a.m.
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(a)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.
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