* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set @ 2019-08-06 11:10 Hannes Reinecke 2019-08-06 13:50 ` Nadolski, Edmund 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2019-08-06 11:10 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 | 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 11:10 [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set Hannes Reinecke @ 2019-08-06 13:50 ` Nadolski, Edmund 2019-08-06 13:53 ` Hannes Reinecke 0 siblings, 1 reply; 7+ messages in thread From: Nadolski, Edmund @ 2019-08-06 13:50 UTC (permalink / raw) 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 13:50 ` Nadolski, Edmund @ 2019-08-06 13:53 ` Hannes Reinecke 2019-08-06 14:07 ` Keith Busch 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2019-08-06 13:53 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 13:53 ` Hannes Reinecke @ 2019-08-06 14:07 ` Keith Busch 2019-08-06 14:13 ` Hannes Reinecke 0 siblings, 1 reply; 7+ messages in thread From: Keith Busch @ 2019-08-06 14:07 UTC (permalink / raw) 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 14:07 ` Keith Busch @ 2019-08-06 14:13 ` Hannes Reinecke 2019-08-06 14:29 ` Keith Busch 0 siblings, 1 reply; 7+ messages in thread From: Hannes Reinecke @ 2019-08-06 14:13 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 14:13 ` Hannes Reinecke @ 2019-08-06 14:29 ` Keith Busch 2019-08-06 14:35 ` Hannes Reinecke 0 siblings, 1 reply; 7+ messages in thread From: Keith Busch @ 2019-08-06 14:29 UTC (permalink / raw) 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? ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set 2019-08-06 14:29 ` Keith Busch @ 2019-08-06 14:35 ` Hannes Reinecke 0 siblings, 0 replies; 7+ messages in thread From: Hannes Reinecke @ 2019-08-06 14:35 UTC (permalink / raw) 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) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-06 14:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-08-06 11:10 [PATCH] nvme: Return BLK_STS_TARGET if the DNR bit is set Hannes Reinecke 2019-08-06 13:50 ` Nadolski, Edmund 2019-08-06 13:53 ` Hannes Reinecke 2019-08-06 14:07 ` Keith Busch 2019-08-06 14:13 ` Hannes Reinecke 2019-08-06 14:29 ` Keith Busch 2019-08-06 14:35 ` Hannes Reinecke
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.