* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
@ 2019-08-12 7:51 Hannes Reinecke
2019-08-12 23:37 ` Nadolski, Edmund
2019-08-13 17:01 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Hannes Reinecke @ 2019-08-12 7:51 UTC (permalink / raw)
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 | 21 +++++++++++++--------
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cc09b81fc7f4..5e1309709917 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -267,15 +267,20 @@ void nvme_complete_rq(struct request *req)
if (nvme_req(req)->ctrl->kas)
nvme_req(req)->ctrl->comp_seen = true;
- if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
- if ((req->cmd_flags & REQ_NVME_MPATH) &&
- blk_path_error(status)) {
- nvme_failover_req(req);
- return;
+ if (unlikely(status != BLK_STS_OK)) {
+ if (nvme_req_needs_retry(req)) {
+ if ((req->cmd_flags & REQ_NVME_MPATH) &&
+ blk_path_error(status)) {
+ nvme_failover_req(req);
+ return;
+ }
+ if (!blk_queue_dying(req->q)) {
+ nvme_retry_req(req);
+ return;
+ }
}
-
- if (!blk_queue_dying(req->q)) {
- nvme_retry_req(req);
+ if (nvme_req(req)->status & NVME_SC_DNR) {
+ blk_mq_end_request(req, BLK_STS_TARGET);
return;
}
}
--
2.16.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-12 7:51 [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set Hannes Reinecke
@ 2019-08-12 23:37 ` Nadolski, Edmund
2019-08-13 6:32 ` Sagi Grimberg
2019-08-13 17:01 ` Christoph Hellwig
1 sibling, 1 reply; 9+ messages in thread
From: Nadolski, Edmund @ 2019-08-12 23:37 UTC (permalink / raw)
On 8/12/2019 1:51 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 | 21 +++++++++++++--------
> 1 file changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cc09b81fc7f4..5e1309709917 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -267,15 +267,20 @@ void nvme_complete_rq(struct request *req)
> if (nvme_req(req)->ctrl->kas)
> nvme_req(req)->ctrl->comp_seen = true;
>
> - if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
> - if ((req->cmd_flags & REQ_NVME_MPATH) &&
> - blk_path_error(status)) {
> - nvme_failover_req(req);
> - return;
> + if (unlikely(status != BLK_STS_OK)) {
> + if (nvme_req_needs_retry(req)) {
> + if ((req->cmd_flags & REQ_NVME_MPATH) &&
> + blk_path_error(status)) {
> + nvme_failover_req(req);
> + return;
> + }
> + if (!blk_queue_dying(req->q)) {
> + nvme_retry_req(req);
> + return;
> + }
> }
> -
> - if (!blk_queue_dying(req->q)) {
> - nvme_retry_req(req);
> + if (nvme_req(req)->status & NVME_SC_DNR) {
> + blk_mq_end_request(req, BLK_STS_TARGET);
> return;
> }
> }
This seems redundant, to re-check the NVME_SC_DNR here, after just
checking it in nvme_req_needs_retry().
Ed
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-12 23:37 ` Nadolski, Edmund
@ 2019-08-13 6:32 ` Sagi Grimberg
2019-08-13 14:15 ` Keith Busch
0 siblings, 1 reply; 9+ messages in thread
From: Sagi Grimberg @ 2019-08-13 6:32 UTC (permalink / raw)
On 8/12/19 4:37 PM, Nadolski, Edmund wrote:
> On 8/12/2019 1:51 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 | 21 +++++++++++++--------
>> ? 1 file changed, 13 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index cc09b81fc7f4..5e1309709917 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -267,15 +267,20 @@ void nvme_complete_rq(struct request *req)
>> ????? if (nvme_req(req)->ctrl->kas)
>> ????????? nvme_req(req)->ctrl->comp_seen = true;
>> -??? if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
>> -??????? if ((req->cmd_flags & REQ_NVME_MPATH) &&
>> -??????????? blk_path_error(status)) {
>> -??????????? nvme_failover_req(req);
>> -??????????? return;
>> +??? if (unlikely(status != BLK_STS_OK)) {
>> +??????? if (nvme_req_needs_retry(req)) {
>> +??????????? if ((req->cmd_flags & REQ_NVME_MPATH) &&
>> +??????????????? blk_path_error(status)) {
>> +??????????????? nvme_failover_req(req);
>> +??????????????? return;
>> +??????????? }
>> +??????????? if (!blk_queue_dying(req->q)) {
>> +??????????????? nvme_retry_req(req);
>> +??????????????? return;
>> +??????????? }
>> ????????? }
>> -
>> -??????? if (!blk_queue_dying(req->q)) {
>> -??????????? nvme_retry_req(req);
>> +??????? if (nvme_req(req)->status & NVME_SC_DNR) {
>> +??????????? blk_mq_end_request(req, BLK_STS_TARGET);
>> ????????????? return;
>> ????????? }
>> ????? }
>
> This seems redundant, to re-check the NVME_SC_DNR here, after just
> checking it in nvme_req_needs_retry().
Indeed, why do we need this at this point? we can simply continue and
end the request down in the func (after we trace it as well).
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-13 6:32 ` Sagi Grimberg
@ 2019-08-13 14:15 ` Keith Busch
2019-08-13 16:37 ` Sagi Grimberg
0 siblings, 1 reply; 9+ messages in thread
From: Keith Busch @ 2019-08-13 14:15 UTC (permalink / raw)
On Mon, Aug 12, 2019@11:32:20PM -0700, Sagi Grimberg wrote:
> On 8/12/19 4:37 PM, Nadolski, Edmund wrote:
> > On 8/12/2019 1:51 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 | 21 +++++++++++++--------
> >> ? 1 file changed, 13 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> >> index cc09b81fc7f4..5e1309709917 100644
> >> --- a/drivers/nvme/host/core.c
> >> +++ b/drivers/nvme/host/core.c
> >> @@ -267,15 +267,20 @@ void nvme_complete_rq(struct request *req)
> >> ????? if (nvme_req(req)->ctrl->kas)
> >> ????????? nvme_req(req)->ctrl->comp_seen = true;
> >> -??? if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
> >> -??????? if ((req->cmd_flags & REQ_NVME_MPATH) &&
> >> -??????????? blk_path_error(status)) {
> >> -??????????? nvme_failover_req(req);
> >> -??????????? return;
> >> +??? if (unlikely(status != BLK_STS_OK)) {
> >> +??????? if (nvme_req_needs_retry(req)) {
> >> +??????????? if ((req->cmd_flags & REQ_NVME_MPATH) &&
> >> +??????????????? blk_path_error(status)) {
> >> +??????????????? nvme_failover_req(req);
> >> +??????????????? return;
> >> +??????????? }
> >> +??????????? if (!blk_queue_dying(req->q)) {
> >> +??????????????? nvme_retry_req(req);
> >> +??????????????? return;
> >> +??????????? }
> >> ????????? }
> >> -
> >> -??????? if (!blk_queue_dying(req->q)) {
> >> -??????????? nvme_retry_req(req);
> >> +??????? if (nvme_req(req)->status & NVME_SC_DNR) {
> >> +??????????? blk_mq_end_request(req, BLK_STS_TARGET);
> >> ????????????? return;
> >> ????????? }
> >> ????? }
> >
> > This seems redundant, to re-check the NVME_SC_DNR here, after just
> > checking it in nvme_req_needs_retry().
>
> Indeed, why do we need this at this point? we can simply continue and
> end the request down in the func (after we trace it as well).
We need to override the default nvme_error_status() to BLK_STS_TARGET
when DNR is set to prevent upper layers from their own retries/failover.
I suggested adding that check in the existing unlikely() case, but maybe
it's more obvious if we teach nvme_error_status() to default to return
BLK_STS_TARGET for this condition?
---
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 8f3fbe5ca937..50adcdde44d5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -199,6 +199,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
static blk_status_t nvme_error_status(struct request *req)
{
+ if (unlikely(nvme_req(req)->status & NVME_SC_DNR))
+ return BLK_STS_TARGET;
+
switch (nvme_req(req)->status & 0x7ff) {
case NVME_SC_SUCCESS:
return BLK_STS_OK;
--
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-13 14:15 ` Keith Busch
@ 2019-08-13 16:37 ` Sagi Grimberg
2019-08-13 16:56 ` Hannes Reinecke
0 siblings, 1 reply; 9+ messages in thread
From: Sagi Grimberg @ 2019-08-13 16:37 UTC (permalink / raw)
> We need to override the default nvme_error_status() to BLK_STS_TARGET
> when DNR is set to prevent upper layers from their own retries/failover.
> I suggested adding that check in the existing unlikely() case, but maybe
> it's more obvious if we teach nvme_error_status() to default to return
> BLK_STS_TARGET for this condition?
>
> ---
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 8f3fbe5ca937..50adcdde44d5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -199,6 +199,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
>
> static blk_status_t nvme_error_status(struct request *req)
> {
> + if (unlikely(nvme_req(req)->status & NVME_SC_DNR))
> + return BLK_STS_TARGET;
> +
> switch (nvme_req(req)->status & 0x7ff) {
> case NVME_SC_SUCCESS:
> return BLK_STS_OK;
> --
Makes sense to me, Hannes does this address your issue?
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-13 16:37 ` Sagi Grimberg
@ 2019-08-13 16:56 ` Hannes Reinecke
0 siblings, 0 replies; 9+ messages in thread
From: Hannes Reinecke @ 2019-08-13 16:56 UTC (permalink / raw)
On 8/13/19 6:37 PM, Sagi Grimberg wrote:
>
>> We need to override the default nvme_error_status() to BLK_STS_TARGET
>> when DNR is set to prevent upper layers from their own retries/failover.
>> I suggested adding that check in the existing unlikely() case, but maybe
>> it's more obvious if we teach nvme_error_status() to default to return
>> BLK_STS_TARGET for this condition?
>>
>> ---
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 8f3fbe5ca937..50adcdde44d5 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -199,6 +199,9 @@ static inline bool nvme_ns_has_pi(struct nvme_ns *ns)
>> ? static blk_status_t nvme_error_status(struct request *req)
>> ? {
>> +??? if (unlikely(nvme_req(req)->status & NVME_SC_DNR))
>> +??????? return BLK_STS_TARGET;
>> +
>> ????? switch (nvme_req(req)->status & 0x7ff) {
>> ????? case NVME_SC_SUCCESS:
>> ????????? return BLK_STS_OK;
>> --
>
> Makes sense to me, Hannes does this address your issue?
Oh, yes, it does.
I was slightly worried as we would override other non-retryable status
codes, but as there is no definite mapping which status codes are
retryable we can go with this one.
I'll be sending a 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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-12 7:51 [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set Hannes Reinecke
2019-08-12 23:37 ` Nadolski, Edmund
@ 2019-08-13 17:01 ` Christoph Hellwig
2019-08-13 17:03 ` Hannes Reinecke
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2019-08-13 17:01 UTC (permalink / raw)
On Mon, Aug 12, 2019@09:51:47AM +0200, Hannes Reinecke wrote:
> If the DNR bit is set we should not retry the command, even if
> the standard status evaluation indicates so.
What problem is this even trying to solve? Nothing in the
documentation of BLK_STS_TARGET says it should be retried any more
or less than other error code.
If you really care about a retryable vs not retryable distinction
at the block layer we need to propagate the equivalent of the DNR
bit.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-13 17:01 ` Christoph Hellwig
@ 2019-08-13 17:03 ` Hannes Reinecke
2019-08-13 17:08 ` Christoph Hellwig
0 siblings, 1 reply; 9+ messages in thread
From: Hannes Reinecke @ 2019-08-13 17:03 UTC (permalink / raw)
On 8/13/19 7:01 PM, Christoph Hellwig wrote:
> On Mon, Aug 12, 2019@09:51:47AM +0200, Hannes Reinecke wrote:
>> If the DNR bit is set we should not retry the command, even if
>> the standard status evaluation indicates so.
>
> What problem is this even trying to solve? Nothing in the
> documentation of BLK_STS_TARGET says it should be retried any more
> or less than other error code.
>
> If you really care about a retryable vs not retryable distinction
> at the block layer we need to propagate the equivalent of the DNR
> bit.
>
That was my worry, too.
But I wasn't sure if that was the direction we should be going.
Personally, I'd be more than happy to add a flag for non-retryable errors.
Let's see how it would look like ...
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)
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set
2019-08-13 17:03 ` Hannes Reinecke
@ 2019-08-13 17:08 ` Christoph Hellwig
0 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-08-13 17:08 UTC (permalink / raw)
On Tue, Aug 13, 2019@07:03:35PM +0200, Hannes Reinecke wrote:
> That was my worry, too.
> But I wasn't sure if that was the direction we should be going.
> Personally, I'd be more than happy to add a flag for non-retryable errors.
>
> Let's see how it would look like ...
Can you start by explaining what the actual problem is, e.g. what
you are doing, what the expected and what the actual behavior is.
That also belongs into the commit log.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-08-13 17:08 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 7:51 [PATCHv3] nvme: Return BLK_STS_TARGET if the DNR bit is set Hannes Reinecke
2019-08-12 23:37 ` Nadolski, Edmund
2019-08-13 6:32 ` Sagi Grimberg
2019-08-13 14:15 ` Keith Busch
2019-08-13 16:37 ` Sagi Grimberg
2019-08-13 16:56 ` Hannes Reinecke
2019-08-13 17:01 ` Christoph Hellwig
2019-08-13 17:03 ` Hannes Reinecke
2019-08-13 17:08 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).