[PATCH] nvme: allow to re-attach namespaces after all paths are down

[Sagi Grimberg]

>>>> @@ -3605,16 +3608,26 @@ static int nvme_init_ns_head(struct nvme_ns 
>>>> *ns, unsigned nsid,
>>>>           head->shared = is_shared;
>>>>       } else {
>>>>           ret = -EINVAL;
>>>> -        if (!is_shared || !head->shared) {
>>>> -            dev_err(ctrl->device,
>>>> -                "Duplicate unshared namespace %d\n", nsid);
>>>> -            goto out_put_ns_head;
>>>> -        }
>>>> -        if (!nvme_ns_ids_equal(&head->ids, ids)) {
>>>> -            dev_err(ctrl->device,
>>>> -                "IDs don't match for shared namespace %d\n",
>>>> +        /*
>>>> +         * If multipath is enabled we might hit an ns head with no
>>>> +         * paths, but that doesn't indicate it's a shared namespace.
>>>> +         */
>>>> +        if (!nvme_ns_head_multipath(head) ||
>>>> +            !list_empty(&head->list)) {
>>>> +            if (!is_shared || !head->shared) {
>>>> +                dev_err(ctrl->device,
>>>> +                    "Duplicate unshared namespace %d\n", nsid);
>>>> +                goto out_put_ns_head;
>>>> +            }
>>>
>>> If not multipath, then it is not shared. The above will fail attaching
>>> single-path namespaces to a known head.
>>>
>>> The rest is similar to something I was working on too, though, so I
>>> think it's the right direction.
>>>
>>>
>>>> +            if (!nvme_ns_ids_equal(&head->ids, ids)) {
>>>> +                dev_err(ctrl->device,
>>>> +                    "IDs don't match for shared namespace %d\n",
>>>>                       nsid);
>>>> -            goto out_put_ns_head;
>>>> +                goto out_put_ns_head;
>>>> +            }
>>>> +        } else {
>>>> +            /* But the ids might have changed, so reset them */
>>>> +            head->ids = *ids;
>>>>           }
>>>>       }
>>>> @@ -3764,8 +3777,6 @@ static void nvme_ns_remove(struct nvme_ns *ns)
>>>>       mutex_lock(&ns->ctrl->subsys->lock);
>>>>       list_del_rcu(&ns->siblings);
>>>> -    if (list_empty(&ns->head->list))
>>>> -        list_del_init(&ns->head->entry);
>>
>> Hannes, you sent a patch like this before, my comment was that a nshead
>> should be removed before the final reference (which who knows when it
>> will ever arrive) as if a nsid were to be reused by the controller for
>> a different namespace then we'd reject it so I'm not sure how this helps?
>>
> 
> But we _need_ to have the nshead available in the linked list if we ever 
> want to re-attach a namespace after reconnecting the controller.

Again, this is a queue_if_no_path functionality, this sequence is 
correct for that.

> And we should be able to catch the 'reuse namespace' issue by checking 
> the contents of the nsids field, which we keep attached to the nshead.

Well, you may be right, what I'm concerned about is that a new namespace
may be falsely identified as the same namespace because it is 
re-attaching to a lingering mounted mpath device.