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@xxxxxxxxxxxxx>
> ---
> 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;
>
