[PATCH 5/8] nvme: sync the namespace scanning during ctrl start

[Max Gurtovoy]

On 24/01/2024 15:54, Sagi Grimberg wrote:
> 
>>>>>> @@ -4537,9 +4536,10 @@ void nvme_start_ctrl(struct nvme_ctrl *ctrl)
>>>>>>           nvme_change_uevent(ctrl, "NVME_EVENT=rediscover");
>>>>>>       if (ctrl->queue_count > 1) {
>>>>>> -        nvme_queue_scan(ctrl);
>>>>>> +        nvme_queue_scan_sync(ctrl);
>>>>>>           nvme_unquiesce_io_queues(ctrl);
>>>>>>           nvme_mpath_update(ctrl);
>>>>>> +        nvme_kick_requeue_lists(ctrl);
>>>>>>       }
>>>>>
>>>>> I really don't think its a good idea to block ctrl start
>>>>> like that.
>>>>
>>>> like how ? scan_sync ?
>>>
>>> Yes
>>
>> why not ? this is control path..
> 
> Because in the real world, namespaces (or HCAs) will not change their
> attributes in 99.999999% of the cases, and when they do, in 99.999999%
> of the cases the inflight IO has already failed over to a different
> path.
> 
> So no, I don't think that preventing the ctrl start from making forward
> progress until a full namespaces scan completes makes any sense.
> 

The correctness is important. Namespaces in NVMe can change dynamically.
Issue a request to non identified namespace doesn't sounds right to me.
For real world use case, that has 1-5 namespaces for a controller this 
sync will not cause any harm.
Also there is some patch sent recently to perform parallel scanning of 
namespaces so it will even not be stalled a lot for the 1k namespaces case.

>> we have to make sure we issue commands to a validated namespace.
> 
> I think we should simply refuse to create the ns when it differs between
> paths, or remove it in the highly unlikely case where it suddenly
> changes its attributes when reconnecting.

how will you refuse creating a namespace if you didn't finish scanning it ?
This is exactly the logic we did, but we need to get the new identifiers 
and understand whether we need to remove old ns and create a new one.