[PATCHv3 0/6] Fixup return value for nvme_submit_sync_cmd()

[PATCHv3 0/6] Fixup return value for nvme_submit_sync_cmd()

[Hannes Reinecke]

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 v2:
- Include reviews from Sagi
- Simplify error logic from nvme_validate_ns()

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

Hannes Reinecke (6):
  nvme: simplify error logic in nvme_validate_ns()
  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 | 11 +++++++----
 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, 18 insertions(+), 7 deletions(-)

-- 
2.29.2


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

[PATCH 1/6] nvme: simplify error logic in nvme_validate_ns()

[Hannes Reinecke]

We only should remove namespaces when we get fatal error back from
the device or when the namespace IDs have changed.
So instead of painfully masking out error numbers which might indicate
that the error should be ignored we could use an NVME status code
to indicated when the namespace should be removed.
That simplifies the final logic and makes it less error-prone.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d77f3f26d8d3..13ed7d3fbbb3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -4036,7 +4036,7 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
 static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 {
 	struct nvme_id_ns *id;
-	int ret = -ENODEV;
+	int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
 
 	if (test_bit(NVME_NS_DEAD, &ns->flags))
 		goto out;
@@ -4045,7 +4045,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 	if (ret)
 		goto out;
 
-	ret = -ENODEV;
+	ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
 	if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
 		dev_err(ns->ctrl->device,
 			"identifiers changed for nsid %d\n", ns->head->ns_id);
@@ -4063,7 +4063,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
 	 *
 	 * TODO: we should probably schedule a delayed retry here.
 	 */
-	if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
+	if (ret > 0 && (ret & NVME_SC_DNR))
 		nvme_ns_remove(ns);
 }
 
-- 
2.29.2


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

[PATCH 2/6] 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>
Reviewed-by: Keith Busch <kbusch@kernel.org>
---
 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 13ed7d3fbbb3..04dc85e0810c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -380,6 +380,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 3/6] 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>
Reviewed-by: Daniel Wagner <dwagner@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 4/6] 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>
Reviewed-by: Daniel Wagner <dwagner@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 5/6] 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 04dc85e0810c..624531a6b7c6 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -379,7 +379,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 6/6] 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 624531a6b7c6..5c1ec3bfb9d3 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1004,6 +1004,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 07b34175c6ce..02aa4ba665c0 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -173,6 +173,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 53ac4d7442ba..99ad2e10b295 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1990,6 +1990,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 69f59d2c5799..64aab742c84d 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2189,6 +2189,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 6/6] nvme: return -ETIMEDOUT in nvme_submit_sync_cmd()

[Keith Busch]

On Thu, Feb 25, 2021 at 11:55:27AM +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.

This driver has been using -EINTR for this condition since the
beginning, though. Is there a reason I'm not seeing why these use cases
can't use the existing command cancelled flag?
 
> 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 624531a6b7c6..5c1ec3bfb9d3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1004,6 +1004,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 07b34175c6ce..02aa4ba665c0 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -173,6 +173,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 53ac4d7442ba..99ad2e10b295 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1990,6 +1990,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 69f59d2c5799..64aab742c84d 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2189,6 +2189,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

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

Re: [PATCH 1/6] nvme: simplify error logic in nvme_validate_ns()

[Keith Busch]

On Thu, Feb 25, 2021 at 11:55:22AM +0100, Hannes Reinecke wrote:
> We only should remove namespaces when we get fatal error back from
> the device or when the namespace IDs have changed.
> So instead of painfully masking out error numbers which might indicate
> that the error should be ignored we could use an NVME status code
> to indicated when the namespace should be removed.
> That simplifies the final logic and makes it less error-prone.

I think this looks good to me. The timeout condition should retain
discovered namespaces since the controller is going to be restarted.

Reviewed-by: Keith Busch <kbusch@kernel.org>
 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/nvme/host/core.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index d77f3f26d8d3..13ed7d3fbbb3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4036,7 +4036,7 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
>  static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>  {
>  	struct nvme_id_ns *id;
> -	int ret = -ENODEV;
> +	int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>  
>  	if (test_bit(NVME_NS_DEAD, &ns->flags))
>  		goto out;
> @@ -4045,7 +4045,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>  	if (ret)
>  		goto out;
>  
> -	ret = -ENODEV;
> +	ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>  	if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
>  		dev_err(ns->ctrl->device,
>  			"identifiers changed for nsid %d\n", ns->head->ns_id);
> @@ -4063,7 +4063,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>  	 *
>  	 * TODO: we should probably schedule a delayed retry here.
>  	 */
> -	if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
> +	if (ret > 0 && (ret & NVME_SC_DNR))
>  		nvme_ns_remove(ns);
>  }
>  
> -- 
> 2.29.2

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

Re: [PATCH 3/6] nvme-fc: set NVME_REQ_CANCELLED in nvme_fc_terminate_exchange()

[Keith Busch]

On Thu, Feb 25, 2021 at 11:55:24AM +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.

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: [PATCH 5/6] nvme: return NVME_SC_ABORT_QUEUE for cancelled commands

[Keith Busch]

On Thu, Feb 25, 2021 at 11:55:26AM +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'm not sure if this makes sense. The controller didn't return a status
in this case, so the host is reclaiming the command absent a status from
the controller. Is there a particular situation where this patch
matters?
 
> 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 04dc85e0810c..624531a6b7c6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -379,7 +379,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

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

[Hannes Reinecke]

On 2/25/21 5:31 PM, Keith Busch wrote:
> On Thu, Feb 25, 2021 at 11:55:27AM +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.
> 
> This driver has been using -EINTR for this condition since the
> beginning, though. Is there a reason I'm not seeing why these use cases
> can't use the existing command cancelled flag?
>   
Technically we have two distinct conditions, a command being aborted due 
to a timeout, and a command being aborted due to a queue disconnect.
Admittedly, the second conditions is the common case, as the queue 
disconnect is typically done due to the error recovery kicking in, which 
in itself is in reaction to a KATO timeout firing.
The first condition is only hit if we run into a timeout for the initial 
commands _before_ KATO is started.

And I thought it might be good to have these two cases differentiated.
But I'd be fine with dropping it if that's not needed.

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

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

[Hannes Reinecke]

On 2/25/21 5:43 PM, Keith Busch wrote:
> On Thu, Feb 25, 2021 at 11:55:26AM +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'm not sure if this makes sense. The controller didn't return a status
> in this case, so the host is reclaiming the command absent a status from
> the controller. Is there a particular situation where this patch
> matters?
>   
Not really. You are right, I guess I'll drop it.

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

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

[Chao Leng]

On 2021/2/25 18:55, 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.
> 
> 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 04dc85e0810c..624531a6b7c6 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -379,7 +379,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;
Use NVME_SC_ABORT_QUEUE instead of NVME_SC_HOST_ABORTED_CMD is not a good idea.
NVME_SC_HOST_ABORTED_CMD is a host path related error, Now used to check if need
fail over retry by multipath.
>   	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",
> 

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

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

[Chao Leng]

Looks good to me.
Maybe it is better to split multiple patches for different transports.


On 2021/2/25 18:55, 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/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 624531a6b7c6..5c1ec3bfb9d3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1004,6 +1004,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 07b34175c6ce..02aa4ba665c0 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -173,6 +173,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 53ac4d7442ba..99ad2e10b295 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1990,6 +1990,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 69f59d2c5799..64aab742c84d 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -2189,6 +2189,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);
>   	}
>   }
> 

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

Re: [PATCH 1/6] nvme: simplify error logic in nvme_validate_ns()

[Chao Leng]

On 2021/2/25 18:55, Hannes Reinecke wrote:
> We only should remove namespaces when we get fatal error back from
> the device or when the namespace IDs have changed.
> So instead of painfully masking out error numbers which might indicate
> that the error should be ignored we could use an NVME status code
> to indicated when the namespace should be removed.
> That simplifies the final logic and makes it less error-prone.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/nvme/host/core.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index d77f3f26d8d3..13ed7d3fbbb3 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -4036,7 +4036,7 @@ static void nvme_ns_remove_by_nsid(struct nvme_ctrl *ctrl, u32 nsid)
>   static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>   {
>   	struct nvme_id_ns *id;
> -	int ret = -ENODEV;
> +	int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>   
>   	if (test_bit(NVME_NS_DEAD, &ns->flags))
>   		goto out;
> @@ -4045,7 +4045,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>   	if (ret)
>   		goto out;
>   
> -	ret = -ENODEV;
> +	ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>   	if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
>   		dev_err(ns->ctrl->device,
>   			"identifiers changed for nsid %d\n", ns->head->ns_id);
> @@ -4063,7 +4063,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids *ids)
>   	 *
>   	 * TODO: we should probably schedule a delayed retry here.
>   	 */
> -	if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
> +	if (ret > 0 && (ret & NVME_SC_DNR))
nvme_identify_ns may return -NODEV.
if ret == -NODEV, also need to remove namespaces.
>   		nvme_ns_remove(ns);
>   }
>   
> 

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

Re: [PATCH 2/6] nvme: add NVME_REQ_CANCELLED flag in nvme_cancel_request()

[Chao Leng]

Reviewed-by: Chao Leng <lengchao@huawei.com>

On 2021/2/25 18:55, 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>
> Reviewed-by: Keith Busch <kbusch@kernel.org>
> ---
>   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 13ed7d3fbbb3..04dc85e0810c 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -380,6 +380,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;
>   }
> 

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

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

[Hannes Reinecke]

On 2/26/21 1:59 AM, Chao Leng wrote:
> 
> 
> On 2021/2/25 18:55, 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.
>>
>> 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 04dc85e0810c..624531a6b7c6 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -379,7 +379,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;
> Use NVME_SC_ABORT_QUEUE instead of NVME_SC_HOST_ABORTED_CMD is not a 
> good idea.
> NVME_SC_HOST_ABORTED_CMD is a host path related error, Now used to check 
> if need fail over retry by multipath.

You are correct; this patch will be dropped for the next iteration.

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

Re: [PATCH 1/6] nvme: simplify error logic in nvme_validate_ns()

[Hannes Reinecke]

On 2/26/21 2:13 AM, Chao Leng wrote:
> 
> 
> On 2021/2/25 18:55, Hannes Reinecke wrote:
>> We only should remove namespaces when we get fatal error back from
>> the device or when the namespace IDs have changed.
>> So instead of painfully masking out error numbers which might indicate
>> that the error should be ignored we could use an NVME status code
>> to indicated when the namespace should be removed.
>> That simplifies the final logic and makes it less error-prone.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/nvme/host/core.c | 6 +++---
>>   1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index d77f3f26d8d3..13ed7d3fbbb3 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -4036,7 +4036,7 @@ static void nvme_ns_remove_by_nsid(struct 
>> nvme_ctrl *ctrl, u32 nsid)
>>   static void nvme_validate_ns(struct nvme_ns *ns, struct nvme_ns_ids 
>> *ids)
>>   {
>>       struct nvme_id_ns *id;
>> -    int ret = -ENODEV;
>> +    int ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>>       if (test_bit(NVME_NS_DEAD, &ns->flags))
>>           goto out;
>> @@ -4045,7 +4045,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, 
>> struct nvme_ns_ids *ids)
>>       if (ret)
>>           goto out;
>> -    ret = -ENODEV;
>> +    ret = NVME_SC_INVALID_NS | NVME_SC_DNR;
>>       if (!nvme_ns_ids_equal(&ns->head->ids, ids)) {
>>           dev_err(ns->ctrl->device,
>>               "identifiers changed for nsid %d\n", ns->head->ns_id);
>> @@ -4063,7 +4063,7 @@ static void nvme_validate_ns(struct nvme_ns *ns, 
>> struct nvme_ns_ids *ids)
>>        *
>>        * TODO: we should probably schedule a delayed retry here.
>>        */
>> -    if (ret && ret != -ENOMEM && !(ret > 0 && !(ret & NVME_SC_DNR)))
>> +    if (ret > 0 && (ret & NVME_SC_DNR))
> nvme_identify_ns may return -NODEV.
> if ret == -NODEV, also need to remove namespaces.

You are correct. Will be updating the patch.

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