[libvirt] [PATCH v4 14/14] nodedev: Remove driver locks around object list mgmt code
Erik Skultety
eskultet at redhat.com
Tue Jul 11 14:38:20 UTC 2017
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 at 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
More information about the libvir-list
mailing list