[libvirt] [PATCH 4/4] qemu: Allow use of iothreads for disk definitions
Martin Kletzander
mkletzan at redhat.com
Tue Aug 26 05:03:53 UTC 2014
On Mon, Aug 25, 2014 at 08:38:09PM -0400, John Ferlan wrote:
>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 at redhat.com>
>---
> 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);
You are using the def->iothread only to format it with virtio disks,
but ... [1]
> }
> 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);
Some disks may not have source set (empty cdrom drive), so it should
probably be NULLSTR(src), but I think ""IOThread '%u' used multiple
times" is more than enough.
This leads me to a code simplification idea. If you checked
QEMU_CAPS_OBJECT_IOTHREAD when creating the iothread objects it's
enough to make virBitmapGetBit() handle NULL bitmap carefully and then
just virBitmapGetBit() the bit the same way. And if the message does
not output the disk name (src), this whole function can be thrown away
and instead of doing:
if (!qemuDomainIothreadsAvailable(def, qemuCaps, disk))
goto error;
you'd do:
if (virBitmapGetBit(def->iothreadmap, disk->iothread, &tmp) || tmp)
virReportError(...);
Just an idea, though, not a requirement ;) If you keep it in a
separate function, though, I'd suggest checking the range and
reporting specific error, because when testing I got for example this
error:
"IOThread '3' for disk '(null)' is already being used"
even though it clearly wasn't; I just had only 1 or 2 I/O threads
created.
>+ 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));
>+ }
> }
>
[1] ... here you check it for all disks. And it's kept in the domain
definition for all disks as well. Maybe removing it from unsupported
disks and checking(+building) the iothreadmap could be done in
qemuDomainDefPostParse().
> 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:
From the names of the functions I see in these two hunks I guess this
is ignored for non-virtio disks, which is a difference to
cold-plugging non-virtio disks. The PostParse() modification I
suggest above would take care of that.
>@@ -2539,6 +2547,9 @@ qemuDomainRemoveDiskDevice(virQEMUDriverPtr driver,
> }
> }
>
>+ if (disk->iothread && vm->def->iothreadmap)
>+ ignore_value(virBitmapClearBit(vm->def->iothreadmap, disk->iothread));
>+
Making virBitmapClearBit() handle NULL bitmaps would clean this up to
be unconditional (even without the check for disk->iothread) ;)
> qemuDomainReleaseDeviceAddress(vm, &disk->info, src);
>
> if (virSecurityManagerRestoreDiskLabel(driver->securityManager,
[...]
>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'/>
Adding the iothread='x' in here and making a copy without it in
tests/qemuxml2xmloutdata/ you could do ... [2]
>+ <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");
[2] ... DO_TEST_DIFFERENT() in here to test what I meant up there.
Martin
P.S.: Other than that it looks fine and I'm looking forward to v2!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20140826/cce44cdd/attachment-0001.sig>
More information about the libvir-list
mailing list