Re: [PATCH v4 14/14] nodedev: Remove driver locks around object list mgmt code

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

 



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@xxxxxxxxxx>

[..]

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

--
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]
  Powered by Linux