[libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML: <controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/> The corresponding QEMU command line: -device virtio-scsi-pci,id=scsi0,cmd_per_lun=50,max_sectors=512, bus=pci.0,addr=0x3 Signed-off-by: Mike Perez <thingee@gmail.com> --- docs/formatdomain.html.in | 29 +++++++++++++++++++--- docs/schemas/domaincommon.rng | 10 ++++++++ src/conf/domain_conf.c | 27 ++++++++++++++++++-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 27 ++++++++++++++++---- .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 9 +++++++ .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml | 29 ++++++++++++++++++++++ .../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 9 +++++++ .../qemuxml2argv-disk-virtio-scsi-max_sectors.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 6 +++++ tests/qemuxml2xmltest.c | 2 ++ 11 files changed, 168 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.xml diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 691a451..8ffb247 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -2559,11 +2559,32 @@ <p> An optional sub-element <code>driver</code> can specify the driver - specific options. Currently it only supports attribute <code>queues</code> - (<span class="since">1.0.5</span>, QEMU and KVM only), which specifies the - number of queues for the controller. For best performance, it's recommended - to specify a value matching the number of vCPUs. + specific options: </p> + <dl> + <dt><code>queues</code></dt> + <dd> + The optional <code>queues</code> attribute specifies the number of + queues for the controller. For best performance, it's recommended to + specify a value matching the number of vCPUs. + <span class="since">Since 1.0.5 (QEMU and KVM only)</span> + </dd> + <dt><code>cmd_per_lun</code></dt> + <dd> + The optional <code>cmd_per_lun</code> attribute specifies the maximum + number of commands that can be queued on devices controlled by the + host. + <span class="since">Since 1.2.5 (QEMU and KVM only)</span> + </dd> + <dt><code>max_sectors</code></dt> + <dd> + The optional <code>max_sectors</code> attribute specifies the maximum + amount of data in bytes that will be transferred to or from the device + in a single command. The transfer length is measured in sectors, where + a sector is 512 bytes. + <span class="since">Since 1.2.5 (QEMU and KVM only)</span> + </dd> + </dl> <p> USB companion controllers have an optional sub-element <code><master></code> to specify the exact diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 4249ed5..4bf4699 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1735,6 +1735,16 @@ <ref name="unsignedInt"/> </attribute> </optional> + <optional> + <attribute name="cmd_per_lun"> + <ref name="unsignedInt"/> + </attribute> + </optional> + <optional> + <attribute name="max_sectors"> + <ref name="unsignedInt"/> + </attribute> + </optional> </element> </optional> </interleave> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 40c385e..4388cb8 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6041,6 +6041,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, char *idx = NULL; char *model = NULL; char *queues = NULL; + char *cmd_per_lun = NULL; + char *max_sectors = NULL; xmlNodePtr saved = ctxt->node; int rc; @@ -6084,6 +6086,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, if (cur->type == XML_ELEMENT_NODE) { if (xmlStrEqual(cur->name, BAD_CAST "driver")) queues = virXMLPropString(cur, "queues"); + cmd_per_lun = virXMLPropString(cur, "cmd_per_lun"); + max_sectors = virXMLPropString(cur, "max_sectors"); } cur = cur->next; } @@ -6094,6 +6098,17 @@ virDomainControllerDefParseXML(xmlNodePtr node, goto error; } + if (cmd_per_lun && virStrToLong_ui(cmd_per_lun, NULL, 10, &def->cmd_per_lun) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Malformed 'cmd_per_lun' value '%s'"), cmd_per_lun); + goto error; + } + + if (max_sectors && virStrToLong_ui(max_sectors, NULL, 10, &def->max_sectors) < 0) { + virReportError(VIR_ERR_XML_ERROR, + _("Malformed 'max_sectors' value %s'"), max_sectors); + } + if (virDomainDeviceInfoParseXML(node, NULL, &def->info, flags) < 0) goto error; @@ -6201,6 +6216,8 @@ virDomainControllerDefParseXML(xmlNodePtr node, VIR_FREE(idx); VIR_FREE(model); VIR_FREE(queues); + VIR_FREE(cmd_per_lun); + VIR_FREE(max_sectors); return def; @@ -15279,13 +15296,19 @@ virDomainControllerDefFormat(virBufferPtr buf, break; } - if (def->queues || virDomainDeviceInfoIsSet(&def->info, flags) || - pcihole64) { + if (def->queues || def->cmd_per_lun || def->max_sectors || + virDomainDeviceInfoIsSet(&def->info, flags) || pcihole64) { virBufferAddLit(buf, ">\n"); virBufferAdjustIndent(buf, 2); if (def->queues) virBufferAsprintf(buf, "<driver queues='%u'/>\n", def->queues); + if (def->cmd_per_lun) + virBufferAsprintf(buf, "<driver cmd_per_lun='%u'/>\n", def->cmd_per_lun); + + if (def->max_sectors) + virBufferAsprintf(buf, "<driver max_sectors='%u'/>\n", def->max_sectors); + if (virDomainDeviceInfoIsSet(&def->info, flags) && virDomainDeviceInfoFormat(buf, &def->info, flags) < 0) return -1; diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index bde303c..66484a1 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -720,6 +720,8 @@ struct _virDomainControllerDef { unsigned int idx; int model; /* -1 == undef */ unsigned int queues; + unsigned int cmd_per_lun; + unsigned int max_sectors; union { virDomainVirtioSerialOpts vioserial; virDomainPCIControllerOpts pciopts; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 193959f..5274d17 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -4236,12 +4236,23 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, virBuffer buf = VIR_BUFFER_INITIALIZER; int model; - if (def->queues && - !(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && + if (!(def->type == VIR_DOMAIN_CONTROLLER_TYPE_SCSI && def->model == VIR_DOMAIN_CONTROLLER_MODEL_SCSI_VIRTIO_SCSI)) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", - _("'queues' is only supported by virtio-scsi controller")); - return NULL; + if (def->queues) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'queues' is only supported by virtio-scsi controller")); + return NULL; + } + if (def->cmd_per_lun) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'cmd_per_lun' is only supported by virtio-scsi controller")); + return NULL; + } + if (def->max_sectors) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s", + _("'max_sectors' is only supported by virtio-scsi controller")); + return NULL; + } } switch (def->type) { @@ -4369,6 +4380,12 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, if (def->queues) virBufferAsprintf(&buf, ",num_queues=%u", def->queues); + if (def->cmd_per_lun) + virBufferAsprintf(&buf, ",cmd_per_lun=%u", def->cmd_per_lun); + + if (def->max_sectors) + virBufferAsprintf(&buf, ",max_sectors=%u", def->max_sectors); + if (qemuBuildDeviceAddressStr(&buf, domainDef, &def->info, qemuCaps) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args new file mode 100644 index 0000000..2c75790 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 8 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,cmd_per_lun=50,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ +-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml new file mode 100644 index 0000000..615a7f5 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver cmd_per_lun='50'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args new file mode 100644 index 0000000..895f379 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.args @@ -0,0 +1,9 @@ +LC_ALL=C PATH=/bin HOME=/home/test USER=test LOGNAME=test QEMU_AUDIO_DRV=none \ +/usr/bin/qemu -S -M pc -m 214 -smp 8 -nographic -nodefconfig -nodefaults \ +-monitor unix:/tmp/test-monitor,server,nowait -no-acpi -boot c \ +-device virtio-scsi-pci,id=scsi0,max_sectors=512,bus=pci.0,addr=0x3 \ +-usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-scsi0-0-0-0 \ +-device scsi-disk,bus=scsi0.0,channel=0,scsi-id=0,lun=0,\ +drive=drive-scsi0-0-0-0,id=scsi0-0-0-0 \ +-device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x4 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.xml b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.xml new file mode 100644 index 0000000..cd7d4a7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-disk-virtio-scsi-max_sectors.xml @@ -0,0 +1,29 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219136</memory> + <currentMemory unit='KiB'>219136</currentMemory> + <vcpu placement='static'>8</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='sdb' bus='scsi'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='scsi' index='0' model='virtio-scsi'> + <driver max_sectors='512'/> + </controller> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 1ea7bf8..8ebb202 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -807,6 +807,12 @@ mymain(void) DO_TEST("disk-virtio-scsi-num_queues", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-cmd_per_lun", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_VIRTIO_SCSI); + DO_TEST("disk-virtio-scsi-max_sectors", + QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, + QEMU_CAPS_VIRTIO_SCSI); DO_TEST("disk-scsi-megasas", QEMU_CAPS_DRIVE, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG, QEMU_CAPS_SCSI_MEGASAS); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index da528da..73c4ff1 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -223,6 +223,8 @@ mymain(void) DO_TEST("disk-scsi-vscsi"); DO_TEST("disk-scsi-virtio-scsi"); DO_TEST("disk-virtio-scsi-num_queues"); + DO_TEST("disk-virtio-scsi-cmd_per_lun"); + DO_TEST("disk-virtio-scsi-max_sectors"); DO_TEST("disk-scsi-megasas"); DO_TEST_FULL("disk-mirror", false, WHEN_ACTIVE); DO_TEST_FULL("disk-mirror", true, WHEN_INACTIVE); -- 1.9.1
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
The corresponding QEMU command line:
-device virtio-scsi-pci,id=scsi0,cmd_per_lun=50,max_sectors=512, bus=pci.0,addr=0x3
Signed-off-by: Mike Perez <thingee@gmail.com> --- docs/formatdomain.html.in | 29 +++++++++++++++++++--- docs/schemas/domaincommon.rng | 10 ++++++++ src/conf/domain_conf.c | 27 ++++++++++++++++++-- src/conf/domain_conf.h | 2 ++ src/qemu/qemu_command.c | 27 ++++++++++++++++---- .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.args | 9 +++++++ .../qemuxml2argv-disk-virtio-scsi-cmd_per_lun.xml | 29 ++++++++++++++++++++++ .../qemuxml2argv-disk-virtio-scsi-max_sectors.args | 9 +++++++ .../qemuxml2argv-disk-virtio-scsi-max_sectors.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 6 +++++ tests/qemuxml2xmltest.c | 2 ++
Good job covering docs and testsuite additions alongside the code changes.
+++ b/docs/schemas/domaincommon.rng @@ -1735,6 +1735,16 @@ <ref name="unsignedInt"/> </attribute> </optional> + <optional>
TAB damage.
@@ -15279,13 +15296,19 @@ virDomainControllerDefFormat(virBufferPtr buf, break; }
- if (def->queues || virDomainDeviceInfoIsSet(&def->info, flags) || - pcihole64) { + if (def->queues || def->cmd_per_lun || def->max_sectors || + virDomainDeviceInfoIsSet(&def->info, flags) || pcihole64) {
More TAB damage. Please make sure 'make syntax-check' passes.
@@ -4369,6 +4380,12 @@ qemuBuildControllerDevStr(virDomainDefPtr domainDef, if (def->queues) virBufferAsprintf(&buf, ",num_queues=%u", def->queues);
+ if (def->cmd_per_lun) + virBufferAsprintf(&buf, ",cmd_per_lun=%u", def->cmd_per_lun);
and again But once we settle on the naming, the patch looks pretty good. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 16:06 Thu 22 May , Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
Thanks Eric. I posted v2 patch up which fixes the tab issues and switches the option names from underscores to dashes. -- Mike Perez
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options. We could also use camelCase [1], or just roll a dice. Jan [1] http://www.redhat.com/archives/libvir-list/2013-April/msg01340.html
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options.
We could also use camelCase [1], or just roll a dice.
Well underscores are originally what I had in my first patch [1]. Eric what do you think? [1] - http://www.redhat.com/archives/libvir-list/2014-May/msg00798.html -- Mike Perez
On 06/28/2014 04:39 AM, Mike Perez wrote:
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko <jtomko@redhat.com> wrote:
On 05/23/2014 12:06 AM, Eric Blake wrote:
On 05/22/2014 12:22 PM, Mike Perez wrote:
This introduces two new attributes "cmd_per_lun" and "max_sectors" same with the names QEMU uses for virtio-scsi. An example of the XML:
<controller type='scsi' index='0' model='virtio-scsi' cmd_per_lun='50' max_sectors='512'/>
I'm not a fan of underscore (why type the shift key, when a dash will do). The libvirt xml name does not have to exactly match the qemu option name, so maybe there's some room for bikeshedding what names we should actually present to the user.
IMO using underscores here would be consistent with other disk-related options like read_iops_sec; dashes are mostly used for network-related options.
We could also use camelCase [1], or just roll a dice.
Well underscores are originally what I had in my first patch [1]. Eric what do you think?
[1] - http://www.redhat.com/archives/libvir-list/2014-May/msg00798.html
I have pushed the underscore version (with tabs and release numbers fixed) now that we're out of the freeze. Sorry it took so long. Jan
participants (3)
-
Eric Blake -
Ján Tomko -
Mike Perez