[PATCH v2] nvme-multipath: Early exit if no path is available

[Chao Leng]

On 2021/1/29 16:33, Hannes Reinecke wrote:
> On 1/29/21 8:45 AM, Chao Leng wrote:
>>
>>
>> On 2021/1/29 15:06, Hannes Reinecke wrote:
>>> On 1/29/21 4:07 AM, Chao Leng wrote:
>>>>
>>>>
>>>> On 2021/1/29 9:42, Sagi Grimberg wrote:
>>>>>
>>>>>>> You can't see exactly where it dies but I followed the assembly to
>>>>>>> nvme_round_robin_path(). Maybe it's not the initial nvme_next_ns(head,
>>>>>>> old) which returns NULL but nvme_next_ns() is returning NULL eventually
>>>>>>> (list_next_or_null_rcu()).
>>>>>> So there is other bug cause nvme_next_ns abormal.
>>>>>> I review the code about head->list and head->current_path, I find 2 bugs
>>>>>> may cause the bug:
>>>>>> First, I already send the patch. see:
>>>>>> https://lore.kernel.org/linux-nvme/20210128033351.22116-1-lengchao@huawei.com/
>>>>>> Second, in nvme_ns_remove, list_del_rcu is before
>>>>>> nvme_mpath_clear_current_path. This may cause "old" is deleted from the
>>>>>> "head", but still use "old". I'm not sure there's any other
>>>>>> consideration here, I will check it and try to fix it.
>>>>>
>>>>> The reason why we first remove from head->list and only then clear
>>>>> current_path is because the other way around there is no way
>>>>> to guarantee that that the ns won't be assigned as current_path
>>>>> again (because it is in head->list).
>>>> ok, I see.
>>>>>
>>>>> nvme_ns_remove fences continue of deletion of the ns by synchronizing
>>>>> the srcu such that for sure the current_path clearance is visible.
>>>> The list will be like this:
>>>> head->next = ns1;
>>>> ns1->next = head;
>>>> old->next = ns1;
>>>
>>> Where does 'old' pointing to?
>>>
>>>> This may cause infinite loop in nvme_round_robin_path.
>>>> for (ns = nvme_next_ns(head, old);
>>>>      ns != old;
>>>>      ns = nvme_next_ns(head, ns))
>>>> The ns will always be ns1, and then infinite loop.
>>>
>>> No. nvme_next_ns() will return NULL.
>> If there is just one path(the "old") and the "old" is deleted,
>> nvme_next_ns() will return NULL.
>> The list like this:
>> head->next = head;
>> old->next = head;
>> If there is two or more path and the "old" is deleted,
>> "for" will be infinite loop. because nvme_next_ns() will return
>> the path which in the list except the "old", check condition will
>> be true for ever.
> 
> But that will be caught by the statement above:
> 
> if (list_is_singular(&head->list))
> 
> no?
Two path just a sample example.
If there is just two path, will enter it, may cause no path but there is
actually one path. It is falsely assumed that the "old" must be not deleted.
If there is more than two path, will cause infinite loop.
> 
> Cheers,
> 
> Hannes