Introduce XML to allowing adding iothreads to the domain. These can be used by virtio-blk-pci devices in order to assign a specific thread to handle the workload for the device. The iothreads are the official implementation of the virtio-blk Data Plane that's been in tech preview for QEMU. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- docs/formatdomain.html.in | 26 ++++++++++++++++++++++++++ docs/schemas/domaincommon.rng | 12 ++++++++++++ src/conf/domain_conf.c | 28 ++++++++++++++++++++++++++++ src/conf/domain_conf.h | 2 ++ 4 files changed, 68 insertions(+) diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in index 59127bb..0fe10f4 100644 --- a/docs/formatdomain.html.in +++ b/docs/formatdomain.html.in @@ -470,6 +470,32 @@ </dd> </dl> + <h3><a name="elementsIOThreadsAllocation">IOThreads Allocation</a></h3> + <p> + IOThreads are a QEMU feature that will allow supported disk + devices to be configured to use a dedicated event loop thread + to handle block I/O requests. + <span class="since">Since 1.2.8 (QEMU only)</span> + </p> + +<pre> +&lt;domain&gt; + ... + &lt;iothreads&gt;4&lt;/iothreads&gt; + ... +&lt;/domain&gt; +</pre> + + <dl> + <dt><code>iothreads</code></dt> + <dd> + The content of this optional element defines the number + of IOThreads to be assigned to the domain for use by + virtio-blk-pci target storage devices. There should be + only 1 or 2 IOThreads per host CPU and 1 IOThread per + supported device. + </dd> + </dl> <h3><a name="elementsCPUTuning">CPU Tuning</a></h3> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 9a89dd8..b4ac483 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -632,6 +632,12 @@ </optional> <optional> + <element name="iothreads"> + <ref name="countIOThreads"/> + </element> + </optional> + + <optional> <ref name="blkiotune"/> </optional> @@ -4747,6 +4753,12 @@ <param name="minInclusive">1</param> </data> </define> + <define name="countIOThreads"> + <data type="unsignedInt"> + <param name="pattern">[0-9]+</param> + <param name="minInclusive">0</param> + </data> + </define> <define name="vcpuid"> <data type="unsignedShort"> <param name="pattern">[0-9]+</param> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 22a7f7e..671c41c 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -11953,6 +11953,23 @@ virDomainDefParseXML(xmlDocPtr xml, } } + /* Optional - iothreads */ + n = virXPathULong("string(./iothreads[1])", ctxt, &count); + if (n == -2) { + virReportError(VIR_ERR_XML_ERROR, "%s", + _("iothreads count must be an integer")); + goto error; + } else if (n < 0) { + def->iothreads = 0; + } else { + if ((unsigned int) count != count) { + virReportError(VIR_ERR_XML_ERROR, + _("invalid iothreads count '%lu'"), count); + goto error; + } + def->iothreads = count; + } + /* Extract cpu tunables. */ if ((n = virXPathULong("string(./cputune/shares[1])", ctxt, &def->cputune.shares)) < -1) { @@ -14461,6 +14478,14 @@ virDomainDefCheckABIStability(virDomainDefPtr src, goto error; } + if (src->iothreads != dst->iothreads) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Target domain iothreads count %u does not " + "match source %u"), + dst->iothreads, src->iothreads); + goto error; + } + if (STRNEQ(src->os.type, dst->os.type)) { virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Target domain OS type %s does not match source %s"), @@ -17874,6 +17899,9 @@ virDomainDefFormatInternal(virDomainDefPtr def, virBufferAsprintf(buf, " current='%u'", def->vcpus); virBufferAsprintf(buf, ">%u</vcpu>\n", def->maxvcpus); + if (def->iothreads > 0) + virBufferAsprintf(buf, "<iothreads>%u</iothreads>\n", def->iothreads); + if (def->cputune.sharesSpecified || (def->cputune.nvcpupin && !virDomainIsAllVcpupinInherited(def)) || def->cputune.period || def->cputune.quota || diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 36ccf10..705ce32 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1908,6 +1908,8 @@ struct _virDomainDef { int placement_mode; virBitmapPtr cpumask; + unsigned int iothreads; + struct { unsigned long shares; bool sharesSpecified; -- 1.9.3

On 08/26/2014 01:03 AM, Martin Kletzander wrote: I (more or less) modeled (eg, cut, copy, paste) after the vcpu element which as you'll note is "countCPU" for both current and maxvcpus. Unlike the vcpu, I went with an 'int' instead of a 'short' although technically I cannot imagine more than a short number of threads or disks assigned to any domain! Also unlike the vcpu argument a "0" is a perfectly valid value. See my above explanation and then look about 10-20 lines before iothreads and see vcpu/current and maxvcpus. The power of the visual suggestion of what code around area I was changing was doing was far greater than thinking hmm.. should this code use the virStrToLong calls. Unlike of course when I added the iothread to the disk property which does use the function. Maybe in a different patch it'd be worth working through the various other calls to virXPathU* and seeing if they too could use the virStrToLong* interfaces. I'll adjust both - no problem. John

Add a new capability to ensure the iothreads feature exists for the qemu emulator being run - requires the "query-iothreads" QMP command. Using the domain XML add correspoding command argument in order to generate the threads. The iothreads will use a name space "libvirtIothread#" where, the future patch to add support for using an iothread to a disk definition to merely define which of the available threads to use. Add tests to ensure the xml/argv processing is correct. Note that no change was made to qemuargv2xmltest.c as processing the -object element would require knowing more than just iothreads. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_capabilities.c | 2 ++ src/qemu/qemu_capabilities.h | 1 + src/qemu/qemu_command.c | 13 ++++++++++ tests/qemuxml2argvdata/qemuxml2argv-iothreads.args | 8 ++++++ tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml | 29 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 56 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 410086b..b331be7 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -268,6 +268,7 @@ VIR_ENUM_IMPL(virQEMUCaps, QEMU_CAPS_LAST, "rtc-reset-reinjection", "splash-timeout", /* 175 */ + "query-iothreads", ); @@ -1430,6 +1431,7 @@ struct virQEMUCapsStringFlags virQEMUCapsCommands[] = { { "nbd-server-start", QEMU_CAPS_NBD_SERVER }, { "change-backing-file", QEMU_CAPS_CHANGE_BACKING_FILE }, { "rtc-reset-reinjection", QEMU_CAPS_RTC_RESET_REINJECTION }, + { "query-iothreads", QEMU_CAPS_OBJECT_IOTHREAD}, }; struct virQEMUCapsStringFlags virQEMUCapsEvents[] = { diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h index 48a4eaa..0980c00 100644 --- a/src/qemu/qemu_capabilities.h +++ b/src/qemu/qemu_capabilities.h @@ -215,6 +215,7 @@ typedef enum { QEMU_CAPS_OBJECT_USB_AUDIO = 173, /* usb-audio device support */ QEMU_CAPS_RTC_RESET_REINJECTION = 174, /* rtc-reset-reinjection monitor command */ QEMU_CAPS_SPLASH_TIMEOUT = 175, /* -boot splash-time */ + QEMU_CAPS_OBJECT_IOTHREAD = 176, /* -object iothread */ QEMU_CAPS_LAST, /* this must always be the last item */ } virQEMUCapsFlags; diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 6dac9d3..287a3f3 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7510,6 +7510,19 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, smp); VIR_FREE(smp); + if (def->iothreads > 0 && + virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD)) { + /* Create named iothread objects starting with 1. These may be used + * by a disk definition which will associate to an iothread by + * supplying a value of 1 up to the number of iothreads available + * (since 0 would indicate to not use the feature). + */ + for (i = 1; i <= def->iothreads; i++) { + virCommandAddArg(cmd, "-object"); + virCommandAddArgFormat(cmd, "iothread,id=libvirtIothread%zu", i); + } + } + if (def->cpu && def->cpu->ncells) if (qemuBuildNumaArgStr(cfg, def, cmd, qemuCaps) < 0) goto error; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads.args new file mode 100644 index 0000000..ca0c174 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads.args @@ -0,0 +1,8 @@ +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 2 \ +-object iothread,id=libvirtIothread1 \ +-object iothread,id=libvirtIothread2 \ +-nographic -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb -hda \ +/dev/HostVG/QEMUGuest1 -net none -serial none -parallel none diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads.xml new file mode 100644 index 0000000..12a92e7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads.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'>2</vcpu> + <iothreads>2</iothreads> + <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'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 7407272..0e3f60a 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1182,6 +1182,8 @@ mymain(void) DO_TEST("smp", QEMU_CAPS_SMP_TOPOLOGY); + DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST("cpu-topology1", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("cpu-topology2", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("cpu-topology3", NONE); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 79cf59f..47314e3 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -298,6 +298,7 @@ mymain(void) DO_TEST("cputune-zero-shares"); DO_TEST("smp"); + DO_TEST("iothreads"); DO_TEST("lease"); DO_TEST("event_idx"); DO_TEST("vhost_queues"); -- 1.9.3

On 08/26/2014 01:03 AM, Martin Kletzander wrote: hmm... so how is this different then say perhaps QEMU_CAPS_OBJECT_RNG_RANDOM, QEMU_CAPS_OBJECT_MEMORY_RAM or QEMU_CAPS_OBJECT_MEMORY_FILE which each eventually uses "-object" to build it's part of the command line Hmm.. I wasn't completely sure where to put this - it's not overly clear which of the various capabilities structures should be used for what reason. Although I guess since the above named flags are in ObjectTypes I suppose I should have used it. I'll move it. Using 'libvirt' as the prefix for "iothread" was more of a convention to make sure it was clear what created it. I can go with the shorter name and initially had done so. John

On Mon, Aug 25, 2014 at 08:38:08PM -0400, John Ferlan wrote: Again, what's the difference to just ref name="unsignedInt"? [...] virBitmapFree(def->iothreadmap) is missing in virDomainDefFree(). Martin

On 08/26/2014 02:38 AM, John Ferlan wrote: Do you plan to do multiple disks on a single iothread in the followup patches? Or is that a configuration that doesn't make sense? Jan

On 08/26/2014 06:42 PM, John Ferlan wrote: I was wondering what's up with all the private comments. Hopefully I can share the bit you're quoting: There are basically two ways to use IOThreads: 1. 1 or maybe 2 IOThreads per host CPU 2. 1 IOThread per device For usage 1), can you just add some <iothreads> to the domain XML and they will automatically be used, or do you need to 'bind' the specific disk devices to them? Jan

For virtio-blk-pci disks with the disk iothread attribute that are running the correct emulator, add the "iothread=libvirtIothread#" to the -device command line in order to enable iothreads for the disk. This code will check both the "start" and "hotplug" paths for the capability, whether the iothreadsmap has been defined, and whether there's an available iothread to be used for the disk. Signed-off-by: John Ferlan <jferlan(a)redhat.com&gt; --- src/qemu/qemu_command.c | 35 +++++++++++++++++++ src/qemu/qemu_command.h | 5 +++ src/qemu/qemu_hotplug.c | 11 ++++++ .../qemuxml2argv-iothreads-disk.args | 17 +++++++++ .../qemuxml2argv-iothreads-disk.xml | 40 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 2 ++ tests/qemuxml2xmltest.c | 1 + 7 files changed, 111 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 287a3f3..740b6ec 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -3862,6 +3862,9 @@ qemuBuildDriveDevStr(virDomainDefPtr def, virBufferAddLit(&opt, "virtio-blk-device"); } else { virBufferAddLit(&opt, "virtio-blk-pci"); + if (disk->iothread > 0) + virBufferAsprintf(&opt, ",iothread=libvirtIothread%u", + disk->iothread); } qemuBuildIoEventFdStr(&opt, disk->ioeventfd, qemuCaps); if (disk->event_idx && @@ -6614,6 +6617,31 @@ qemuBuildNumaArgStr(virQEMUDriverConfigPtr cfg, } +bool +qemuDomainIothreadsAvailable(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainDiskDefPtr disk) +{ + bool inuse; + const char *src = virDomainDiskGetSource(disk); + + if (!virQEMUCapsGet(qemuCaps, QEMU_CAPS_OBJECT_IOTHREAD) || + !def->iothreadmap) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOThreads not supported for this QEMU " + "for disk '%s'"), src); + return false; + } + if (virBitmapGetBit(def->iothreadmap, disk->iothread, &inuse) || inuse) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("IOThread '%u' for disk '%s' is already " + "being used"), disk->iothread, src); + return false; + } + return true; +} + + static int qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, virCommandPtr cmd, @@ -8055,6 +8083,13 @@ qemuBuildCommandLine(virConnectPtr conn, disk->src->driverName, disk->src->path); goto error; } + + /* Check iothreads relationship */ + if (disk->iothread > 0) { + if (!qemuDomainIothreadsAvailable(def, qemuCaps, disk)) + goto error; + ignore_value(virBitmapSetBit(def->iothreadmap, disk->iothread)); + } } if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE)) { diff --git a/src/qemu/qemu_command.h b/src/qemu/qemu_command.h index 633ff71..bec6c14 100644 --- a/src/qemu/qemu_command.h +++ b/src/qemu/qemu_command.h @@ -81,6 +81,11 @@ virCommandPtr qemuBuildCommandLine(virConnectPtr conn, bool forXMLToArgv) ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(11); +bool +qemuDomainIothreadsAvailable(virDomainDefPtr def, + virQEMUCapsPtr qemuCaps, + virDomainDiskDefPtr disk); + /* Generate '-device' string for chardev device */ int qemuBuildChrDeviceStr(char **deviceStr, diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c index a364c52..720220d 100644 --- a/src/qemu/qemu_hotplug.c +++ b/src/qemu/qemu_hotplug.c @@ -335,6 +335,11 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, } } + if (disk->iothread > 0) { + if (!qemuDomainIothreadsAvailable(vm->def, priv->qemuCaps, disk)) + goto cleanup; + } + if (qemuDomainPrepareDisk(driver, vm, disk, NULL, false) < 0) goto cleanup; @@ -396,6 +401,9 @@ qemuDomainAttachVirtioDiskDevice(virConnectPtr conn, if (ret < 0) goto error; + if (disk->iothread && vm->def->iothreadmap) + ignore_value(virBitmapSetBit(vm->def->iothreadmap, disk->iothread)); + virDomainDiskInsertPreAlloced(vm->def, disk); cleanup: @@ -2539,6 +2547,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver, } } + if (disk->iothread && vm->def->iothreadmap) + ignore_value(virBitmapClearBit(vm->def->iothreadmap, disk->iothread)); + qemuDomainReleaseDeviceAddress(vm, &disk->info, src); if (virSecurityManagerRestoreDiskLabel(driver->securityManager, diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args new file mode 100644 index 0000000..693cb4c --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.args @@ -0,0 +1,17 @@ +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 2 \ +-object iothread,id=libvirtIothread1 \ +-object iothread,id=libvirtIothread2 \ +-nographic -nodefaults -monitor \ +unix:/tmp/test-monitor,server,nowait -no-acpi -boot c -usb \ +-drive file=/dev/HostVG/QEMUGuest1,if=none,id=drive-ide0-0-0 \ +-device ide-drive,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0 \ +-drive file=/var/lib/libvirt/images/iothrtest1.img,if=none,\ +id=drive-virtio-disk1 \ +-device virtio-blk-pci,iothread=libvirtIothread1,bus=pci.0,addr=0x4,\ +drive=drive-virtio-disk1,id=virtio-disk1 \ +-drive file=/var/lib/libvirt/images/iothrtest2.img,if=none,\ +id=drive-virtio-disk2 \ +-device virtio-blk-pci,iothread=libvirtIothread2,bus=pci.0,addr=0x3,\ +drive=drive-virtio-disk2,id=virtio-disk2 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml new file mode 100644 index 0000000..72122fb --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-iothreads-disk.xml @@ -0,0 +1,40 @@ +<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'>2</vcpu> + <iothreads>2</iothreads> + <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'> + <driver name='qemu' type='raw'/> + <source dev='/dev/HostVG/QEMUGuest1'/> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' iothread='1'/> + <source file='/var/lib/libvirt/images/iothrtest1.img'/> + <target dev='vdb' bus='virtio'/> + <address type='pci' domain='0x0000' bus='0x00' slot='0x04' function='0x0'/> + </disk> + <disk type='file' device='disk'> + <driver name='qemu' type='raw' iothread='2'/> + <source file='/var/lib/libvirt/images/iothrtest2.img'/> + <target dev='vdc' bus='virtio'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='none'/> + </devices> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index 0e3f60a..0bb282d 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1183,6 +1183,8 @@ mymain(void) DO_TEST("smp", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("iothreads", QEMU_CAPS_OBJECT_IOTHREAD); + DO_TEST("iothreads-disk", QEMU_CAPS_OBJECT_IOTHREAD, QEMU_CAPS_DEVICE, + QEMU_CAPS_DRIVE); DO_TEST("cpu-topology1", QEMU_CAPS_SMP_TOPOLOGY); DO_TEST("cpu-topology2", QEMU_CAPS_SMP_TOPOLOGY); diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index 47314e3..bc345c9 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -299,6 +299,7 @@ mymain(void) DO_TEST("smp"); DO_TEST("iothreads"); + DO_TEST("iothreads-disk"); DO_TEST("lease"); DO_TEST("event_idx"); DO_TEST("vhost_queues"); -- 1.9.3

On 08/26/2014 01:03 AM, Martin Kletzander wrote: OK - fair enough - although I suspect a cdrom wasn't an expected device for an iothread... My thinking was since/if we do have src available it's "nicer" to let the user know which disk/src is failing - although yes, finding multiple "iothread='#'" is fairly simple too... I'll look into your ideas here after the caffeine has coursed through my veins for a little while longer. I wasn't quite sure the "best" place to make the checks. It had to be after all the parsing was done, but before the command was generated - the domain [device] post parse checking code wasn't on my radar of places to look... I'm also not sure an iothread can support (at this time) "all" the various disk types/formats, but figured at the very least I had to start 'somewhere'. The problem with using a "definitive" list is adding support for 'newer' types in the future requires multiple changes; however, being more relaxed with what is allowed could cause unforeseen problems when the vm is started. Yes, it's in a loop checking all ndisks for the initial start, but would only check the disk if the iothread was set for the disk - which in my mind is a deliberate action. For the hotplug virtio case - if the map isn't there, but iothread was "somehow" set that (to me) meant someone was trying to add an iothread to the disk, but none existed or were available, thus cause failure. But I'm guessing perhaps the post parse will be a better place - so again - I'll look into using it. Right support is supposed to be for "virtio-blk-pci" devices only... Right - since bit 0 is meaningless anyway for this purpose I suppose it's a tossup whether clearing for "every" disk is any more/less of a "performance penalty" than making the check for available and then doing the clear. Not sure I quite understand what you're driving at. This disk uses bus 'ide' which isn't a supported option. Sure I could "change" it, but then what does DO_TEST_DIFFERENT provide? Thanks for the quick review... John

On 08/26/2014 01:03 AM, Martin Kletzander wrote: I looked at the above code simplification idea and I wasn't able to see what you were driving at. Maybe it's my current code myopia... There's a ditty about not being able to see the forest through the trees that may apply for me. Maybe I'm missing something obvious - I dunno... Beyond the obvious of if any other code passes a NULL Bitmap, the existing code will fail miserably... Modifying GetBit/SetBit/ClearBit in order to handle a NULL iothreadmap doesn't help the case where iothread == 0 which I think could be the more common case. I've adjusted where the checks are made to within the BuildDriveDevStr code. That way both the hotplug and regular startup will make the same check. I'm not sure using a PostParse will work properly, especially in the hotplug case where the disk xml is not added to the configuration yet - the live add is attempted first and if it succeeds, then the bitmap would need to be updated. If the configuration is to be saved, then the on-disk config is updated. I assume that for hotplug the domain def passed along is the "live" one that already has the map filled in for any disks that were properly vetted at startup time. Without the "live" map being updated, one could add 'n' live disks to the same IOThread and that has seemingly nothing to do with PostParse - although I may have misfollowed things. The other part of PostParse that's difficult is that the DevicePostParse would probably be the place to check whether the device itself is valid because while 'virtio' bus could be set, when does 'pci' get set? I also note qemuDomainDefPostParse() uses virCapsPtr and not virQEMUCapsPtr and it's seemingly tasked to primarily add things that will be needed by the guest due to "other" found devices in the guest XML and not to peruse through the ndisks looking to fill in a bitmap or check for all the right configuration setting options for iothreads. Just wasn't clear enough whether it "should be" used for this purpose. I'm not against using it - just wasn't completely sure. So v2 is coming to a mailbox near you soon... John