[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 4/4] qemu: Allow use of iothreads for disk definitions



On Tue, Aug 26, 2014 at 07:34:16AM -0400, John Ferlan wrote:
On 08/26/2014 01:03 AM, Martin Kletzander wrote:
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 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.

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...


As I said, NULLSTR(src) is fine, too if you want, that wasn't some
strict requirement to get rid of that.


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.

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.


It's definitely better to allow it for one disk type where it does
work, is allowed, etc. and disable for everything else as there is no
problem with relaxing the condition in future.  However, making it
more strict would not be very nice to users ;)



+        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().


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.


Yes, you're right.  I wasn't saying it's bad, it was more of a
question whether it was intended :)

But I'm guessing perhaps the post parse will be a better place - so
again - I'll look into using it.

    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.


Right support is supposed to be for "virtio-blk-pci" devices only...

@@ -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) ;)


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.

    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]


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?


Wow, I'm reading my comment third time and I kinda don't understand
what I meant either :-D  Sorry for that, but I guess I wanted to show
either how it didn't work with target bud='ide' or the error I had.
Not sure though :)

Martin

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]