>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@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.
>
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.
>
I'll use virTristateSwitch instead
>> +
>> 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.
>
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
>>
>>
