[libvirt] [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug
Boris Fiuczynski
fiuczy at linux.vnet.ibm.com
Wed Dec 2 08:27:13 UTC 2015
On 12/02/2015 01:16 AM, John Ferlan wrote:
>
>
> On 11/30/2015 06:05 AM, Boris Fiuczynski wrote:
>
> [...]
>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index cbfc41e..69cfd0f 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -16285,6 +16285,9 @@ virDomainDefParseXML(xmlDocPtr xml,
>> }
>>
>> def->hostdevs[def->nhostdevs++] = hostdev;
>> +
>> + if (virDomainDefMaybeAddHostdevSCSIcontroller(def) < 0)
>> + goto error;
>> }
>> VIR_FREE(nodes);
>>
>>
>
> After some more experimentation and making sure my debug environment was
> "up to date" (e.g. had virtlogd started)...
>
> Using top of tree I can hotplug a hostdev as long as it has a drive
> address defined. Not sure why it was failing before, but at least things
> are making more sense now. The reason why that path works with top of
> tree is because virDomainDeviceDefPostParseInternal won't call
> virDomainHostdevAssignAddress since hdev->info->type is not
> VIR_DOMAIN_DEVICE_ADDRESS_TYPE_NONE. That means when the
> qemuDomainFindOrCreateSCSIDiskController is called it will do the magic add.
>
> The path for this patch handles the inactive domain definition. While
> patch 2 handles the backend of the active domain path.
Patch 2 is also used in the "inactive domain definition" case.
The important difference is that the controller is NOT created on the
device definition path so that the controller is added via hotplug by
qemuDomainFindOrCreateSCSIDiskController since device hotplug uses the
device definition path! That is why this patch reintroduces adding the
controller into domain definition that the hotplug code does not use.
>
> When you have a running domain, then qemuDomainAttachDeviceFlags (e.g.
> what virsh attach-device would use) calls virDomainDeviceDefParse which
> is where the controller is added during virDomainDeviceDefPostParse. So
> patch 2 where you're removing that PostParse callback is why the hotplug
> will then work because no longer does post parse add the controller to
> the domain def.
Correct.
>
> The call to qemuDomainFindOrCreateSCSIDiskController called by
> qemuDomainAttachSCSIDisk during qemuDomainAttachDeviceDiskLive in the
> live path of qemuDomainAttachDeviceFlags would then automagically add
> the controller. Since the active path wouldn't have added it during post
> parse.
Correct.
>
> While these patches would work for the active domain hostdev add, I
> think perhaps a different mechanism should be used. Given the failure
> from trying to add a disk in the same environment, I'm not sure either
> patch solves the root problem that during hotplug we're relying on *not*
> adding a controller device when a new one is deemed necessary (for both
> hostdev and disk).
I think that the root problem was introduced by moving the adding of the
controller from the DOMAIN definition parsing into the DEVICE definition
parsing.
Since the code at this point seems by its multiple usage patterns rather
complex and fragile to changes I choose for this fix to revert the code
rather than to reengineer the code and not potentially make things worse.
>
> One thing that is of interest is that virDomainDefMaybeAddController has
> 3 return values -1 fail, 0 didn't add a controller, and 1 added a
> controller. I'm wondering if that can somehow be used to ensure that if
> we add a controller that we ensure it really gets plugged in. Instead of
> relying on qemuDomainFindOrCreateSCSIDiskController to backdoor it in.
I must say that calling vir > DomainDef < MaybeAddController from the
DEVICE parsing and not from the DOMAIN parsing feels rather strange to
me. In both cases domain parsing and hotplugging devices the controllers
are plugged in AFTER the device parsing is finished! I do not see any
backdoor on the hotplug path!
>
> John
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Martina Köderitz
Geschäftsführung: Dirk Wittkopp
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
More information about the libvir-list
mailing list