[PATCH 2/2] qemu: support for virtio-blk-pci discard options

[y005616296]

>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 at 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.
>

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
>>
>>