[libvirt] [PATCH 2/4] qemu: Use same model when adding hostdev SCSI controller

[John Ferlan]

On 12/13/2017 10:43 AM, Eric Farman wrote:
> 
> 
> On 12/06/2017 08:08 AM, John Ferlan wrote:
>> When qemuDomainFindOrCreateSCSIDiskController adds a controller,
>> let's use the same model as a currently found controller under the
>> assumption that the reason to add the controller in hotplug is
>> because virDomainHostdevAssignAddress determined that there were
>> too many devices on the existing controller, but only assigned a
>> new controller index and did not add a new controller and we
>> desire to use the same controller model as any existing conroller
> 
> s/conroller/controller
> 
>> and not take a chance that qemuDomainSetSCSIControllerModel would
>> use a default that may be incompatible.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>>   src/qemu/qemu_hotplug.c | 14 ++++++++++----
>>   1 file changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/qemu/qemu_hotplug.c b/src/qemu/qemu_hotplug.c
>> index 9317e134a..90d50e7b1 100644
>> --- a/src/qemu/qemu_hotplug.c
>> +++ b/src/qemu/qemu_hotplug.c
>> @@ -587,6 +587,7 @@
>> qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
>>   {
>>       size_t i;
>>       virDomainControllerDefPtr cont;
>> +    virDomainControllerModelSCSI model = -1;
>>
>>       for (i = 0; i < vm->def->ncontrollers; i++) {
>>           cont = vm->def->controllers[i];
>> @@ -596,6 +597,12 @@
>> qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
>>
>>           if (cont->idx == controller)
>>               return cont;
>> +
>> +        /* Save off the model - if we end up creating a controller it's
>> +         * because the user didn't provide one and we need to
>> automagically
>> +         * create one because the existing one is full - so let's be
>> sure
>> +         * to keep the same model in that case. */
>> +        model = cont->model;
> 
> Hrm...  I'm missing something...
> 
> This is confusing, because nothing in this loop tells us the controller
> we find is full, just that we have a TYPE_SCSI controller and the index
> isn't found.  And if the index WAS found, we would've exited before this
> line anyway so we don't know if it was full or not.

Does this explanation help:

    /* Because virDomainHostdevAssignAddress called during
     * virDomainHostdevDefPostParse cannot add a new controller
     * it will assign a controller index to a controller that doesn't
     * exist leaving this code to perform the magic of adding the
     * controller. Because that code would be attempting to add a
     * SCSI disk to an existing controller, let's save off the model
     * of the "last" SCSI controller we find so that if we end up
     * creating a controller below it uses the same controller model. */

The magic key is in patch 4 where it's noted that for
virDomainHostdevAssignAddress we need to assign a controller index to an
<address> element where the controller itself doesn't yet exist.

> 
> Nothing in a <hostdev> tag says what type of controller we want, so what
> happens when controller[1] is virtio-scsi, and controller[2] is LSI?
> This will create a second LSI controller which wouldn't help if we're
> hotplugging another virtio-scsi device and controller[1] was full.  (Is
> mixing SCSI controller types common?  I don't know, I stay in a little
> virtio-scsi playpen.)

Yes, true, and that I think ends up being the perhaps unavoidable tragic
flaw because after all how do you really know which model to use? I'm
not opposed to adding that truth to the comment, but wording it could be
a bit tricky...

Ironically I do have patches that would "prefer" virtio-scsi for hostdev
paths that I considered adding to this series, but wasn't sure it'd be
accepted. Something that I'm reconsidering now that I've read Michal's
patch on qemu-devel:

http://lists.nongnu.org/archive/html/qemu-devel/2017-12/msg01125.html

I also have another patch that would prefer virtio-scsi as the default
for new SCSI controllers by swapping the QEMU_CAPS_VIRTIO_SCSI and
QEMU_CAPS_SCSI_LSI if logic in qemuDomainSetSCSIControllerModel in the }
else { condition.  That way we'd choose virtio-scsi over lsi if it
exists (at least for non pseries).

That conceivably would make this patch unnecessary other than this patch
would force the model value for the controller to be filled in rather
than it being empty (e.g. -1) and then allowing the default code to take
over. That's a different can of worms, but the answer to that is in one
of your follow-up questions.

Of course for the consumer that's being smart ;-) they would create
enough virtio-scsi controllers for the number of SCSI LUN's they are
adding and none of this matters.

> 
> And I'm still trying to figure out how qemuDomainSetSCSIControllerModel
> plays into this and other codepaths.
> 

It's awful...  If a controller model isn't provided, then we need to
determine some value so that we create the correct command line. The
code doesn't really change or set the default in the domain XML, just in
the temporary @model variable used to generate the command line.

Ironically I have another series posted regarding moving controller
validation out of command line processing and into domain def validation
processing. One of the patches there, Jan I believe asked why not set
the model in post parse processing. Quite honestly I'm not sure, but I'm
making a supposition based on a few years working on the project and
that historically changing defaults or attempting to set some sort of
policy has been a bit of an uphill battle. So for this series, I just
took the path of least resistance.

Now, I'm not opposed to setting the model in PostParse processing as
long as we set it "smartly", but how do you do that without knowledge of
the consumers intentions (or perhaps lack of knowledge).

If we set during post parse processing, then we've set model policy
going forward for the future for the domain since we'll save the value
in the permanent/config file. This is opposed to perhaps allowing the
future be able to "choose" the default that is the "most recent" and
perhaps a better selection. Once we set/save it though - we're stuck
with it.  I'm not going to make *that* decision alone!

> Sorry, just sending my questions because this code is more fresh in your
> mind than my own.
> 

Good thing you asked this close to working on the code; my short term
memory seems to fade faster each day ;-)

And sorry for the long winded response - it's a be careful for what you ask!


> 
>>       }
>>
>>       /* No SCSI controller present, for backward compatibility we
>> @@ -604,11 +611,10 @@
>> qemuDomainFindOrCreateSCSIDiskController(virQEMUDriverPtr driver,
>>           return NULL;
>>       cont->type = VIR_DOMAIN_CONTROLLER_TYPE_SCSI;
>>       cont->idx = controller;
>> -    cont->model = -1;
>> +    cont->model = model;
>>
>> -    VIR_INFO("No SCSI controller present, hotplugging one");
>> -    if (qemuDomainAttachControllerDevice(driver,
>> -                                         vm, cont) < 0) {
>> +    VIR_INFO("No SCSI controller present, hotplugging one model=%d",
>> model);
> 
> Seems like a good use for
> virDomainControllerModelSCSITypeToString(model) perhaps?
> 

That could be done too...

Tks -

John

>> +    if (qemuDomainAttachControllerDevice(driver, vm, cont) < 0) {
>>           VIR_FREE(cont);
>>           return NULL;
>>       }
>>
>