Re: [PATCH v2] nvme: scan sequentially only when list scan unsupported

[Chaitanya Kulkarni <chaitanyak@nvidia.com>]

Uday,

On 11/7/22 11:54, Uday Shankar wrote:
> Currently, if nvme_scan_ns_list fails, nvme_scan_work will fall back to
> a sequential scan. nvme_scan_ns_list can fail for a variety of reasons,
> e.g. transient transport issue. And the resulting sequential scan can be

's/issues .And/ issue and/' ?

> extremely expensive on controllers reporting an NN value close to the
> maximum allowed (>4 billion). Avoid sequential scans wherever possible
> by only falling back to them if nvme_scan_ns_list fails due to
> controller non-support of Identify NS List. This breaks (noncompliant)
> devices that claim to support version NVME_VS(1, 1, 0) or later, but
> don't support Identify NS List. Such devices can be made to work again
> using the existing NVME_QUIRK_IDENTIFY_CNS.
> 
> Signed-off-by: Uday Shankar <ushankar@purestorage.com>
> ---
> Changes from v1:
> - Move limited_cns check from nvme_scan_ns_list to nvme_scan_work
> - Move note about devices that may break with this change into commit
>    message
> 
>   drivers/nvme/host/core.c | 7 +++----
>   1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0090dc0b3ae6..5abd8d4c6d9b 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4425,9 +4425,6 @@ static int nvme_scan_ns_list(struct nvme_ctrl *ctrl)
>   	u32 prev = 0;
>   	int ret = 0, i;
>   
> -	if (nvme_ctrl_limited_cns(ctrl))
> -		return -EOPNOTSUPP;
> -
>   	ns_list = kzalloc(NVME_IDENTIFY_DATA_SIZE, GFP_KERNEL);
>   	if (!ns_list)
>   		return -ENOMEM;
> @@ -4535,8 +4532,10 @@ static void nvme_scan_work(struct work_struct *work)
>   	}
>   
>   	mutex_lock(&ctrl->scan_lock);
> -	if (nvme_scan_ns_list(ctrl) != 0)
> +	if (nvme_ctrl_limited_cns(ctrl))
>   		nvme_scan_ns_sequential(ctrl);
> +	else
> +		nvme_scan_ns_list(ctrl);
>   	mutex_unlock(&ctrl->scan_lock);
>   }
>   
> 

Overall it looks good, but I'm curious to know what kind of testing is
done on this patch in order to validate the correctness of this
patch ? or it this scenario is covered by any blktests/nvme category ?

-ck