All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 1/2] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
  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
  0 siblings, 0 replies; 6+ messages in thread
From: Daniel Wagner @ 2021-01-27  9:14 UTC (permalink / raw)
  To: Hannes Reinecke; +Cc: linux-nvme, Christoph Hellwig, Keith Busch, Sagi Grimberg

On Wed, Jan 27, 2021 at 09:06:04AM +0100, Hannes Reinecke wrote:
> 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>

Reviewed-by: Daniel Wagner <dwagner@suse.de>

_______________________________________________
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  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 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.