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

[Eric Farman]

On 12/13/2017 02:39 PM, John Ferlan wrote:
> 
> 
> 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. */
> 

Yeah, that helps considerably.

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

I never got that far.  :(  But with the above clarification, consider 
this patch

Reviewed-by: Eric Farman <farman at linux.vnet.ibm.com>

> 
>>
>> 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 is certainly interesting, though I get worried when defaults change 
since I don't understand the other options.  In my environment, only 
"auto" and "virtio-scsi" will boot as type=scsi controllers.  Everything 
else gives me some variation of an invalid configuration message.  But 
that doesn't mean that's the case for other setups.

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

Oh my guest configs ALWAYS have a controller tag.  Definitely.  I 
totally never forget to add one.  :-)

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

Agreed.  I can foresee host devices going to wrong controllers because 
of copy/paste errors, so trying to provide educated guesses in a 
scenario where the controller is auto-generated would be equally tough.

> 
> 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, I appreciate the insight!  I saw your vhost-scsi series this morning 
and thought I'd take that opportunity.  I didn't see this series until 
your ping, so opted to follow this trail for a while too.  Will pick up 
on other patches later methinks.

  - Eric

> 
> 
>>
>>>        }
>>>
>>>        /* 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;
>>>        }
>>>
>>
>