[libvirt] [PATCH v4 14/14] nodedev: Remove driver locks around object list mgmt code
John Ferlan
jferlan at redhat.com
Fri Jul 14 16:44:49 UTC 2017
On 07/14/2017 11:37 AM, 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 at 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?
>
Let me amend this statement to include - modulo passing @def to
virNodeDeviceObjNew in patch 5 as Michal has requested in his review of
Secrets and NWFilter that @def is not passed to the vir*ObjNew.
I will also post a reversal for Interfaces to keep everything the same...
John
> I can then post a v5 that alters the virNodeDeviceObjListGetParentHost
> to take parameters instead of @def. That should allay this concern and
> make patches 13/14 be 2 and 3 of the next series.
>
> John
>
>>>
>>> BTW: I recall doing a review suggesting perhaps creating an mdev pool
>>> driver of sorts. Daniel essentially responded that using the node device
>>> driver and augmenting it to fit the needs would be OK. At the time, I
>>> wasn't specifically thinking about this case, but it certainly qualifies
>>> as something of concern where the existing node device code can make
>>> some assumptions about names that the underlying udev code provides.
>>>
>>> You may need to add some extra layer of protection if names can be
>>> reused especially because of this event mgmt "problem"... You may also
>>> need to modify that "if (dev)" code to check if the dev is an mdev and
>>> if so, do something different.
>>>
>>> Looks like the code was added by commit id '546fa3ef'. Perhaps as a way
>>> to "follow" how other drivers did things.
>>>
>>> "Fortunately" the only consumer of CreateXML/Destroy ends up being vHBA
>>> and we know there's rules around the naming. IIRC - you were letting
>>> MDEV also set the name, right? That is, a 'name' on input is essentially
>>
>> No, mdev's name is a readonly attribute that is optional and exposed by the
>> vendor, the only thing libvirt can currently write to the mdev's sysfs
>> interface is UUID.
>>
>> Erik
>>
>>> and happily ignored. So what creates that name? And can you be assured
>>> it's going to be unique for the life/run time of the host? If so,
>>> there's no way a CreateXML could reuse a name that AddOneDevice would be
>>> using, right?
>>>
>>> Maybe I need to think some more - it's been a long day already
>>>
>>> John
>>>
>>>>
>>>> The rest of the patch looks okay, but I want to try it first.
>>>>
>>>> Erik
>>>>
>
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
More information about the libvir-list
mailing list