Re: [PATCH 2/2] nvme: add 'queue_if_no_path' semantics

[Hannes Reinecke <hare@suse.de>]

On 3/5/21 9:11 PM, Sagi Grimberg wrote:
> 
>> I think what you're showing is a problem for everyone, including
>> fabrics. The namespace was attached to the previously existing
>> subsystem, nvme1, but we didn't find the previously existing "head" to
>> reattach the namespace.
>>
>> I just threw the below together very hastily, so I'm sure there's
>> something wrong with it, but just to capture what's wrong with the
>> current code:
>>
>> ---
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 3833584b5cee..0b3bbbe6d1e8 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -3897,8 +3897,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);
> 
> Not removing it ever from the head->list?
> 
>>       mutex_unlock(&ns->ctrl->subsys->lock);
>>         synchronize_rcu(); /* guarantee not available in head->list */
>> -- 
> 
> This is a problem. beacuse if you don't remove the ns from the
> head->list nothing prevents it from being re-selected as the
> current_path after we synchronize the srcu, although it is going away...
> 
You misread the patch.
I'm not skipping the removal from 'head->list', I'm skipping the removal
from 'subsys->nsheads' (which is where 'head->entry' is pointing to).


But I'll be sending an updated patch which clarifies this situation.

Cheers,

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme