Re: [PATCH v2 3/3] nvme: fire discovery log page change events to userspace

[Hannes Reinecke <hare@suse.de>]

On 8/31/19 12:24 AM, James Smart wrote:
> On 8/30/2019 2:07 PM, Sagi Grimberg wrote:
>>
>>>>>>> Yes we do, userspace should use it to order events.  Does udev not
>>>>>>> handle that properly today?
>>>>>>
>>>>>> The problem is not ordering of events, its really about the fact that
>>>>>> the chardev can be removed and reallocated for a different controller
>>>>>> (could be a completely different discovery controller) by the time
>>>>>> that userspace handles the event.
>>>>>
>>>>> The same is generally true for lot of kernel devices.  We could reduce
>>>>> the chance by using the idr cyclic allocator.
>>>>
>>>> Well, it was raised by Hannes and James, so I'll ask them respond here
>>>> because I don't mind having it this way. I personally think that this
>>>> is a better approach than having a cyclic idr allocator. In general, I
>>>> don't necessarily think that this is a good idea to have cyclic
>>>> controller enumerations if we don't absolutely have to...
>>>
>>> We hit it right and left without the cyclic allocator, but that won't
>>> necessarily remove it.
>>>
>>> Perhaps we should have had a unique token assigned to the controller,
>>> and have the event pass the name and the token.  The cli would then,
>>> if the token is present, validate it via an ioctl before proceeding
>>> with other ioctls.
>>>
>>> Where all the connection arguments were added we due to the reuse
>>> issue and then solving the question of how to verify and/or lookup
>>> the desired controller, by using the shotgun approach rather than
>>> being very pointed, which is what the name/token would do.
>>
>> This unique token is: trtype:traddr:trsvcid:host-traddr ...
> 
> well yes :)  though rather verbose.   There is still a minute window as
> we're comparing values in sysfs, prior to opening the device, so
> technically something could change in that window between when we
> checked sysfs and when we open'd.   We can certain check after we open
> the device to solve that issue.
> 
> There is some elegance to a 32-bit token for the controller (can be an
> incrementing value) passed in the event and used when servicing the
> event that avoids a bunch of work.
> 
> Doing either of these would eliminate what Hannes liked - looking for
> the discovery controller with those attributes. Although, I don't know
> that looking for it is all that meaningful.
> 
yeah, we do have this fundamental problem with uevents; they do refer to
a '/dev/nvmeX' device with those parameters, but this device might be
long gone (or have been reallocated) by the time the event is processed.

From my POV we have two choices here:
- rely on the transport options to find a matching controller (ignoring
the device name) and use that for sending out discovery requests. In the
end, it shouldn't really matter device we're using if the transport
options are identical. Although I'm not sure for RDMA; here we don't
necessarily have a host transport address, so we _might_ send the
discovery via the wrong controller in a CMIC enviroment.
- Match the options in nvme-cli, and just discard the event if it
doesn't match. Which is some additional coding in nvme-cli and might ran
afoul if we ever miss events.

I'd go for the second option; after considering the first introduces far
too many conditions rendering debugging impractical.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer