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

[PATCH v2] 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 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);
 }
 

base-commit: d30a909f9bb5283e701a5fdfffac763ef57a3e7c
-- 
2.25.1

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

[Guixin Liu]

nvme_ctrl_limited_cns check in nvme_scan_ns_list can be removed.

Best regards.

Guixin Liu


在 2022/11/8 03:54, 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 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);
>   }
>   
>
> base-commit: d30a909f9bb5283e701a5fdfffac763ef57a3e7c

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

[Guixin Liu]

在 2022/11/8 10:30, Guixin Liu 写道:
> nvme_ctrl_limited_cns check in nvme_scan_ns_list can be removed.
>
Sorry, my bad, already removed.
> Best regards.
>
> Guixin Liu
>
>
> 在 2022/11/8 03:54, 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 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);
>>   }
>>
>> base-commit: d30a909f9bb5283e701a5fdfffac763ef57a3e7c

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

[Chaitanya Kulkarni]

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

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

[Keith Busch]

On Mon, Nov 07, 2022 at 12:54:44PM -0700, 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 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.

Looks fine, but I really don't look forward to finding out how many
devices, if any, were secretly relying on this fallback.

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

[Christoph Hellwig]

On Tue, Nov 08, 2022 at 04:41:20PM -0700, Keith Busch wrote:
> Looks fine, but I really don't look forward to finding out how many
> devices, if any, were secretly relying on this fallback.

I'm a little worried as well.  Maybe we can whitelist a few status
codes as suggestions that a device might just be a broken.  I.e.
"NVME_INVALID_FIELD | NVME_DNR" would very much be an indicator of tat.

Also Uday, the return value from nvme_scan_ns_list can and should be
dropped now.

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

[Uday Shankar]

On Wed, Nov 09, 2022 at 07:23:30AM +0100, Christoph Hellwig wrote:
> On Tue, Nov 08, 2022 at 04:41:20PM -0700, Keith Busch wrote:
> > Looks fine, but I really don't look forward to finding out how many
> > devices, if any, were secretly relying on this fallback.
> 
> I'm a little worried as well.  Maybe we can whitelist a few status
> codes as suggestions that a device might just be a broken.  I.e.
> "NVME_INVALID_FIELD | NVME_DNR" would very much be an indicator of tat.

Can we get away with just testing for the DNR bit? The practical goal of
this patch is to prevent fallback to sequential scan when the Identify
NS List command fails for transient reasons, and I think the error code
returned in such cases should not have DNR set.

> Also Uday, the return value from nvme_scan_ns_list can and should be
> dropped now.

We'll want to keep the return value if we go with the above suggestion.