On Mon, Jul 03, 2017 at 05:25:30PM -0400, John Ferlan wrote:
Since virnodedeviceobj now has a self-lockable hash table, there's no need to lock the table from the driver for processing. Thus remove the locks from the driver for NodeDeviceObjList mgmt.
Signed-off-by: John Ferlan <jferlan@redhat.com>
[..]
@@ -601,8 +565,6 @@ nodeDeviceDestroy(virNodeDevicePtr device) return -1; def = virNodeDeviceObjGetDef(obj);
- nodeDeviceLock(); -
Consider the following scenario handling the same device: Thread 1 (udev event handler callback) | Thread 2 (nodeDeviceDestroy) =======================================|============================= | # attempt to destroy a device | obj = nodeDeviceObjFindByName() | | # @obj is locked | def = virNodeDeviceObjGetDef(obj) | | virNodeDeviceObjEndAPI(&obj) | # @obj is unlocked <------ # change event | udevAddOneDevice() | | obj = virNodeDeviceObjListFindByName | # @obj locked | new_device = false | # @obj unlocked | | obj = virNodeDeviceObjListAssignDef | # @obj locked | virNodeDeviceObjEndAPI(&obj) | # @obj unlocked and @def changed | ------> | virNodeDeviceObjListGetParentHost() | if (def->parent) ===> SIGSEGV In patch 12/14 this would have been a deadlock because you first locked the @obj, then nodedev driver while udevAddOneDevice did in the reverse order. I don't know about NPIV so I'm not sure whether there is another way of removing a device and updating the parent device tree, might require an updated model, but for now, you need to make a deep copy of @def. I can see that the chance of getting an 'change' event from udev on a vHBA device is low, but I'm trying to look at it from a higher perspective, as we want to be able to remove mdevs this way, possibly other devices in the future. The rest of the patch looks okay, but I want to try it first. Erik