* [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
@ 2021-01-27 10:33 ` Hannes Reinecke
2021-01-27 15:05 ` Keith Busch
2021-01-28 8:15 ` Sagi Grimberg
2021-01-27 10:33 ` [PATCH 2/5] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange() Hannes Reinecke
` (4 subsequent siblings)
5 siblings, 2 replies; 13+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Sagi Grimberg, Daniel Wagner, James Smart,
linux-nvme, 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>
Reviewed-by: Daniel Wagner <dwagner@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 2e09eed3d898..4a5059472625 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -365,6 +365,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] 13+ messages in thread* Re: [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
2021-01-27 10:33 ` [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
@ 2021-01-27 15:05 ` Keith Busch
2021-01-28 8:15 ` Sagi Grimberg
1 sibling, 0 replies; 13+ messages in thread
From: Keith Busch @ 2021-01-27 15:05 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Keith Busch, Daniel Wagner, James Smart, linux-nvme,
Christoph Hellwig, Sagi Grimberg
On Wed, Jan 27, 2021 at 11:33:22AM +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>
Looks good,
Reviewed-by: Keith Busch <kbusch@kernel.org>
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
2021-01-27 10:33 ` [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
2021-01-27 15:05 ` Keith Busch
@ 2021-01-28 8:15 ` Sagi Grimberg
1 sibling, 0 replies; 13+ messages in thread
From: Sagi Grimberg @ 2021-01-28 8:15 UTC (permalink / raw)
To: Hannes Reinecke, Christoph Hellwig
Cc: linux-nvme, Daniel Wagner, Keith Busch, James Smart
> 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.
We will have an issue here. Because now nvme_validate_ns will return
EINTR, which means that now scans failing to validate during resets
will now end up incorrectly removing the namespace:
--
out:
/*
* Only remove the namespace if we got a fatal error back from the
* device, otherwise ignore the error and just move on.
*
* TODO: we should probably schedule a delayed retry here.
*/
if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
nvme_ns_remove(ns);
--
Did you check resets loop with scans with this?
If we go down this path (we already had this discussion in the
past), we need to decide on either of:
- Don't remove the namespace for any of the return codes from
nvme_submit_sync_cmd (understanding that this may be just a
transport issue).
- Or, return the raw positive nvme status (which doesn't have
the DNR bit set) and make sure we return ret codes on top.
But this change alone, re-triggers a condition that during
resets a namespace is incorrectly removed.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/5] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange()
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 10:33 ` [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
@ 2021-01-27 10:33 ` Hannes Reinecke
2021-01-27 10:59 ` Daniel Wagner
2021-01-27 10:33 ` [PATCH 3/5] nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been aborted Hannes Reinecke
` (3 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Sagi Grimberg, Daniel Wagner, James Smart,
linux-nvme, Hannes Reinecke
nvme_fc_terminate_exchange() is being called when exchanges are
being deleted, and as such we should be setting the NVME_REQ_CANCELLED
flag to have identical behaviour on all transports.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/fc.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 20dadd86e981..ef12a619daec 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2443,6 +2443,7 @@ nvme_fc_terminate_exchange(struct request *req, void *data, bool reserved)
struct nvme_fc_ctrl *ctrl = to_fc_ctrl(nctrl);
struct nvme_fc_fcp_op *op = blk_mq_rq_to_pdu(req);
+ op->nreq.flags |= NVME_REQ_CANCELLED;
__nvme_fc_abort_op(ctrl, op);
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] 13+ messages in thread* [PATCH 3/5] nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been aborted
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 10:33 ` [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request() Hannes Reinecke
2021-01-27 10:33 ` [PATCH 2/5] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange() Hannes Reinecke
@ 2021-01-27 10:33 ` Hannes Reinecke
2021-01-27 11:00 ` Daniel Wagner
2021-01-27 10:33 ` [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands Hannes Reinecke
` (2 subsequent siblings)
5 siblings, 1 reply; 13+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Sagi Grimberg, Daniel Wagner, James Smart,
linux-nvme, Hannes Reinecke
When a command has been aborted we should return NVME_SC_HOST_ABORTED_CMD
to be consistent with the other transports.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/fc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index ef12a619daec..97e3424c7b03 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1956,7 +1956,7 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
sizeof(op->rsp_iu), DMA_FROM_DEVICE);
if (opstate == FCPOP_STATE_ABORTED)
- status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
+ status = cpu_to_le16(NVME_SC_HOST_ABORTED_CMD << 1);
else if (freq->status) {
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
--
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] 13+ messages in thread* [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
` (2 preceding siblings ...)
2021-01-27 10:33 ` [PATCH 3/5] nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been aborted Hannes Reinecke
@ 2021-01-27 10:33 ` Hannes Reinecke
2021-01-27 11:01 ` Daniel Wagner
2021-01-27 10:33 ` [PATCH 5/5] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
2021-01-27 15:09 ` [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Keith Busch
5 siblings, 1 reply; 13+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Sagi Grimberg, Daniel Wagner, James Smart,
linux-nvme, Hannes Reinecke
A request will be cancelled when a queue is deleted, so we should
be returning a status of NVME_SC_ABORT_QUEUE.
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
drivers/nvme/host/core.c | 2 +-
drivers/nvme/host/fc.c | 9 ++++++---
2 files changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 4a5059472625..1f723b7e9efb 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -364,7 +364,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
if (blk_mq_request_completed(req))
return true;
- nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD;
+ nvme_req(req)->status = NVME_SC_ABORT_QUEUE;
nvme_req(req)->flags |= NVME_REQ_CANCELLED;
blk_mq_complete_request(req);
return true;
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 97e3424c7b03..86395b66310d 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1955,9 +1955,12 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
fc_dma_sync_single_for_cpu(ctrl->lport->dev, op->fcp_req.rspdma,
sizeof(op->rsp_iu), DMA_FROM_DEVICE);
- if (opstate == FCPOP_STATE_ABORTED)
- status = cpu_to_le16(NVME_SC_HOST_ABORTED_CMD << 1);
- else if (freq->status) {
+ if (opstate == FCPOP_STATE_ABORTED) {
+ if (op->nreq.flags & NVME_REQ_CANCELLED)
+ status = cpu_to_le16(NVME_SC_ABORT_QUEUE << 1);
+ else
+ status = cpu_to_le16(NVME_SC_HOST_ABORTED_CMD << 1);
+ } else if (freq->status) {
status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
dev_info(ctrl->ctrl.device,
"NVME-FC{%d}: io failed due to lldd error %d\n",
--
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] 13+ messages in thread* Re: [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands
2021-01-27 10:33 ` [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands Hannes Reinecke
@ 2021-01-27 11:01 ` Daniel Wagner
0 siblings, 0 replies; 13+ messages in thread
From: Daniel Wagner @ 2021-01-27 11:01 UTC (permalink / raw)
To: Hannes Reinecke
Cc: James Smart, linux-nvme, Christoph Hellwig, Keith Busch,
Sagi Grimberg
On Wed, Jan 27, 2021 at 11:30:31AM +0100, Hannes Reinecke wrote:
> A request will be cancelled when a queue is deleted, so we should
> be returning a status of NVME_SC_ABORT_QUEUE.
I think this patch should be spitted as it does two different
things.
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
> drivers/nvme/host/core.c | 2 +-
> drivers/nvme/host/fc.c | 9 ++++++---
> 2 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 4a5059472625..1f723b7e9efb 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -364,7 +364,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
> if (blk_mq_request_completed(req))
> return true;
>
> - nvme_req(req)->status = NVME_SC_HOST_ABORTED_CMD;
> + nvme_req(req)->status = NVME_SC_ABORT_QUEUE;
> nvme_req(req)->flags |= NVME_REQ_CANCELLED;
> blk_mq_complete_request(req);
> return true;
The commit message describes this part of the change.
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 97e3424c7b03..86395b66310d 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -1955,9 +1955,12 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
> fc_dma_sync_single_for_cpu(ctrl->lport->dev, op->fcp_req.rspdma,
> sizeof(op->rsp_iu), DMA_FROM_DEVICE);
>
> - if (opstate == FCPOP_STATE_ABORTED)
> - status = cpu_to_le16(NVME_SC_HOST_ABORTED_CMD << 1);
> - else if (freq->status) {
> + if (opstate == FCPOP_STATE_ABORTED) {
> + if (op->nreq.flags & NVME_REQ_CANCELLED)
> + status = cpu_to_le16(NVME_SC_ABORT_QUEUE << 1);
> + else
> + status = cpu_to_le16(NVME_SC_HOST_ABORTED_CMD << 1);
> + } else if (freq->status) {
IMO, this part is not explained in the commit message.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 5/5] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
` (3 preceding siblings ...)
2021-01-27 10:33 ` [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands Hannes Reinecke
@ 2021-01-27 10:33 ` Hannes Reinecke
2021-01-27 11:00 ` Daniel Wagner
2021-01-27 15:09 ` [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Keith Busch
5 siblings, 1 reply; 13+ messages in thread
From: Hannes Reinecke @ 2021-01-27 10:33 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Keith Busch, Sagi Grimberg, Daniel Wagner, James Smart,
linux-nvme, 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/fc.c | 1 +
drivers/nvme/host/nvme.h | 1 +
drivers/nvme/host/rdma.c | 1 +
drivers/nvme/host/tcp.c | 1 +
5 files changed, 6 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 1f723b7e9efb..8376f65d52ca 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -969,6 +969,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/fc.c b/drivers/nvme/host/fc.c
index 86395b66310d..bcc086fcbe61 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -2551,6 +2551,7 @@ nvme_fc_timeout(struct request *rq, bool reserved)
"x%08x/x%08x\n",
ctrl->cnum, op->queue->qnum, sqe->common.opcode,
sqe->connect.fctype, sqe->common.cdw10, sqe->common.cdw11);
+ op->nreq.flags |= NVME_REQ_TIMEOUT;
if (__nvme_fc_abort_op(ctrl, op))
nvme_fc_error_recovery(ctrl, "io timeout abort failed");
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 88a6b97247f5..b0254692dc2a 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 b7ce4f221d99..634ed5f5f778 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1984,6 +1984,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 4c13c7110dbe..eee9470bb1bd 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2183,6 +2183,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] 13+ messages in thread* Re: [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd()
2021-01-27 10:33 [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd() Hannes Reinecke
` (4 preceding siblings ...)
2021-01-27 10:33 ` [PATCH 5/5] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd() Hannes Reinecke
@ 2021-01-27 15:09 ` Keith Busch
5 siblings, 0 replies; 13+ messages in thread
From: Keith Busch @ 2021-01-27 15:09 UTC (permalink / raw)
To: Hannes Reinecke
Cc: Keith Busch, Daniel Wagner, James Smart, linux-nvme,
Christoph Hellwig, Sagi Grimberg
On Wed, Jan 27, 2021 at 11:33:21AM +0100, Hannes Reinecke wrote:
> Hi all,
>
> here are some 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 we need to fix up nvme_cancel_request() to return an -EINTR
> on any pending sync commands during reset.
> And modify nvme-fc to return the same nvme status after timing out
> or cancelling requests.
> Plus, finally, adding 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().
Mostly looks good, but I'm not sure I understand the NVME_REQ_TIMEOUT
flag at the end. That appears to be serving the same purpose as the
"CANCELLED" flag, no?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 13+ messages in thread