[PATCH v3 0/7] nvme controller reset and namespace scan work race conditions

[PATCH v3 0/7] nvme controller reset and namespace scan work race conditions

[Sagi Grimberg]

Hey Hannes and Keith,

This is the third attempt to handle the reset and scanning race saga.

The approach is to have the relevant admin commands return a proper
status code that reflects that we had a transport error and
not remove the namepsace if that is indeed the case.

This should be a reliable way to know if the revalidate_disk failed
due to a transport error or not.

I am able to reproduce this race with the following command (using
tcp/rdma):
for j in `seq 50`; do for i in `seq 50`; do nvme reset /dev/nvme0; done ; nvme disconnect-all; nvme connect-all; done

With this patch set (plus two more tcp/rdma transport specific patches
that address a other issues) I was able to pass the test without
reproducing the hang that you hannes reported.

Changes from v2:
- added fc patch from James (can you test please?)
- made nvme_identify_ns return id or PTR_ERR (Hannes)

Changes from v1:
- different approach

James Smart (1):
  nvme-fc: Fail transport errors with NVME_SC_HOST_PATH

Sagi Grimberg (6):
  nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR
  nvme: return nvme_error_status for sync commands failure
  nvme: make nvme_identify_ns propagate errors back
  nvme: make nvme_report_ns_ids propagate error back
  nvme-tcp: fail command with NVME_SC_HOST_PATH_ERROR send failed
  nvme: don't remove namespace if revalidate failed because of a
    transport error

 drivers/nvme/host/core.c | 49 +++++++++++++++++++++++++++-------------
 drivers/nvme/host/fc.c   | 37 ++++++++++++++++++++++++------
 drivers/nvme/host/tcp.c  |  2 +-
 3 files changed, 64 insertions(+), 24 deletions(-)

-- 
2.17.1

[PATCH v3 1/7] nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR

[Sagi Grimberg]

NVME_SC_ABORT_REQ means that the request was aborted due to
an abort command received. In our case, this is a transport
cancellation, so host pathing error is much more appropriate.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6956041224ec..d2b4f57fa67e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -294,7 +294,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
 	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
 				"Cancelling I/O %d", req->tag);
 
-	nvme_req(req)->status = NVME_SC_ABORT_REQ;
+	nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
 	blk_mq_complete_request_sync(req);
 	return true;
 }
-- 
2.17.1

[PATCH v3 2/7] nvme: return nvme_error_status for sync commands failure

[Sagi Grimberg]

callers should not rely on raw nvme status, blk_status_t
is more appropriate.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d2b4f57fa67e..f435c85c4062 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -803,7 +803,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		ret = -EINTR;
 	else
-		ret = nvme_req(req)->status;
+		ret = nvme_error_status(req);
  out:
 	blk_mq_free_request(req);
 	return ret;
@@ -894,7 +894,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
 	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
 		ret = -EINTR;
 	else
-		ret = nvme_req(req)->status;
+		ret = nvme_error_status(req);
 	if (result)
 		*result = le32_to_cpu(nvme_req(req)->result.u32);
 	if (meta && !ret && !write) {
-- 
2.17.1

[PATCH v3 3/7] nvme: make nvme_identify_ns propagate errors back

[Sagi Grimberg]

right now callers of nvme_identify_ns only know that it failed,
but don't know why. Make nvme_identify_ns propagate the error back.

Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index f435c85c4062..e503fd14de81 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1108,13 +1108,13 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
 
 	id = kmalloc(sizeof(*id), GFP_KERNEL);
 	if (!id)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
 	if (error) {
 		dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
 		kfree(id);
-		return NULL;
+		return ERR_PTR(error);
 	}
 
 	return id;
@@ -1748,8 +1748,8 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	}
 
 	id = nvme_identify_ns(ctrl, ns->head->ns_id);
-	if (!id)
-		return -ENODEV;
+	if (IS_ERR(id))
+		return PTR_ERR(id);
 
 	if (id->ncap == 0) {
 		ret = -ENODEV;
@@ -3337,8 +3337,8 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 	nvme_set_queue_limits(ctrl, ns->queue);
 
 	id = nvme_identify_ns(ctrl, nsid);
-	if (!id) {
-		ret = -EIO;
+	if (IS_ERR(id)) {
+		ret = PTR_ERR(id);
 		goto out_free_queue;
 	}
 
-- 
2.17.1

[PATCH v3 4/7] nvme: make nvme_report_ns_ids propagate error back

[Sagi Grimberg]

And make the callers check the return status and propagate
back accordingly.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e503fd14de81..49ffccd72091 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1603,9 +1603,11 @@ static void nvme_config_write_zeroes(struct gendisk *disk, struct nvme_ns *ns)
 	blk_queue_max_write_zeroes_sectors(disk->queue, max_sectors);
 }
 
-static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
+static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
 		struct nvme_id_ns *id, struct nvme_ns_ids *ids)
 {
+	int ret = 0;
+
 	memset(ids, 0, sizeof(*ids));
 
 	if (ctrl->vs >= NVME_VS(1, 1, 0))
@@ -1616,10 +1618,12 @@ static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
 		 /* Don't treat error as fatal we potentially
 		  * already have a NGUID or EUI-64
 		  */
-		if (nvme_identify_ns_descs(ctrl, nsid, ids))
+		ret = nvme_identify_ns_descs(ctrl, nsid, ids);
+		if (ret)
 			dev_warn(ctrl->device,
 				 "%s: Identify Descriptors failed\n", __func__);
 	}
+	return ret;
 }
 
 static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
@@ -1757,7 +1761,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)
 	}
 
 	__nvme_revalidate_disk(disk, id);
-	nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
+	ret = nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
+	if (ret)
+		goto out;
+
 	if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
 		dev_err(ctrl->device,
 			"identifiers changed for nsid %d\n", ns->head->ns_id);
@@ -3182,7 +3189,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
 	head->ns_id = nsid;
 	kref_init(&head->ref);
 
-	nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
+	ret = nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
+	if (ret)
+		goto out_cleanup_srcu;
 
 	ret = __nvme_check_ids(ctrl->subsys, head);
 	if (ret) {
@@ -3230,7 +3239,10 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
 	} else {
 		struct nvme_ns_ids ids;
 
-		nvme_report_ns_ids(ctrl, nsid, id, &ids);
+		ret = nvme_report_ns_ids(ctrl, nsid, id, &ids);
+		if (ret)
+			goto out_unlock;
+
 		if (!nvme_ns_ids_equal(&head->ids, &ids)) {
 			dev_err(ctrl->device,
 				"IDs don't match for shared namespace %d\n",
-- 
2.17.1

[PATCH v3 5/7] nvme-tcp: fail command with NVME_SC_HOST_PATH_ERROR send failed

[Sagi Grimberg]

This is a more appropriate error status for a transport error
detected by us (the host).

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/tcp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 2de33c4c5d14..7daf39f31b08 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -842,7 +842,7 @@ static inline void nvme_tcp_done_send_req(struct nvme_tcp_queue *queue)
 
 static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
 {
-	nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_DATA_XFER_ERROR);
+	nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_HOST_PATH_ERROR);
 }
 
 static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
-- 
2.17.1

[PATCH v3 6/7] nvme-fc: Fail transport errors with NVME_SC_HOST_PATH

[Sagi Grimberg]

From: James Smart <james.smart@broadcom.com>

NVME_SC_INTERNAL should indicate an internal controller errors
and not host transport errors. These errors will propagate to
upper layers (essentially nvme core) and be interpereted as
transport errors which should not be taken into account for
namespace state or condition.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/fc.c | 37 ++++++++++++++++++++++++++++++-------
 1 file changed, 30 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 3f3725d1eca1..c289f46f6d13 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1608,9 +1608,13 @@ 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_ABORT_REQ << 1);
-	else if (freq->status)
-		status = cpu_to_le16(NVME_SC_INTERNAL << 1);
+		status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 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",
+			ctrl->cnum, freq->status);
+	}
 
 	/*
 	 * For the linux implementation, if we have an unsuccesful
@@ -1637,8 +1641,13 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 		 * no payload in the CQE by the transport.
 		 */
 		if (freq->transferred_length !=
-			be32_to_cpu(op->cmd_iu.data_len)) {
-			status = cpu_to_le16(NVME_SC_INTERNAL << 1);
+		    be32_to_cpu(op->cmd_iu.data_len)) {
+			status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
+			dev_info(ctrl->ctrl.device,
+				"NVME-FC{%d}: io failed due to bad transfer "
+				"length: %d vs expected %d\n",
+				ctrl->cnum, freq->transferred_length,
+				be32_to_cpu(op->cmd_iu.data_len));
 			goto done;
 		}
 		result.u64 = 0;
@@ -1655,7 +1664,17 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 					freq->transferred_length ||
 			     op->rsp_iu.status_code ||
 			     sqe->common.command_id != cqe->command_id)) {
-			status = cpu_to_le16(NVME_SC_INTERNAL << 1);
+			status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
+			dev_info(ctrl->ctrl.device,
+				"NVME-FC{%d}: io failed due to bad NVMe_ERSP: "
+				"iu len %d, xfr len %d vs %d, status code "
+				"%d, cmdid %d vs %d\n",
+				ctrl->cnum, be16_to_cpu(op->rsp_iu.iu_len),
+				be32_to_cpu(op->rsp_iu.xfrd_len),
+				freq->transferred_length,
+				op->rsp_iu.status_code,
+				sqe->common.command_id,
+				cqe->command_id);
 			goto done;
 		}
 		result = cqe->result;
@@ -1663,7 +1682,11 @@ nvme_fc_fcpio_done(struct nvmefc_fcp_req *req)
 		break;
 
 	default:
-		status = cpu_to_le16(NVME_SC_INTERNAL << 1);
+		status = cpu_to_le16(NVME_SC_HOST_PATH_ERROR << 1);
+		dev_info(ctrl->ctrl.device,
+			"NVME-FC{%d}: io failed due to odd NVMe_xRSP iu "
+			"len %d\n",
+			ctrl->cnum, freq->rcv_rsplen);
 		goto done;
 	}
 
-- 
2.17.1

[PATCH v3 7/7] nvme: don't remove namespace if revalidate failed because of a transport error

[Sagi Grimberg]

If a controller reset is racing with a namespace revalidation, the
revalidation (admin) I/O will surely fail, but we should not remove the
namespace as we will execute the I/O when the controller is back up.

Fix this by checking the specific error code that revalidate_disk
returns, and if it is a transport related error, do not remove
the namespace as it will either recover when the controller is
back up and schedule a subsequent scan, or the controller is
going away and the namespaces will be removed anyways.

This fixes a hang namespace scanning racing with a controller reset and
also sporious I/O errors in path failover coditions where the
controller reset is racing with the namespace scan work with multipath
enabled.

Reported-by: Hannes Reinecke  <hare at suse.de>
Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 49ffccd72091..001bcc8a8d3e 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -228,6 +228,8 @@ static blk_status_t nvme_error_status(struct request *req)
 		return BLK_STS_PROTECTION;
 	case NVME_SC_RESERVATION_CONFLICT:
 		return BLK_STS_NEXUS;
+	case NVME_SC_HOST_PATH_ERROR:
+		return BLK_STS_TRANSPORT;
 	default:
 		return BLK_STS_IOERR;
 	}
@@ -3452,8 +3454,11 @@ static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid)
 
 	ns = nvme_find_get_ns(ctrl, nsid);
 	if (ns) {
-		if (ns->disk && revalidate_disk(ns->disk))
-			nvme_ns_remove(ns);
+		if (ns->disk) {
+			blk_status_t sts = revalidate_disk(ns->disk);
+			if (sts && sts != BLK_STS_TRANSPORT)
+				nvme_ns_remove(ns);
+		}
 		nvme_put_ns(ns);
 	} else
 		nvme_alloc_ns(ctrl, nsid);
-- 
2.17.1

[PATCH v3 3/7] nvme: make nvme_identify_ns propagate errors back

[Keith Busch]

On Thu, Aug 08, 2019@01:53:21PM -0700, Sagi Grimberg wrote:
> right now callers of nvme_identify_ns only know that it failed,
> but don't know why. Make nvme_identify_ns propagate the error back.
> 
> Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>  drivers/nvme/host/core.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f435c85c4062..e503fd14de81 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1108,13 +1108,13 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
>  
>  	id = kmalloc(sizeof(*id), GFP_KERNEL);
>  	if (!id)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
>  	if (error) {
>  		dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
>  		kfree(id);
> -		return NULL;
> +		return ERR_PTR(error);

The previous patch has nvme_submit_sync_cmd() return a BLK_STS_* value in
some cases, but ERR_PTR requires -errno's in order for IS_ERR to detect
errors. I think you need:

		if (error > 0)
			return ERR_PTR(blk_status_to_errno(error));
		else
			return ERR_PTR(error);

[PATCH v3 3/7] nvme: make nvme_identify_ns propagate errors back

[Sagi Grimberg]

>> right now callers of nvme_identify_ns only know that it failed,
>> but don't know why. Make nvme_identify_ns propagate the error back.
>>
>> Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
>> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
>> ---
>>   drivers/nvme/host/core.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index f435c85c4062..e503fd14de81 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -1108,13 +1108,13 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
>>   
>>   	id = kmalloc(sizeof(*id), GFP_KERNEL);
>>   	if (!id)
>> -		return NULL;
>> +		return ERR_PTR(-ENOMEM);
>>   
>>   	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
>>   	if (error) {
>>   		dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
>>   		kfree(id);
>> -		return NULL;
>> +		return ERR_PTR(error);
> 
> The previous patch has nvme_submit_sync_cmd() return a BLK_STS_* value in
> some cases, but ERR_PTR requires -errno's in order for IS_ERR to detect
> errors. I think you need:
> 
> 		if (error > 0)
> 			return ERR_PTR(blk_status_to_errno(error));
> 		else
> 			return ERR_PTR(error);
> 

You are right. sending a respin soon

[PATCH v3 0/7] nvme controller reset and namespace scan work race conditions

[Bart Van Assche]

On 8/8/19 1:53 PM, Sagi Grimberg wrote:
> for j in `seq 50`; do for i in `seq 50`; do nvme reset /dev/nvme0; done ; nvme disconnect-all; nvme connect-all; done

Has anyone already volunteered to convert this into a patch for the 
blktests project?

Thanks,

Bart.

[PATCH v3 0/7] nvme controller reset and namespace scan work race conditions

[Sagi Grimberg]

>> for j in `seq 50`; do for i in `seq 50`; do nvme reset /dev/nvme0; 
>> done ; nvme disconnect-all; nvme connect-all; done
> 
> Has anyone already volunteered to convert this into a patch for the 
> blktests project?

I'll see if it can reliably reproduce it for loop. Or perhaps I can
run this with tcp on 127.0.0.1?

[PATCH v3 0/7] nvme controller reset and namespace scan work race conditions

[Bart Van Assche]

On 8/8/19 4:33 PM, Sagi Grimberg wrote:
> 
>>> for j in `seq 50`; do for i in `seq 50`; do nvme reset /dev/nvme0; 
>>> done ; nvme disconnect-all; nvme connect-all; done
>>
>> Has anyone already volunteered to convert this into a patch for the 
>> blktests project?
> 
> I'll see if it can reliably reproduce it for loop. Or perhaps I can
> run this with tcp on 127.0.0.1?

There are more tests in blktests that use loopback over a local network 
interface, e.g. the SRP tests. You may want to have a look at 
get_ipv4_addr(), get_ipv6_addr() and other functions in 
common/multipath-over-rdma.

Bart.

[PATCH v3 1/7] nvme: fail cancelled commands with NVME_SC_HOST_PATH_ERROR

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> NVME_SC_ABORT_REQ means that the request was aborted due to
> an abort command received. In our case, this is a transport
> cancellation, so host pathing error is much more appropriate.
> 
> Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 6956041224ec..d2b4f57fa67e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -294,7 +294,7 @@ bool nvme_cancel_request(struct request *req, void *data, bool reserved)
>   	dev_dbg_ratelimited(((struct nvme_ctrl *) data)->device,
>   				"Cancelling I/O %d", req->tag);
>   
> -	nvme_req(req)->status = NVME_SC_ABORT_REQ;
> +	nvme_req(req)->status = NVME_SC_HOST_PATH_ERROR;
>   	blk_mq_complete_request_sync(req);
>   	return true;
>   }
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 2/7] nvme: return nvme_error_status for sync commands failure

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> callers should not rely on raw nvme status, blk_status_t
> is more appropriate.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index d2b4f57fa67e..f435c85c4062 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -803,7 +803,7 @@ int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
>   	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>   		ret = -EINTR;
>   	else
> -		ret = nvme_req(req)->status;
> +		ret = nvme_error_status(req);
>    out:
>   	blk_mq_free_request(req);
>   	return ret;
Please update the description for this function, too.

> @@ -894,7 +894,7 @@ static int nvme_submit_user_cmd(struct request_queue *q,
>   	if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
>   		ret = -EINTR;
>   	else
> -		ret = nvme_req(req)->status;
> +		ret = nvme_error_status(req);
>   	if (result)
>   		*result = le32_to_cpu(nvme_req(req)->result.u32);
>   	if (meta && !ret && !write) {
> 
But even with this change we'll be leaking blk_status_t error code to 
things like nvme_ioctl(). I'd rather have it mapped to posix error codes 
here by calling blk_status_to_errno().

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 3/7] nvme: make nvme_identify_ns propagate errors back

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> right now callers of nvme_identify_ns only know that it failed,
> but don't know why. Make nvme_identify_ns propagate the error back.
> 
> Reviewed-by: Minwoo Im <minwoo.im.dev at gmail.com>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index f435c85c4062..e503fd14de81 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1108,13 +1108,13 @@ static struct nvme_id_ns *nvme_identify_ns(struct nvme_ctrl *ctrl,
>   
>   	id = kmalloc(sizeof(*id), GFP_KERNEL);
>   	if (!id)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>   
>   	error = nvme_submit_sync_cmd(ctrl->admin_q, &c, id, sizeof(*id));
>   	if (error) {
>   		dev_warn(ctrl->device, "Identify namespace failed (%d)\n", error);
>   		kfree(id);
> -		return NULL;
> +		return ERR_PTR(error);
>   	}
>   
>   	return id;
> @@ -1748,8 +1748,8 @@ static int nvme_revalidate_disk(struct gendisk *disk)
>   	}
>   
>   	id = nvme_identify_ns(ctrl, ns->head->ns_id);
> -	if (!id)
> -		return -ENODEV;
> +	if (IS_ERR(id))
> +		return PTR_ERR(id);
>   
>   	if (id->ncap == 0) {
>   		ret = -ENODEV;
> @@ -3337,8 +3337,8 @@ static int nvme_alloc_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>   	nvme_set_queue_limits(ctrl, ns->queue);
>   
>   	id = nvme_identify_ns(ctrl, nsid);
> -	if (!id) {
> -		ret = -EIO;
> +	if (IS_ERR(id)) {
> +		ret = PTR_ERR(id);
>   		goto out_free_queue;
>   	}
>   
> 
See my comments to the previous patch. This will only work if we return 
POSIX error codes only; it'll probably fall flat on its face when 
returning blk_status_t codes.
Once we have converted nvme_submit_sync_cmd() to return POSIX error 
codes this is fine.

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 4/7] nvme: make nvme_report_ns_ids propagate error back

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> And make the callers check the return status and propagate
> back accordingly.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 22 +++++++++++++++++-----
>   1 file changed, 17 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e503fd14de81..49ffccd72091 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -1603,9 +1603,11 @@ static void nvme_config_write_zeroes(struct gendisk *disk, struct nvme_ns *ns)
>   	blk_queue_max_write_zeroes_sectors(disk->queue, max_sectors);
>   }
>   
> -static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
> +static int nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
>   		struct nvme_id_ns *id, struct nvme_ns_ids *ids)
>   {
> +	int ret = 0;
> +
>   	memset(ids, 0, sizeof(*ids));
>   
>   	if (ctrl->vs >= NVME_VS(1, 1, 0))
> @@ -1616,10 +1618,12 @@ static void nvme_report_ns_ids(struct nvme_ctrl *ctrl, unsigned int nsid,
>   		 /* Don't treat error as fatal we potentially
>   		  * already have a NGUID or EUI-64
>   		  */
> -		if (nvme_identify_ns_descs(ctrl, nsid, ids))
> +		ret = nvme_identify_ns_descs(ctrl, nsid, ids);
> +		if (ret)
>   			dev_warn(ctrl->device,
>   				 "%s: Identify Descriptors failed\n", __func__);
>   	}
> +	return ret;
>   }
>   
>   static bool nvme_ns_ids_valid(struct nvme_ns_ids *ids)
> @@ -1757,7 +1761,10 @@ static int nvme_revalidate_disk(struct gendisk *disk)
>   	}
>   
>   	__nvme_revalidate_disk(disk, id);
> -	nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
> +	ret = nvme_report_ns_ids(ctrl, ns->head->ns_id, id, &ids);
> +	if (ret)
> +		goto out;
> +
>   	if (!nvme_ns_ids_equal(&ns->head->ids, &ids)) {
>   		dev_err(ctrl->device,
>   			"identifiers changed for nsid %d\n", ns->head->ns_id);
> @@ -3182,7 +3189,9 @@ static struct nvme_ns_head *nvme_alloc_ns_head(struct nvme_ctrl *ctrl,
>   	head->ns_id = nsid;
>   	kref_init(&head->ref);
>   
> -	nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
> +	ret = nvme_report_ns_ids(ctrl, nsid, id, &head->ids);
> +	if (ret)
> +		goto out_cleanup_srcu;
>   
>   	ret = __nvme_check_ids(ctrl->subsys, head);
>   	if (ret) {
> @@ -3230,7 +3239,10 @@ static int nvme_init_ns_head(struct nvme_ns *ns, unsigned nsid,
>   	} else {
>   		struct nvme_ns_ids ids;
>   
> -		nvme_report_ns_ids(ctrl, nsid, id, &ids);
> +		ret = nvme_report_ns_ids(ctrl, nsid, id, &ids);
> +		if (ret)
> +			goto out_unlock;
> +
>   		if (!nvme_ns_ids_equal(&head->ids, &ids)) {
>   			dev_err(ctrl->device,
>   				"IDs don't match for shared namespace %d\n",
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 5/7] nvme-tcp: fail command with NVME_SC_HOST_PATH_ERROR send failed

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> This is a more appropriate error status for a transport error
> detected by us (the host).
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/tcp.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index 2de33c4c5d14..7daf39f31b08 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -842,7 +842,7 @@ static inline void nvme_tcp_done_send_req(struct nvme_tcp_queue *queue)
>   
>   static void nvme_tcp_fail_request(struct nvme_tcp_request *req)
>   {
> -	nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_DATA_XFER_ERROR);
> +	nvme_tcp_end_request(blk_mq_rq_from_pdu(req), NVME_SC_HOST_PATH_ERROR);
>   }
>   
>   static int nvme_tcp_try_send_data(struct nvme_tcp_request *req)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 6/7] nvme-fc: Fail transport errors with NVME_SC_HOST_PATH

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> From: James Smart <james.smart at broadcom.com>
> 
> NVME_SC_INTERNAL should indicate an internal controller errors
> and not host transport errors. These errors will propagate to
> upper layers (essentially nvme core) and be interpereted as
> transport errors which should not be taken into account for
> namespace state or condition.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/fc.c | 37 ++++++++++++++++++++++++++++++-------
>   1 file changed, 30 insertions(+), 7 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 7/7] nvme: don't remove namespace if revalidate failed because of a transport error

[Hannes Reinecke]

On 8/8/19 10:53 PM, Sagi Grimberg wrote:
> If a controller reset is racing with a namespace revalidation, the
> revalidation (admin) I/O will surely fail, but we should not remove the
> namespace as we will execute the I/O when the controller is back up.
> 
> Fix this by checking the specific error code that revalidate_disk
> returns, and if it is a transport related error, do not remove
> the namespace as it will either recover when the controller is
> back up and schedule a subsequent scan, or the controller is
> going away and the namespaces will be removed anyways.
> 
> This fixes a hang namespace scanning racing with a controller reset and
> also sporious I/O errors in path failover coditions where the
> controller reset is racing with the namespace scan work with multipath
> enabled.
> 
> Reported-by: Hannes Reinecke  <hare at suse.de>
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 49ffccd72091..001bcc8a8d3e 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -228,6 +228,8 @@ static blk_status_t nvme_error_status(struct request *req)
>   		return BLK_STS_PROTECTION;
>   	case NVME_SC_RESERVATION_CONFLICT:
>   		return BLK_STS_NEXUS;
> +	case NVME_SC_HOST_PATH_ERROR:
> +		return BLK_STS_TRANSPORT;
>   	default:
>   		return BLK_STS_IOERR;
>   	}
> @@ -3452,8 +3454,11 @@ static void nvme_validate_ns(struct nvme_ctrl *ctrl, unsigned nsid)
>   
>   	ns = nvme_find_get_ns(ctrl, nsid);
>   	if (ns) {
> -		if (ns->disk && revalidate_disk(ns->disk))
> -			nvme_ns_remove(ns);
> +		if (ns->disk) {
> +			blk_status_t sts = revalidate_disk(ns->disk);
> +			if (sts && sts != BLK_STS_TRANSPORT)
> +				nvme_ns_remove(ns);
> +		}
>   		nvme_put_ns(ns);
>   	} else
>   		nvme_alloc_ns(ctrl, nsid);
> 
This is a neat solution, yet I'm not convinced that it'll solve the 
problem entirely. I'll give it a spin.
But in any case, it's definitely a step in the right direction.
So:

Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare at suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG N?rnberg)

[PATCH v3 7/7] nvme: don't remove namespace if revalidate failed because of a transport error

[Sagi Grimberg]

> This is a neat solution, yet I'm not convinced that it'll solve the 
> problem entirely. I'll give it a spin.

Thanks Hannes,

Note that with tcp/rdma, there is another patch needed to pass
this test.

See:
[PATCH rfc] nvme: make all fabrics command run on a separate request queue

The reason is, that because we have admin commands making their way
to the target between we disable the controller and (re)enable it
as part of the reset process. Because of that, we have the target
failing some of the scan commands that are wrongly made it through
(which cause the scan work to wrongly remove the ns and hang).

So I would recommend running it with that patch as well (and also
get your feedback on it ;))