Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
On 06/28/2014 04:39 AM, Mike Perez wrote: On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko jto...@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 signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
On Thu, Jun 5, 2014 at 6:57 AM, Ján Tomko jto...@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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
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 signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 thin...@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 codedriver/code can specify the driver - specific options. Currently it only supports attribute codequeues/code - (span class=since1.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 + dtcodequeues/code/dt + dd +The optional codequeues/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=sinceSince 1.0.5 (QEMU and KVM only)/span + /dd + dtcodecmd_per_lun/code/dt + dd +The optional codecmd_per_lun/code attribute specifies the maximum +number of commands that can be queued on devices controlled by the +host. +span class=sinceSince 1.2.5 (QEMU and KVM only)/span + /dd + dtcodemax_sectors/code/dt + dd +The optional codemax_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=sinceSince 1.2.5 (QEMU and KVM only)/span + /dd +/dl p USB companion controllers have an optional sub-element codelt;mastergt;/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, +
Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
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 thin...@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 signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemu: Add cmd_per_lun, max_sectors to virtio-scsi
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 -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list