Re: [PATCH] nvme-core: Fix subsystem instance mismatches

[Logan Gunthorpe]

On 2019-09-04 9:54 a.m., Keith Busch wrote:
> On Wed, Sep 04, 2019 at 05:42:15PM +0200, Christoph Hellwig wrote:
>> On Wed, Sep 04, 2019 at 08:44:27AM -0600, Keith Busch wrote:
>>> Let me step through an example:
>>>
>>>   Ctrl A gets instance 0.
>>>
>>>   Its subsystem gets the same instance, and takes ref count on it:
>>>   all namespaces in this subsystem will use '0'.
>>>
>>>   Ctrl B gets instance 1, and it's in the same subsystem as Ctrl A so
>>>   no new subsytem is allocated.
>>>
>>>   Ctrl A is disconnected, dropping its ref on instance 0, but the
>>>   subsystem still has its refcount, making it unavailable.
>>>
>>>   Ctrl A is reconnected, and allocates instance 2 because 0 is still in
>>>   use.
>>>
>>> Now all the namespaces in this subsystem are prefixed with nvme0, but no
>>> controller exists with the same prefix. We still have inevitable naming
>>> mismatch, right?
>>
>> I think th major confusion was that we can use the same handle for
>> and unrelated subsystem vs controller, and that would avoid it.
>>
>> I don't see how we can avoid the controller is entirely different
>> from namespace problem ever.


Yes, I agree, we can't solve the mismatch problem in the general case:
with sequences of hot plug events there will always be a case that
mismatches. I just think we can do better in the simple common default case.


> Can we just ensure there is never a matching controller then? This
> patch will accomplish that and simpler than wrapping the instance in a
> refcount'ed object:
> 
> http://lists.infradead.org/pipermail/linux-nvme/2019-May/024142.html

I don't really like that idea. It reduces the confusion caused by
mismatching numbers, but causes the controller to never match the
namespace, which is also confusing but in a different way.

I like the nvme_instance idea. It's not going to be perfect but it has
some nice properties: the subsystem will try to match the controller's
instance whenever possible, but in cases where it doesn't, the instance
number of the subsystem will never be the same as an existing controller.

I'll see if I can work up a quick patch set and see what people think.

Logan