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

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



On Fri, Jul 14, 2017 at 11:37:36AM -0400, John Ferlan wrote:
>
>
> On 07/14/2017 05:23 AM, Erik Skultety wrote:
> > On Thu, Jul 13, 2017 at 07:23:58PM -0400, John Ferlan wrote:
> >>
> >>
> >> On 07/11/2017 10:38 AM, Erik Skultety wrote:
> >>> 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:
> >>>
> >>
> >> What if I told you that's impossible?  You cannot "have" a scsi_hostN,
> >> delete a scsi_hostN, and then have a new one created with the same name.
> >
> > Except I wasn't considering creation, rather than plain change. Although I
> > didn't manage to find out under what circumstances would kernel actually emit
> > the 'change' event (I tried to write to various writable attributes - but since
> > it either lacks documentation completely or it's just well hidden from me - I
> > wasn't able to trigger it), other than explicitly triggering it by writing to
> > sysfs' uevent - so in that aspect, as I wrote below in my previous response,
> > it's highly unlikely but not impossible to hit the race.
> >
>
> Not quite sure what would trigger a change event. A vHBA has a wwnn/wwpn
> and a parent wwnn/wwpn and WWN. If any of those change, the vHBA would
> need to be deleted and recreated.
>
> I do have a faint recollection of considering the ramifications of
> dropping the obj lock in that path and the race drawback, but I
> dismissed it mainly because of "how" vHBA's are created and what could
> constitute a change event for a vHBA essentially redefines it.
>
> >>
> >> The scsi_hostN's are an ever increasing value of N. Once created and
> >> deleted, the N will not be reused.
> >>
> >>> 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.
> >>
> >> I think what happens is code from virNodeDeviceGetWWNs through
> >> virVHBAManageVport gets placed into a function that handles vHBA's on
> >> deletion.  Similarly for CreateXML, vHBA specific code ends up in a
> >> helper function. Those helpers would be called based on the type of
> >> object/device we're talking about (vHBA/mdev).
> >
> > Despite the likelihood of the case I'm describing, the main point I'm trying to
> > make is that the lock protects a mutable resource (@def) and by releasing it
> > followed by querying it without actually holding the lock violates thread
> > safety.
> >
>
> I understand the position and while the likelihood is essentially next
> to zero that something like that could happen it's also possible to
> remove @def and pass copies of the name, parent, parent_wwnn,
> parent_wwpn, and parent_fabric_wwn.
>
> So before I do that - can we close on patches 1-12?

Yeah, I wasn't explicit with the ACK for 1-12 because I first wanted to have a
small discussion about the changes, but we can now focus solely on the rest.

So, ACK to 1-12 with the proposed change to virNode*ObjNew not consuming @def
(I don't have a strong preference here that I could back with some reasonable
pros/cons, to me, having it behave like a constructor for OOP, which we're
essentially trying to copy in libvirt, naturally makes sense, but I also see
Michal's point).

Erik


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