[PATCH 2/2] qemu: support for virtio-blk-pci discard options
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. 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(-) 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", +); + 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)); + 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 @@ -457,6 +457,13 @@ typedef enum { VIR_DOMAIN_DISK_DISCARD_LAST } virDomainDiskDiscard; +typedef enum { + VIR_DOMAIN_DISK_DISCARD_ENABLE_DEFAULT = 0, + VIR_DOMAIN_DISK_DISCARD_ENABLE_OFF, + VIR_DOMAIN_DISK_DISCARD_ENABLE_ON, + VIR_DOMAIN_DISK_DISCARD_ENABLE_LAST +} virDomainDiskDiscardEnable; + typedef enum { VIR_DOMAIN_DISK_DETECT_ZEROES_DEFAULT = 0, VIR_DOMAIN_DISK_DETECT_ZEROES_OFF, @@ -589,6 +596,7 @@ struct _virDomainDiskDef { bool diskElementAuth; bool diskElementEnc; + virDomainDiskDiscardEnable discard_enable; }; @@ -3838,6 +3846,7 @@ VIR_ENUM_DECL(virDomainDiskIo); VIR_ENUM_DECL(virDomainDeviceSGIO); VIR_ENUM_DECL(virDomainDiskTray); VIR_ENUM_DECL(virDomainDiskDiscard); +VIR_ENUM_DECL(virDomainDiskDiscardEnable); VIR_ENUM_DECL(virDomainDiskDetectZeroes); VIR_ENUM_DECL(virDomainDiskModel); VIR_ENUM_DECL(virDomainDiskMirrorState); diff --git a/src/conf/domain_validate.c b/src/conf/domain_validate.c index 60f7ccdddd..6eb346916a 100644 --- a/src/conf/domain_validate.c +++ b/src/conf/domain_validate.c @@ -612,6 +612,12 @@ virDomainDiskDefValidate(const virDomainDef *def, return -1; } + if (disk->discard_enable) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("discard_enable attribute in disk driver element is only supported by virtio-blk")); + return -1; + } + if (disk->event_idx != VIR_TRISTATE_SWITCH_ABSENT) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", _("disk event_idx mode supported only for virtio bus")); 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. @@ -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)) { + 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
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@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
participants (2)
-
Peter Krempa -
yuxiating