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

Martin Kletzander mkletzan at redhat.com
Wed Aug 27 03:48:52 UTC 2014


On Tue, Aug 26, 2014 at 06:03:46PM -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:
>>> 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(...);
>>
>
>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.
>

Oh, yes, I missed a case, but anyway, nothing is wrong with reporting
it when building the command-line.  Looking back at my ideas, it seems
like a copy-paste from insomnia-driven programming book.

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

Great, that way it's still in building a command-line, but applies for
hotplug too.

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

Yes, PostParse shouldn't be doing that, the iothreadmap is easier to
be created when starting the machine.  Then it's clear that live
machines have iothreadmap and persistent definitions don't.  I was
just curious whether it would make sense to error out earlier (when
defining the XML) rather then later (when starting the machine).
Unfortunately I covered that simple piece of information in a crusty
hardly readable shell.

[...]
>So v2 is coming to a mailbox near you soon...
>

Looking forward!

Martin
-------------- 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/20140827/05aecc0b/attachment-0001.sig>


More information about the libvir-list mailing list