[PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd()

[PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd()

[Hannes Reinecke]

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().

As usual, comments and reviews are welcome.

Changes to v1:
- Include reviews from Daniel
- Include changes for nvme-fc to return the same status as the
  other transports

Hannes Reinecke (5):
  nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()
  nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange()
  nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been
    aborted
  nvme: return NVME_SC_ABORT_QUEUE for cancelled commands
  nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()

 drivers/nvme/host/core.c |  5 ++++-
 drivers/nvme/host/fc.c   | 11 ++++++++---
 drivers/nvme/host/nvme.h |  1 +
 drivers/nvme/host/rdma.c |  1 +
 drivers/nvme/host/tcp.c  |  1 +
 5 files changed, 15 insertions(+), 4 deletions(-)

-- 
2.29.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

[PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()

[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

[PATCH 2/5] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange()

[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

[PATCH 3/5] nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been aborted

[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

[PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands

[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

[PATCH 5/5] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()

[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

Re: [PATCH 2/5] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange()

[Daniel Wagner]

On Wed, Jan 27, 2021 at 11:33:23AM +0100, Hannes Reinecke wrote:
> 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>

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 3/5] nvme-fc: return NVME_SC_HOST_ABORTED_CMD when a command has been aborted

[Daniel Wagner]

On Wed, Jan 27, 2021 at 11:33:24AM +0100, Hannes Reinecke wrote:
> 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>

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 5/5] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()

[Daniel Wagner]

On Wed, Jan 27, 2021 at 11:33:26AM +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>

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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH 4/5] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands

[Daniel Wagner]

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

Re: [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()

[Keith Busch]

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

Re: [PATCHv2 0/5][RESEND] Fixup return value for nvme_submit_sync_cmd()

[Keith Busch]

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

Re: [PATCH 1/5] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()

[Sagi Grimberg]

> 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