Re: [PATCH 2/2] qemu: Support multiqueue virtio-blk

[Ján Tomko <jtomko@xxxxxxxxxx>]

On Fri, Sep 29, 2017 at 09:54:23PM +0800, Lin Ma wrote:
> @@ -3053,6 +3053,10 @@
>             <code>bus</code> and "pci" or "ccw" <code>address</code> types.
>             <span class='since'>Since 1.2.8 (QEMU 2.1)</span>
>           </li>
> +          <li>
> +            The optional <code>queues</code> attribute specifies the number of
> +            virt queues for virtio-blk. (<span class="since">Since 3.8.0</span>)

3.9.0, now that 3.8.0 is frozen

> +          </li>
>           <li>
>           For virtio disks,
>           <a href="#elementsVirtio">Virtio-specific options</a> can also be

> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 87192eb2d..90572d51a 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -8761,6 +8761,15 @@ virDomainDiskDefDriverParseXML(virDomainDiskDefPtr def,
>     }
>     VIR_FREE(tmp);
>
> +    if ((tmp = virXMLPropString(cur, "queues")) &&
> +        virStrToLong_ui(tmp, NULL, 10, &def->queues) < 0) {

virStrToLong_ui allows specifying a negative number and wraps it into a
positive number (e.g. -1 is a shortcut for UINT_MAX)
Please use virStrToLong_uip instead.

> +        virReportError(VIR_ERR_XML_ERROR,
> +                       _("'queues' attribute must be positive number: %s"),
> +                       tmp);
> +        goto cleanup;
> +    }
> +    VIR_FREE(tmp);
> +
>     ret = 0;
>
>  cleanup:
> @@ -21996,6 +22005,16 @@ virDomainDiskDefFormat(virBufferPtr buf,
>         virBufferAsprintf(&driverBuf, " iothread='%u'", def->iothread);
>     if (def->detect_zeroes)
>         virBufferAsprintf(&driverBuf, " detect_zeroes='%s'", detect_zeroes);
> +    if (def->queues) {
> +        if (def->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
> +            virBufferAsprintf(&driverBuf, " queues='%u'", def->queues);
> +        else {
> +            virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
> +                           _("queues attribute in disk driver element is only "
> +                             "supported by virtio-blk"));
> +            return -1;

This check does not belong in the formatter. If we parsed it, we should
be able to format it back.

Either only parse the attribute if the bus is DISK_BUS_VIRTIO, or add the check
to qemuDomain*DefValidate.

> +        }
> +    }
>
>     virDomainVirtioOptionsFormat(&driverBuf, def->virtio);

> diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> index 4f141e0ac..4d2787d8f 100644
> --- a/src/qemu/qemu_command.c
> +++ b/src/qemu/qemu_command.c
> @@ -2082,6 +2082,10 @@ qemuBuildDriveDevStr(const virDomainDef *def,
>                               (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN)
>                               ? "on" : "off");
>         }
> +        if (disk->queues &&
> +            virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_NUM_QUEUES)) {
> +            virBufferAsprintf(&opt, ",num-queues=%u", disk->queues);

If QEMU does not have the capability, it would be nice to report an
error to the user instead of quietly doing nothing with the attribute.
(The check also probably belongs in qemuDomain*DefValidate)

> +        }
>
>         if (qemuBuildVirtioOptionsStr(&opt, disk->virtio, qemuCaps) < 0)
>             goto error;

Jan

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list