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

[Hannes Reinecke]

On 5/11/21 12:25 AM, Sagi Grimberg wrote:
> 
>>> @@ -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.
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.

I know I need to fixup the patch for that, though.

> I think that for the raid issue we should solve that one directly, this
> patch may have other implications in my mind...

The RAID issue _is_ solved with this patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer