[PATCH 1/1] qemu: add virtio-blk queue-size option

[Peter Krempa]

On Tue, Aug 31, 2021 at 18:51:07 +0900, Hiroki Narukawa wrote:
> The option "queue-size" in virtio-blk was added in qemu-2.12.0, and default value increased from qemu-5.0.0.
> However, increasing this value may lead to drop of random access performance.
> This is configurable value, so we want to use it via libvirt.
> 
> Signed-off-by: Hiroki Narukawa <hnarukaw at yahoo-corp.jp>
> ---
>  docs/schemas/domaincommon.rng                 |  5 +++
>  src/conf/domain_conf.c                        |  6 ++++
>  src/conf/domain_conf.h                        |  1 +
>  src/qemu/qemu_capabilities.c                  |  5 +++
>  src/qemu/qemu_capabilities.h                  |  1 +
>  src/qemu/qemu_command.c                       |  3 ++
>  src/qemu/qemu_validate.c                      |  7 ++++
>  .../caps_2.12.0.aarch64.xml                   |  1 +
>  .../caps_2.12.0.s390x.xml                     |  1 +
>  .../qemucapabilitiesdata/caps_3.0.0.ppc64.xml |  1 +
>  .../caps_3.0.0.riscv32.xml                    |  1 +
>  .../caps_3.0.0.riscv64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_3.0.0.s390x.xml |  1 +
>  .../caps_3.0.0.x86_64.xml                     |  1 +
>  .../qemucapabilitiesdata/caps_3.1.0.ppc64.xml |  1 +
>  .../caps_3.1.0.x86_64.xml                     |  1 +
>  .../caps_4.0.0.aarch64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_4.0.0.ppc64.xml |  1 +
>  .../caps_4.0.0.riscv32.xml                    |  1 +
>  .../caps_4.0.0.riscv64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_4.0.0.s390x.xml |  1 +
>  .../caps_4.0.0.x86_64.xml                     |  1 +
>  .../caps_4.1.0.x86_64.xml                     |  1 +
>  .../caps_4.2.0.aarch64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_4.2.0.ppc64.xml |  1 +
>  .../qemucapabilitiesdata/caps_4.2.0.s390x.xml |  1 +
>  .../caps_4.2.0.x86_64.xml                     |  1 +
>  .../caps_5.0.0.aarch64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_5.0.0.ppc64.xml |  1 +
>  .../caps_5.0.0.riscv64.xml                    |  1 +
>  .../caps_5.0.0.x86_64.xml                     |  1 +
>  .../qemucapabilitiesdata/caps_5.1.0.sparc.xml |  1 +
>  .../caps_5.1.0.x86_64.xml                     |  1 +
>  .../caps_5.2.0.aarch64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_5.2.0.ppc64.xml |  1 +
>  .../caps_5.2.0.riscv64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_5.2.0.s390x.xml |  1 +
>  .../caps_5.2.0.x86_64.xml                     |  1 +
>  .../caps_6.0.0.aarch64.xml                    |  1 +
>  .../qemucapabilitiesdata/caps_6.0.0.s390x.xml |  1 +
>  .../caps_6.0.0.x86_64.xml                     |  1 +
>  .../caps_6.1.0.x86_64.xml                     |  1 +
>  .../disk-virtio-queue-size.args               | 29 +++++++++++++++
>  .../disk-virtio-queue-size.xml                | 35 +++++++++++++++++++
>  tests/qemuxml2argvtest.c                      |  2 ++
>  .../disk-virtio-queue-size.xml                | 35 +++++++++++++++++++
>  tests/qemuxml2xmltest.c                       |  1 +
>  47 files changed, 165 insertions(+)
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.args
>  create mode 100644 tests/qemuxml2argvdata/disk-virtio-queue-size.xml
>  create mode 100644 tests/qemuxml2xmloutdata/disk-virtio-queue-size.xml

You'll have to split this into multiple patches. Generally we split it
into following logical steps (note that after every single patch the
tree must build cleanly)

1) XML/public api changes
2) qemu capability probing changes
3) qemu implementation and test updates

[...]

> @@ -5053,6 +5054,10 @@ virQEMUCapsInitQMPBasicArch(virQEMUCaps *qemuCaps)
>  static void
>  virQEMUCapsInitQMPVersionCaps(virQEMUCaps *qemuCaps)
>  {
> +    /* virtio-blk queue-size is added on QEMU 2.12 */
> +    if (qemuCaps->version >= 2012000)
> +        virQEMUCapsSet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE);

NACK to this hunk, there should be a reasonable way to detect this other
than version check by probing the properties of virtio-blk which we
should already be doing.

> +
>      /* -enable-fips is deprecated in QEMU 5.2.0, and QEMU
>       * should be built with gcrypt to achieve FIPS compliance
>       * automatically / implicitly

[...]

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index b230314f7f..5c360b7c6f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -1738,6 +1738,9 @@ qemuBuildDiskDeviceStr(const virDomainDef *def,
>          if (disk->queues) {
>              virBufferAsprintf(&opt, ",num-queues=%u", disk->queues);
>          }
> +        if (disk->queue_size) {

Please use an explicit '> 0' check here since it's not a boolean.

> +            virBufferAsprintf(&opt, ",queue-size=%u", disk->queue_size);
> +        }
>  
>          qemuBuildVirtioOptionsStr(&opt, disk->virtio);
>  
> diff --git a/src/qemu/qemu_validate.c b/src/qemu/qemu_validate.c
> index 8906aa52d9..c72aaa2163 100644
> --- a/src/qemu/qemu_validate.c
> +++ b/src/qemu/qemu_validate.c
> @@ -2822,6 +2822,13 @@ qemuValidateDomainDeviceDefDiskFrontend(const virDomainDiskDef *disk,
>                               "QEMU binary"));
>              return -1;
>          }
> +        if (disk->queue_size &&

same as above.

> +            !virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_QUEUE_SIZE)) {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("queue-size property isn't supported by this "
> +                             "QEMU binary"));

Error messages on a single line please.

> +            return -1;
> +        }
>          break;
>