Re: [PATCH 1/3] conf: Reposition adding SCSI controller for SCSI hostdev hotplug

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

 



On 12/02/2015 04:08 PM, John Ferlan wrote:


On 12/02/2015 03:27 AM, Boris Fiuczynski wrote:
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.

It's all domain_conf code...

virDomainHostdevDefParseXML is either called by virDomainDefParseXML
(inactive) or virDomainDeviceDefParse (active, via various drivers).

The post parse processing code for driver callbacks is handled more
directly through virDomainDeviceDefPostParse at the end of
virDomainDeviceDefParse.

The post parse processing for inactive callbacks is handled through
virDomainDefPostParseDeviceIterator as part of validating the domain in
virDomainDefPostParse.

IMO it's splitting hairs to claim device processing is that much
different than domain processing. It's just that the hotplug code knows
how to directly go to the device it wants based on the xml from the
attach-device command rather than going through all domain processing.

In any case, the existing mechanism to assign an address and thus maybe
add a controller for the inactive domain was introduced by commit id
'cdb978955'. That also introduced the code used by the active/hotplug
processing. The "side effect" of the code not trying to add a controller
was that setting the controller value to something didn't exist (I
assume) knowing that CreateSCSIDiskController would do the dirty work
later (not documented either which is another "issue" in my mind).

Just for completeness... Auto-generating a controller for a hostdev was
introduced by commit id '881eb780'. The CreateSCSIDiskController code
has been around a lot longer...

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.


I think this may be a short term solution unless I can figure out a
better way short term to hotplug an added controller as well if one is
created.


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!


The "concept" of moving virDomainDefMaybeAddController into
virDomainHostdevAssignAddress was I believe meant to be an optimization
of sorts, but yes, it's turned into a bug or exposure of one of those
unwritten/undocumented code paths which "expected" to have some other
code path do the add.

As for a different way to fix the problem... When a controller is really
added into the domain it will gain an alias (which is what those error
messages are complaining about).

So, what if qemuDomainFindOrCreateSCSIDiskController checked the "found"
cont in the first loop for "cont->info.alias". If it doesn't exist, then
fall through under the "assumption" that the controller was added by
some other mechanism into the list?  That would require some more
adjustments in qemuDomainAttachControllerDevice to know the controller
was already in the list, but not active in the domain.

I thought about that as well and decided against it because you would add controllers in the domain data structure and than decide based on the aliases if the controller needs to be hotplugged afterwards. I thought that using the alias to detect is the domain is running in such way would not be proper! Anyhow what are you going to do if hotplugging the controller fails? Would this require an additional cleanup method? qemuDomainAttachControllerDevice is also used when adding a new scsi controller directly from virsh attach-device (scsi-controller.xml). What additional special casing would be required there...?


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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]