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

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

[Uday Shankar]

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
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 would break devices that claim to support version NVME_VS(1, 1, 0)
or above, but don't support Identify NS List. But it looks like we
already have NVME_QUIRK_IDENTIFY_CNS to deal with that.

 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 0090dc0b3ae6..5bd19819d33b 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4535,7 +4535,7 @@ static void nvme_scan_work(struct work_struct *work)
 	}
 
 	mutex_lock(&ctrl->scan_lock);
-	if (nvme_scan_ns_list(ctrl) != 0)
+	if (nvme_scan_ns_list(ctrl) == -EOPNOTSUPP)
 		nvme_scan_ns_sequential(ctrl);
 	mutex_unlock(&ctrl->scan_lock);
 }

base-commit: d30a909f9bb5283e701a5fdfffac763ef57a3e7c
-- 
2.25.1

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

[Uday Shankar]

> 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
> 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.

Signed-off-by: Uday Shankar <ushankar@purestorage.com>

> ---
> This would break devices that claim to support version NVME_VS(1, 1, 0)
> or above, but don't support Identify NS List. But it looks like we
> already have NVME_QUIRK_IDENTIFY_CNS to deal with that.
> 
>  drivers/nvme/host/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 0090dc0b3ae6..5bd19819d33b 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4535,7 +4535,7 @@ static void nvme_scan_work(struct work_struct *work)
>  	}
>  
>  	mutex_lock(&ctrl->scan_lock);
> -	if (nvme_scan_ns_list(ctrl) != 0)
> +	if (nvme_scan_ns_list(ctrl) == -EOPNOTSUPP)
>  		nvme_scan_ns_sequential(ctrl);
>  	mutex_unlock(&ctrl->scan_lock);
>  }
> 
> base-commit: d30a909f9bb5283e701a5fdfffac763ef57a3e7c
> -- 
> 2.25.1
> 

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

[Christoph Hellwig]

On Fri, Nov 04, 2022 at 04:36:22PM -0600, 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
> 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 would break devices that claim to support version NVME_VS(1, 1, 0)
> or above, but don't support Identify NS List. But it looks like we
> already have NVME_QUIRK_IDENTIFY_CNS to deal with that.

Yes.  I'm a little worried about changing something this fundamental
after such a long time, even if it is the right thing to do.  But at
least we get a warning when the Identify NS List fails, so between
that and the fact tht nvme_scan_ns_list logs an error I think we can
cope.

>  	mutex_lock(&ctrl->scan_lock);
> -	if (nvme_scan_ns_list(ctrl) != 0)
> +	if (nvme_scan_ns_list(ctrl) == -EOPNOTSUPP)
>  		nvme_scan_ns_sequential(ctrl);

Maybe make this a bit more clear by lifting the nvme_ctrl_limited_cns
check into nvme_scan_work, though:

	if (nvme_ctrl_limited_cns(ctrl))
		nvme_scan_ns_sequential(ctrl);
	else
		nvme_scan_ns_list(ctrl);

and drop the now superflous error return from nvme_scan_ns_list.

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

[Uday Shankar]

On Mon, Nov 07, 2022 at 06:56:05AM +0100, Christoph Hellwig wrote:
> Maybe make this a bit more clear by lifting the nvme_ctrl_limited_cns
> check into nvme_scan_work, though:
> 
> 	if (nvme_ctrl_limited_cns(ctrl))
> 		nvme_scan_ns_sequential(ctrl);
> 	else
> 		nvme_scan_ns_list(ctrl);
> 
> and drop the now superflous error return from nvme_scan_ns_list.

Sure. I will resubmit with that revision.