* [PATCH 0/2] Fixup return value for nvme_submit_sync_cmd()
@ 2021-01-27 8:06 Hannes Reinecke
2021-01-27 8:06 ` [PATCH 1/2] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
2021-01-27 8:06 ` [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
0 siblings, 2 replies; 6+ messages in thread
From: Hannes Reinecke @ 2021-01-27 8:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, Daniel Wagner, Sagi Grimberg, Keith Busch,
Hannes Reinecke
Hi all,
here are two small patches for fixing up the return value of nvme_submit_sync_cmd().
As Keith correctly noted, nvme_submit_sync_cmd() should be returning
an error if the command could not be performed; however, currently
only pci does that.
So the first patch fixes up nvme_cancel_request() to return an -EINTR
on any pending sync commands during reset.
And the second patch adds a new flag 'NVME_REQ_TIMEOUT' to indicate
that a command had been aborted due to a timeout, and translates that
back into -ETIMEDOUT as a return code for nvme_submit_sync_cmd().
As usual, comments and reviews are welcome.
Hannes Reinecke (2):
nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()
drivers/nvme/host/core.c | 3 +++
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/rdma.c | 1 +
drivers/nvme/host/tcp.c | 1 +
4 files changed, 6 insertions(+)
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/2] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
2021-01-27 8:06 [PATCH 0/2] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
@ 2021-01-27 8:06 ` Hannes Reinecke
2021-01-27 9:14 ` Daniel Wagner
2021-01-27 8:06 ` [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
1 sibling, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2021-01-27 8:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, Daniel Wagner, Sagi Grimberg, Keith Busch,
Hannes Reinecke
NVME_REQ_CANCELLED is translated into -EINTR in nvme_submit_sync_cmd(),
so we should be setting this flags during nvme_cancel_request() to
ensure that the callers to nvme_submit_sync_cmd() will get the correct
error code when the controller is reset.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ce1b61519441..279d506ce794 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -367,6 +367,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
return true;
nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD;
+ nvme_req(req)->flags |= NVME_REQ_CANCELLED;
blk_mq_complete_request(req);
return true;
}
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()
2021-01-27 8:06 [PATCH 0/2] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 8:06 ` [PATCH 1/2] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
@ 2021-01-27 8:06 ` Hannes Reinecke
2021-01-27 9:22 ` Daniel Wagner
1 sibling, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2021-01-27 8:06 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-nvme, Daniel Wagner, Sagi Grimberg, Keith Busch,
Hannes Reinecke
When a command times out we should be returning -ETIMEDOUT in
nvme_submit_sync_cmd(), and not requiring the caller to guess whether
a specific NVMe status should be interpreted as a timeout.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/core.c | 2 ++
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/rdma.c | 1 +
drivers/nvme/host/tcp.c | 1 +
4 files changed, 5 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 279d506ce794..d7fea5a0aa1f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -972,6 +972,8 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
*result = nvme_req(req)->result;
if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
ret = -EINTR;
+ else if (nvme_req(req)->flags & NVME_REQ_TIMEOUT)
+ ret = -ETIMEDOUT;
else
ret = nvme_req(req)->status;
out:
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 7e49f61f81df..0d3c16d9e493 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -167,6 +167,7 @@ struct nvme_request {
enum {
NVME_REQ_CANCELLED = (1 << 0),
NVME_REQ_USERCMD = (1 << 1),
+ NVME_REQ_TIMEOUT = (1 << 2),
};
static inline struct nvme_request *nvme_req(struct request *req)
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index cf6c49d09c82..a4d5fbd50212 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1977,6 +1977,7 @@ static void nvme_rdma_complete_timed_out(struct request *rq)
nvme_rdma_stop_queue(queue);
if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) {
nvme_req(rq)->status = NVME_SC_HOST_ABORTED_CMD;
+ nvme_req(rq)->flags |= NVME_REQ_TIMEOUT;
blk_mq_complete_request(rq);
}
}
diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 1ba659927442..21415cba1c69 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2170,6 +2170,7 @@ static void nvme_tcp_complete_timed_out(struct request *rq)
nvme_tcp_stop_queue(ctrl, nvme_tcp_queue_id(req->queue));
if (blk_mq_request_started(rq) && !blk_mq_request_completed(rq)) {
nvme_req(rq)->status = NVME_SC_HOST_ABORTED_CMD;
+ nvme_req(rq)->flags |= NVME_REQ_TIMEOUT;
blk_mq_complete_request(rq);
}
}
--
2.29.2
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()
2021-01-27 8:06 ` [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
@ 2021-01-27 9:22 ` Daniel Wagner
2021-01-27 10:04 ` Hannes Reinecke
0 siblings, 1 reply; 6+ messages in thread
From: Daniel Wagner @ 2021-01-27 9:22 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: linux-nvme, Christoph Hellwig, Keith Busch, Sagi Grimberg
On Wed, Jan 27, 2021 at 09:06:05AM +0100, Hannes Reinecke wrote:
> When a command times out we should be returning -ETIMEDOUT in
> nvme_submit_sync_cmd(), and not requiring the caller to guess whether
> a specific NVMe status should be interpreted as a timeout.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/core.c | 2 ++
> drivers/nvme/host/nvme.h | 1 +
> drivers/nvme/host/rdma.c | 1 +
> drivers/nvme/host/tcp.c | 1 +
I wonder if we have to do something similar for FC. As far I understand
an abort command is issued and the drivers will return the original
request with FCPOP_STATE_ABORTED which then will be mapped to
NVME_SC_HOST_PATH_ERROR.
Can't say if this is good or bad, it's a just different.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()
2021-01-27 9:22 ` Daniel Wagner
@ 2021-01-27 10:04 ` Hannes Reinecke
0 siblings, 0 replies; 6+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:04 UTC (permalink / raw)
To: Daniel Wagner; +Cc: linux-nvme, Christoph Hellwig, Keith Busch, Sagi Grimberg
On 1/27/21 10:22 AM, Daniel Wagner wrote:
> On Wed, Jan 27, 2021 at 09:06:05AM +0100, Hannes Reinecke wrote:
>> When a command times out we should be returning -ETIMEDOUT in
>> nvme_submit_sync_cmd(), and not requiring the caller to guess whether
>> a specific NVMe status should be interpreted as a timeout.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>> drivers/nvme/host/core.c | 2 ++
>> drivers/nvme/host/nvme.h | 1 +
>> drivers/nvme/host/rdma.c | 1 +
>> drivers/nvme/host/tcp.c | 1 +
>
> I wonder if we have to do something similar for FC. As far I understand
> an abort command is issued and the drivers will return the original
> request with FCPOP_STATE_ABORTED which then will be mapped to
> NVME_SC_HOST_PATH_ERROR.
>
> Can't say if this is good or bad, it's a just different.
>
Good point, and indeed you are correct.
Will be sending a v2 version.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-27 10:05 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-01-27 8:06 [PATCH 0/2] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 8:06 ` [PATCH 1/2] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
2021-01-27 9:14 ` Daniel Wagner
2021-01-27 8:06 ` [PATCH 2/2] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 9:22 ` Daniel Wagner
2021-01-27 10:04 ` Hannes Reinecke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox