[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Hannes Reinecke]

If the DNR bit is set we should not retry the command, even if
the standard status evaluation indicates so.

Signed-off-by: Hannes Reinecke <hare at suse.com>
---
 drivers/nvme/host/core.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cc09b81fc7f4..c3e9254f4757 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
 			return;
 		}
 	}
+	/*
+	 * Any pathing error might be retried, but the DNR bit takes
+	 * precedence. So return BLK_STS_TARGET if the DNR bit is set
+	 * to avoid retrying.
+	 */
+	if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
+		status = BLK_STS_TARGET;
 	blk_mq_end_request(req, status);
 }
 EXPORT_SYMBOL_GPL(nvme_complete_rq);
-- 
2.16.4

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Nadolski, Edmund]

On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
> If the DNR bit is set we should not retry the command, even if
> the standard status evaluation indicates so.
> 
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
>   drivers/nvme/host/core.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cc09b81fc7f4..c3e9254f4757 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
>   			return;
>   		}
>   	}
> +	/*
> +	 * Any pathing error might be retried, but the DNR bit takes
> +	 * precedence. So return BLK_STS_TARGET if the DNR bit is set
> +	 * to avoid retrying.
> +	 */
> +	if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
> +		status = BLK_STS_TARGET;
>   	blk_mq_end_request(req, status);
>   }
>   EXPORT_SYMBOL_GPL(nvme_complete_rq);
> 

- If DNR always takes precedence, is the blk_path_error() check still 
needed?

- Can/Should this be done in nvme_error_status()?

Ed

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Hannes Reinecke]

On 8/6/19 3:50 PM, Nadolski, Edmund wrote:
> On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
>> If the DNR bit is set we should not retry the command, even if
>> the standard status evaluation indicates so.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>> ---
>> ? drivers/nvme/host/core.c | 7 +++++++
>> ? 1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index cc09b81fc7f4..c3e9254f4757 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
>> ????????????? return;
>> ????????? }
>> ????? }
>> +??? /*
>> +???? * Any pathing error might be retried, but the DNR bit takes
>> +???? * precedence. So return BLK_STS_TARGET if the DNR bit is set
>> +???? * to avoid retrying.
>> +???? */
>> +??? if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
>> +??????? status = BLK_STS_TARGET;
>> ????? blk_mq_end_request(req, status);
>> ? }
>> ? EXPORT_SYMBOL_GPL(nvme_complete_rq);
>>
> 
> - If DNR always takes precedence, is the blk_path_error() check still 
> needed?
> 
It takes precedence in the sense that it should cause the command not to 
be retried. It should not overwrite any error code indicating a 
non-retryable error.

> - Can/Should this be done in nvme_error_status()?
> 
Possibly; I don't mind.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Keith Busch]

On Tue, Aug 06, 2019@03:53:29PM +0200, Hannes Reinecke wrote:
> On 8/6/19 3:50 PM, Nadolski, Edmund wrote:
> > On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
> > > If the DNR bit is set we should not retry the command, even if
> > > the standard status evaluation indicates so.
> > > 
> > > Signed-off-by: Hannes Reinecke <hare at suse.com>
> > > ---
> > > ? drivers/nvme/host/core.c | 7 +++++++
> > > ? 1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > index cc09b81fc7f4..c3e9254f4757 100644
> > > --- a/drivers/nvme/host/core.c
> > > +++ b/drivers/nvme/host/core.c
> > > @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
> > > ????????????? return;
> > > ????????? }
> > > ????? }
> > > +??? /*
> > > +???? * Any pathing error might be retried, but the DNR bit takes
> > > +???? * precedence. So return BLK_STS_TARGET if the DNR bit is set
> > > +???? * to avoid retrying.
> > > +???? */
> > > +??? if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
> > > +??????? status = BLK_STS_TARGET;
> > > ????? blk_mq_end_request(req, status);
> > > ? }
> > > ? EXPORT_SYMBOL_GPL(nvme_complete_rq);
> > > 
> > 
> > - If DNR always takes precedence, is the blk_path_error() check still
> > needed?
> > 
> It takes precedence in the sense that it should cause the command not to be
> retried. It should not overwrite any error code indicating a non-retryable
> error.

But we're already past the nvme retry logic, which also handles the DNR
bit. Who is this telling not to retry?

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Hannes Reinecke]

On 8/6/19 4:07 PM, Keith Busch wrote:
> On Tue, Aug 06, 2019@03:53:29PM +0200, Hannes Reinecke wrote:
>> On 8/6/19 3:50 PM, Nadolski, Edmund wrote:
>>> On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
>>>> If the DNR bit is set we should not retry the command, even if
>>>> the standard status evaluation indicates so.
>>>>
>>>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>>>> ---
>>>>  ? drivers/nvme/host/core.c | 7 +++++++
>>>>  ? 1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>> index cc09b81fc7f4..c3e9254f4757 100644
>>>> --- a/drivers/nvme/host/core.c
>>>> +++ b/drivers/nvme/host/core.c
>>>> @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
>>>>  ????????????? return;
>>>>  ????????? }
>>>>  ????? }
>>>> +??? /*
>>>> +???? * Any pathing error might be retried, but the DNR bit takes
>>>> +???? * precedence. So return BLK_STS_TARGET if the DNR bit is set
>>>> +???? * to avoid retrying.
>>>> +???? */
>>>> +??? if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
>>>> +??????? status = BLK_STS_TARGET;
>>>>  ????? blk_mq_end_request(req, status);
>>>>  ? }
>>>>  ? EXPORT_SYMBOL_GPL(nvme_complete_rq);
>>>>
>>>
>>> - If DNR always takes precedence, is the blk_path_error() check still
>>> needed?
>>>
>> It takes precedence in the sense that it should cause the command not to be
>> retried. It should not overwrite any error code indicating a non-retryable
>> error.
> 
> But we're already past the nvme retry logic, which also handles the DNR
> bit. Who is this telling not to retry?
> 
Anything layered on top of it, namely dm-multipath or MD.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Keith Busch]

On Tue, Aug 06, 2019@04:13:40PM +0200, Hannes Reinecke wrote:
> On 8/6/19 4:07 PM, Keith Busch wrote:
> > On Tue, Aug 06, 2019@03:53:29PM +0200, Hannes Reinecke wrote:
> > > On 8/6/19 3:50 PM, Nadolski, Edmund wrote:
> > > > On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
> > > > > If the DNR bit is set we should not retry the command, even if
> > > > > the standard status evaluation indicates so.
> > > > > 
> > > > > Signed-off-by: Hannes Reinecke <hare at suse.com>
> > > > > ---
> > > > >  ? drivers/nvme/host/core.c | 7 +++++++
> > > > >  ? 1 file changed, 7 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > > > > index cc09b81fc7f4..c3e9254f4757 100644
> > > > > --- a/drivers/nvme/host/core.c
> > > > > +++ b/drivers/nvme/host/core.c
> > > > > @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
> > > > >  ????????????? return;
> > > > >  ????????? }
> > > > >  ????? }
> > > > > +??? /*
> > > > > +???? * Any pathing error might be retried, but the DNR bit takes
> > > > > +???? * precedence. So return BLK_STS_TARGET if the DNR bit is set
> > > > > +???? * to avoid retrying.
> > > > > +???? */
> > > > > +??? if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
> > > > > +??????? status = BLK_STS_TARGET;
> > > > >  ????? blk_mq_end_request(req, status);
> > > > >  ? }
> > > > >  ? EXPORT_SYMBOL_GPL(nvme_complete_rq);
> > > > > 
> > > > 
> > > > - If DNR always takes precedence, is the blk_path_error() check still
> > > > needed?
> > > > 
> > > It takes precedence in the sense that it should cause the command not to be
> > > retried. It should not overwrite any error code indicating a non-retryable
> > > error.
> > 
> > But we're already past the nvme retry logic, which also handles the DNR
> > bit. Who is this telling not to retry?
> > 
> Anything layered on top of it, namely dm-multipath or MD.

Okay, that's what I thought. Can we just move this check to fit in the
unlikely() error case just above where you have it?

[PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set

[Hannes Reinecke]

On 8/6/19 4:29 PM, Keith Busch wrote:
> On Tue, Aug 06, 2019@04:13:40PM +0200, Hannes Reinecke wrote:
>> On 8/6/19 4:07 PM, Keith Busch wrote:
>>> On Tue, Aug 06, 2019@03:53:29PM +0200, Hannes Reinecke wrote:
>>>> On 8/6/19 3:50 PM, Nadolski, Edmund wrote:
>>>>> On 8/6/2019 5:10 AM, Hannes Reinecke wrote:
>>>>>> If the DNR bit is set we should not retry the command, even if
>>>>>> the standard status evaluation indicates so.
>>>>>>
>>>>>> Signed-off-by: Hannes Reinecke <hare at suse.com>
>>>>>> ---
>>>>>>   ? drivers/nvme/host/core.c | 7 +++++++
>>>>>>   ? 1 file changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>>>>> index cc09b81fc7f4..c3e9254f4757 100644
>>>>>> --- a/drivers/nvme/host/core.c
>>>>>> +++ b/drivers/nvme/host/core.c
>>>>>> @@ -279,6 +279,13 @@ void nvme_complete_rq(struct request *req)
>>>>>>   ????????????? return;
>>>>>>   ????????? }
>>>>>>   ????? }
>>>>>> +??? /*
>>>>>> +???? * Any pathing error might be retried, but the DNR bit takes
>>>>>> +???? * precedence. So return BLK_STS_TARGET if the DNR bit is set
>>>>>> +???? * to avoid retrying.
>>>>>> +???? */
>>>>>> +??? if (blk_path_error(status) && nvme_req(req)->status & NVME_SC_DNR)
>>>>>> +??????? status = BLK_STS_TARGET;
>>>>>>   ????? blk_mq_end_request(req, status);
>>>>>>   ? }
>>>>>>   ? EXPORT_SYMBOL_GPL(nvme_complete_rq);
>>>>>>
>>>>>
>>>>> - If DNR always takes precedence, is the blk_path_error() check still
>>>>> needed?
>>>>>
>>>> It takes precedence in the sense that it should cause the command not to be
>>>> retried. It should not overwrite any error code indicating a non-retryable
>>>> error.
>>>
>>> But we're already past the nvme retry logic, which also handles the DNR
>>> bit. Who is this telling not to retry?
>>>
>> Anything layered on top of it, namely dm-multipath or MD.
> 
> Okay, that's what I thought. Can we just move this check to fit in the
> unlikely() error case just above where you have it?
> 
Why, sure, of course.
Will be sending an updated patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)