From: "Meneghini, John" <John.Meneghini@netapp.com>
To: Mike Snitzer <snitzer@redhat.com>, Sagi Grimberg <sagi@grimberg.me>
Cc: "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
Christoph Hellwig <hch@infradead.org>,
"dm-devel@redhat.com" <dm-devel@redhat.com>,
Hannes Reinecke <hare@suse.de>, Chao Leng <lengchao@huawei.com>,
Keith Busch <kbusch@kernel.org>,
"Meneghini, John" <John.Meneghini@netapp.com>,
Ewan Milne <emilne@redhat.com>
Subject: Re: [RESEND PATCH] nvme: explicitly use normal NVMe error handling when appropriate
Date: Fri, 14 Aug 2020 03:23:47 +0000 [thread overview]
Message-ID: <FE38832C-9966-4455-BE4D-74745AD1AEB1@netapp.com> (raw)
In-Reply-To: <20200813144811.GA5452@redhat.com>
On 8/13/20, 10:48 AM, "Mike Snitzer" <snitzer@redhat.com> wrote:
Commit 764e9332098c0 ("nvme-multipath: do not reset on unknown
status"), among other things, fixed NVME_SC_CMD_INTERRUPTED error
handling by changing multipathing's nvme_failover_req() to short-circuit
path failover and then fallback to NVMe's normal error handling (which
takes care of NVME_SC_CMD_INTERRUPTED).
This detour through native NVMe multipathing code is unwelcome because
it prevents NVMe core from handling NVME_SC_CMD_INTERRUPTED independent
of any multipathing concerns.
Introduce nvme_status_needs_local_error_handling() to prioritize
non-failover retry, when appropriate, in terms of normal NVMe error
handling. nvme_status_needs_local_error_handling() will naturely evolve
to include handling of any other errors that normal error handling must
be used for.
How is this any better than blk_path_error()?
nvme_failover_req()'s ability to fallback to normal NVMe error handling
has been preserved because it may be useful for future NVME_SC that
nvme_status_needs_local_error_handling() hasn't been trained for yet.
Signed-off-by: Mike Snitzer <snitzer@redhat.com>
---
drivers/nvme/host/core.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 88cff309d8e4..be749b690af7 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -252,6 +252,16 @@ static inline bool nvme_req_needs_retry(struct request *req)
return true;
}
+static inline bool nvme_status_needs_local_error_handling(u16 status)
+{
+ switch (status & 0x7ff) {
+ case NVME_SC_CMD_INTERRUPTED:
+ return true;
+ default:
+ return false;
+ }
+}
I assume that what you mean by nvme_status_needs_local_error_handling is - do you want the nvme core
driver to handle the command retry.
If this is true, I don't think this function will ever work correctly because,. as discussed, whether or
not the command needs to be retried has nothing to do with the NVMe Status Code Field itself, it
has to so with the DNR bit, the CRD field, and the Status Code Type field - in that order.
+
static void nvme_retry_req(struct request *req)
{
struct nvme_ns *ns = req->q->queuedata;
@@ -270,7 +280,8 @@ static void nvme_retry_req(struct request *req)
void nvme_complete_rq(struct request *req)
{
- blk_status_t status = nvme_error_status(nvme_req(req)->status);
+ u16 nvme_status = nvme_req(req)->status;
+ blk_status_t status = nvme_error_status(nvme_status);
trace_nvme_complete_rq(req);
@@ -280,7 +291,8 @@ void nvme_complete_rq(struct request *req)
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) && nvme_failover_req(req))
+ if (!nvme_status_needs_local_error_handling(nvme_status) &&
This defeats the nvme-multipath logic by inserting a second evaluation of the NVMe Status Code into the retry logic.
This is basically another version of blk_path_error().
In fact, in your case REQ_NVME_MPATH is probably not set, so I don't see what difference this would make at all.
/John
+ (req->cmd_flags & REQ_NVME_MPATH) && nvme_failover_req(req))
return;
if (!blk_queue_dying(req->q)) {
--
2.18.0
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
next prev parent reply other threads:[~2020-08-14 3:24 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-27 5:58 [PATCH] nvme-core: fix io interrupt when work with dm-multipah Chao Leng
2020-07-28 11:19 ` Christoph Hellwig
2020-07-29 2:54 ` Chao Leng
2020-07-29 5:59 ` Christoph Hellwig
2020-07-30 1:49 ` Chao Leng
2020-08-05 6:40 ` Chao Leng
2020-08-05 15:29 ` Keith Busch
2020-08-06 5:52 ` Chao Leng
2020-08-06 14:26 ` Keith Busch
2020-08-06 15:59 ` Meneghini, John
2020-08-06 16:17 ` Meneghini, John
2020-08-06 18:40 ` Mike Snitzer
2020-08-06 19:19 ` [PATCH] nvme: restore use of blk_path_error() in nvme_complete_rq() Mike Snitzer
2020-08-06 22:42 ` Meneghini, John
2020-08-07 0:07 ` Mike Snitzer
2020-08-07 1:21 ` Sagi Grimberg
2020-08-07 4:50 ` Mike Snitzer
2020-08-07 23:35 ` Sagi Grimberg
2020-08-08 21:08 ` Meneghini, John
2020-08-08 21:11 ` Meneghini, John
2020-08-10 14:48 ` Mike Snitzer
2020-08-11 12:54 ` Meneghini, John
2020-08-10 8:10 ` Chao Leng
2020-08-11 12:36 ` Meneghini, John
2020-08-12 7:51 ` Chao Leng
2020-08-10 14:36 ` Mike Snitzer
2020-08-10 17:22 ` [PATCH] nvme: explicitly use normal NVMe error handling when appropriate Mike Snitzer
2020-08-11 3:32 ` Chao Leng
2020-08-11 4:20 ` Mike Snitzer
2020-08-11 6:17 ` Chao Leng
2020-08-11 14:12 ` Mike Snitzer
2020-08-13 14:48 ` [RESEND PATCH] " Mike Snitzer
2020-08-13 15:29 ` Meneghini, John
2020-08-13 15:43 ` Mike Snitzer
2020-08-13 15:59 ` Meneghini, John
2020-08-13 15:36 ` Christoph Hellwig
2020-08-13 17:47 ` Mike Snitzer
2020-08-13 18:43 ` Christoph Hellwig
2020-08-13 19:03 ` Mike Snitzer
2020-08-14 4:26 ` Meneghini, John
2020-08-14 6:53 ` Sagi Grimberg
2020-08-14 6:55 ` Christoph Hellwig
2020-08-14 7:02 ` Sagi Grimberg
2020-08-14 3:23 ` Meneghini, John [this message]
2020-08-07 0:44 ` [PATCH] nvme: restore use of blk_path_error() in nvme_complete_rq() Sagi Grimberg
2020-08-10 12:43 ` Christoph Hellwig
2020-08-10 15:06 ` Mike Snitzer
2020-08-11 3:45 ` [PATCH] " Chao Leng
2020-08-07 0:03 ` [PATCH] nvme-core: fix io interrupt when work with dm-multipah Sagi Grimberg
2020-08-07 2:28 ` Chao Leng
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=FE38832C-9966-4455-BE4D-74745AD1AEB1@netapp.com \
--to=john.meneghini@netapp.com \
--cc=dm-devel@redhat.com \
--cc=emilne@redhat.com \
--cc=hare@suse.de \
--cc=hch@infradead.org \
--cc=kbusch@kernel.org \
--cc=lengchao@huawei.com \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
--cc=snitzer@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).