Six NVMeOF-related patches

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

Adjust indentation such that arguments are aligned.

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---
 drivers/nvme/host/rdma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/nvme/host/rdma.c +1 -1

index 5a83881..9612ea0 100644
@@ -568,14 +568,16 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 			NVME_RDMA_CONNECT_TIMEOUT_MS);
 	if (ret) {
 		dev_info(ctrl->ctrl.device,
-			"rdma_resolve_addr failed (%d).\n", ret);
+			 "rdma_resolve_addr(%pISpc) failed (%d).\n",

Quoted 28 lines · Bart Van Assche

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---
?drivers/nvme/host/rdma.c | 6 ++++--
?1 file changed, 4 insertions(+), 2 deletions(-)

drivers/nvme/host/rdma.c +2 -1

index 5a83881..9612ea0 100644
@@ -568,14 +568,16 @@ static int nvme_rdma_init_queue(struct
 nvme_rdma_ctrl *ctrl,
 ?			NVME_RDMA_CONNECT_TIMEOUT_MS);
 ?	if (ret) {
 ?		dev_info(ctrl->ctrl.device,
-			"rdma_resolve_addr failed (%d).\n", ret);
+			?"rdma_resolve_addr(%pISpc) failed
 (%d).\n",
+			?&ctrl->addr, ret);
 ?		goto out_destroy_cm_id;
 ?	}

?
? ret = nvme_rdma_wait_for_cm(queue);
? if (ret) {
? dev_info(ctrl->ctrl.device,

drivers/nvme/host/rdma.c +0 -1

-			"rdma_resolve_addr wait failed (%d).\n",

ret);

drivers/nvme/host/rdma.c +1 -0

+			?"rdma_resolve_addr(%pISpc) wait failed

(%d).\n",

drivers/nvme/host/rdma.c

Quoted 1 line

drivers/nvme/host/rdma.c +1 -0

+			?&ctrl->addr, ret);

Can you skip the indentation change???Also once have the address how about:

"Failed to resolve %pISpc (instant), error %d).\n"

"Failed to resolve %pISpc (wait), error %d.\n"

Quoted 15 lines

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---
 drivers/nvme/host/rdma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/nvme/host/rdma.c +1 -1

index 5a83881..9612ea0 100644
@@ -568,14 +568,16 @@ static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl,
 			NVME_RDMA_CONNECT_TIMEOUT_MS);
 	if (ret) {
 		dev_info(ctrl->ctrl.device,
-			"rdma_resolve_addr failed (%d).\n", ret);
+			 "rdma_resolve_addr(%pISpc) failed (%d).\n",

drivers/nvme/host/rdma.c static int nvme_rdma_init_queue(struct nvme_rdma_ctrl *ctrl, · line 580

Quoted 10 lines

drivers/nvme/host/rdma.c +3 -1

+			 &ctrl->addr, ret);
 		goto out_destroy_cm_id;
 	}
 
 	ret = nvme_rdma_wait_for_cm(queue);
 	if (ret) {
 		dev_info(ctrl->ctrl.device,
-			"rdma_resolve_addr wait failed (%d).\n", ret);
+			 "rdma_resolve_addr(%pISpc) wait failed (%d).\n",
+			 &ctrl->addr, ret);

Can you skip the indentation change? Also once have the address how about:

"Failed to resolve %pISpc (instant), error %d).\n"

"Failed to resolve %pISpc (wait), error %d.\n"

While I was figuring out how to make the nvme-rdma driver log in to the nvmet-rdma driver, the following message appeared in the system log:

nvme nvme0: Connect rejected, status 0.

That message is not very helpful. Hence this patch that makes the messages reported by nvme_rdma_conn_rejected() more informative.

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---

While I was figuring out how to make the nvme-rdma driver log in to the nvmet-rdma driver, the following message appeared in the system log:

nvme nvme0: Connect rejected, status 0.

That message is not very helpful. Hence this patch that makes the messages reported by nvme_rdma_conn_rejected() more informative.

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---
 drivers/nvme/host/rdma.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

drivers/nvme/host/rdma.c +0 -0

index 9612ea0..df7f599 100644

drivers/nvme/host/rdma.c static int nvme_rdma_conn_established(struct nvme_rdma_queue *queue) · line 1221

Quoted 11 lines

{

drivers/nvme/host/rdma.c +9 -0

+	char reason[32];
 
+	switch (ev->status) {
+	case IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID:
+		strlcpy(reason, "duplicate local comm id", sizeof(reason));
+		break;
+	case IB_CM_REJ_CONSUMER_DEFINED:
+		if (ev->param.conn.private_data_len) {
+			const struct nvme_rdma_cm_rej *rej =
+				(const void *)ev->param.conn.private_data;

What's the point of this void pointer cast?

drivers/nvme/host/rdma.c +5 -0

+	switch (ev->status) {
+	case IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID:
+		strlcpy(reason, "duplicate local comm id", sizeof(reason));
+		break;
+	case IB_CM_REJ_CONSUMER_DEFINED:

While I was figuring out how to make the nvme-rdma driver log in to the nvmet-rdma driver, the following message appeared in the system log:

nvme nvme0: Connect rejected, status 0.

That message is not very helpful. Hence this patch that makes the messages reported by nvme_rdma_conn_rejected() more informative.

Signed-off-by Bart Van Assche <bart.vanassche at sandisk.com>

---
 drivers/nvme/host/rdma.c | 39 +++++++++++++++++++++++++++++----------
 1 file changed, 29 insertions(+), 10 deletions(-)

drivers/nvme/host/rdma.c +17 -10

index 9612ea0..df7f599 100644
@@ -1207,20 +1207,39 @@ static int nvme_rdma_conn_established(struct
 nvme_rdma_queue *queue)
 }
 
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
-		struct rdma_cm_event *ev)
+				   const struct rdma_cm_event *ev)
 {
-	if (ev->param.conn.private_data_len) {
-		struct nvme_rdma_cm_rej *rej =
-			(struct nvme_rdma_cm_rej *)ev-
 >param.conn.private_data;
+	char reason[32];
 
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, status %d.", le16_to_cpu(rej->sts));
-		/* XXX: Think of something clever to do here... */
-	} else {
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, no private data.\n");
+	switch (ev->status) {
+	case IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID:
+		strlcpy(reason, "duplicate local comm id", sizeof(reason));
+		break;
+	case IB_CM_REJ_CONSUMER_DEFINED:
+		if (ev->param.conn.private_data_len) {
+			const struct nvme_rdma_cm_rej *rej =
+				(const void *)ev->param.conn.private_data;
+
+			snprintf(reason, sizeof(reason), "status %d",
+				 le16_to_cpu(rej->sts));
+			/* XXX: Think of something clever to do here... */
+		} else {
+			strlcpy(reason, "no private data", sizeof(reason));
+		}

iWARP RDMA_CM_EVENT_REJECTED events have a -errno as the event status. Perhaps you could at the very least dump the error as an integer here? But an errno string would be ideal. :)

Quoted 1 line

iWARP RDMA_CM_EVENT_REJECTED events have a -errno as the event status.

Perhaps

Quoted 2 lines

you could at the very least dump the error as an integer here? But an errno string would be ideal. :)

Hello Steve,

Is something like the patch below perhaps what you had in mind? While I was preparing this patch I was disappointed to see that another implementation is needed for IB/RoCE transports compared to iWARP transports.

Thanks,

Bart.

[PATCH] nvme/rdma: Provide more information about rejected connection attempts

While I was figuring out how to make the nvme-rdma driver log in to the nvmet-rdma driver over an IB network, the following message appeared in the system log:

nvme nvme0: Connect rejected, status 0.

That message is not very helpful. Hence this patch that makes the messages reported by nvme_rdma_conn_rejected() more informative.

---
 drivers/nvme/host/rdma.c | 79
++++++++++++++++++++++++++++++++++++++++++------
 1 file changed, 69 insertions(+), 10 deletions(-)

drivers/nvme/host/rdma.c +67 -9

index 8a80a04..c1b9fd6 100644
@@ -1206,24 +1206,82 @@ static int nvme_rdma_conn_established(struct
 nvme_rdma_queue *queue)
 	return ret;
 }
 
-static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
-		struct rdma_cm_event *ev)
+static int nvme_rdma_ib_conn_rejected(struct nvme_rdma_queue *queue,
+		const struct rdma_cm_event *ev)
+{
+	char reason[32];
+
+	switch (ev->status) {
+	case IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID:
+		strlcpy(reason, "duplicate local comm id", sizeof(reason));
+		break;
+	case IB_CM_REJ_CONSUMER_DEFINED:
+		if (ev->param.conn.private_data_len) {
+			const struct nvme_rdma_cm_rej *rej =
+				ev->param.conn.private_data;
+
+			snprintf(reason, sizeof(reason),
+				 "NVMe RDMA CM status %d",
+				 le16_to_cpu(rej->sts));
+			/* XXX: Think of something clever to do here... */
+		} else {
+			strlcpy(reason, "no private data", sizeof(reason));
+		}
+		break;
+	case IB_CM_REJ_INVALID_SERVICE_ID:
+		strlcpy(reason, "invalid service ID", sizeof(reason));
+		break;
+	case IB_CM_REJ_STALE_CONN:
+		strlcpy(reason, "stale connection", sizeof(reason));
+		break;
+	default:
+		snprintf(reason, sizeof(reason), "REJ reason %#x", ev->status);
+		break;
+	}
+
+	dev_err(queue->ctrl->ctrl.device, "Connect rejected, %s.\n", reason);
+
+	return -ECONNRESET;
+}
+
+static int nvme_rdma_iw_conn_rejected(struct nvme_rdma_queue *queue,
+		const struct rdma_cm_event *ev)
 {
+	char reason[32];
+
 	if (ev->param.conn.private_data_len) {
-		struct nvme_rdma_cm_rej *rej =
-			(struct nvme_rdma_cm_rej *)ev-
 >param.conn.private_data;
+		const struct nvme_rdma_cm_rej *rej =
+			ev->param.conn.private_data;
 
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, status %d.", le16_to_cpu(rej->sts));
-		/* XXX: Think of something clever to do here... */
+		snprintf(reason, sizeof(reason),
+			 "NVMe RDMA CM status %d",
+			 le16_to_cpu(rej->sts));
+			/* XXX: Think of something clever to do here... */
 	} else {
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, no private data.\n");
+		strlcpy(reason, "no private data", sizeof(reason));
 	}
 
+	dev_err(queue->ctrl->ctrl.device, "Connect rejected, errno %d, %s.\n",
+		ev->status, reason);
+
 	return -ECONNRESET;
 }
 
+static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
+		enum rdma_transport_type tt,
+		const struct rdma_cm_event *ev)
+{
+	switch (tt) {
+	case RDMA_TRANSPORT_IB:
+		return nvme_rdma_ib_conn_rejected(queue, ev);
+	case RDMA_TRANSPORT_IWARP:
+		return nvme_rdma_iw_conn_rejected(queue, ev);
+	default:
+		WARN_ON_ONCE(true);
+		return -ECONNRESET;
+	}
+}
+
 static int nvme_rdma_addr_resolved(struct nvme_rdma_queue *queue)

{

drivers/nvme/host/rdma.c +2 -1

 	struct nvme_rdma_device *dev;

@@ -1331,7 +1389,8 @@ static int nvme_rdma_cm_handler(struct rdma_cm_id
 *cm_id,
 		complete(&queue->cm_done);
 		return 0;
 	case RDMA_CM_EVENT_REJECTED:
-		cm_error = nvme_rdma_conn_rejected(queue, ev);
+		cm_error = nvme_rdma_conn_rejected(queue,
+				cm_id->route.addr.dev_addr.transport, ev);
 		break;
 	case RDMA_CM_EVENT_ADDR_ERROR:

case RDMA_CM_EVENT_ROUTE_ERROR:
--
2.10.1

I agree. What if we add a helper function in the core to map the event->status value to something human readable? That would at
least push this into the core. The nvme host really doesn't do
anything other than display a different message...

Something like rdma_event_msg(), but using event->status.

Quoted 6 lines

I agree. What if we add a helper function in the core to map the event->status value to something human readable? That would at
least push this into the core. The nvme host really doesn't do
anything other than display a different message...

Something like rdma_event_msg(), but using event->status.

Hello Steve,

It won't be possible to let rdma_event_msg() decode the NVME_RDMA_CM_* status unless a callback function that performs such decoding is passed to rdma_event_msg(). Since such an approach would work for translating a status into a message but not for any more advanced CM status handling my preference is to unify the calling conventions for IB/RoCE and iWARP CM reject callbacks.

Quoted 9 lines · Steve Wise

Quoted 6 lines

I agree. What if we add a helper function in the core to map the event->status value to something human readable? That would at
least push this into the core. The nvme host really doesn't do
anything other than display a different message...

Something like rdma_event_msg(), but using event->status.

Quoted 8 lines

Hello Steve,

It won't be possible to let rdma_event_msg() decode the NVME_RDMA_CM_* status unless a callback function that performs such decoding is passed to rdma_event_msg(). Since such an approach would work for translating a status into a message but not for any more advanced CM status handling my preference is to unify the calling conventions for IB/RoCE and iWARP CM reject callbacks.

I think for this particular case, mapping event->status to a string is all that nvme needs. And having a status to string mapping would be easy to do in the core. I agree though, that unifying the status codes is a "good thing".

-----Original Message-----
From: Steve Wise [mailto:swise at opengridcomputing.com]
Sent: Thursday, October 20, 2016 2:51 PM
To: 'Bart Van Assche'; 'Keith Busch'
Cc: 'Jens Axboe'; 'Christoph Hellwig'; linux-nvme at lists.infradead.org; 'Sagi Grimberg'
Subject: RE: [PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

Quoted 18 lines

Quoted 17 lines · Steve Wise

Quoted 6 lines

I agree. What if we add a helper function in the core to map the event->status value to something human readable? That would at
least push this into the core. The nvme host really doesn't do
anything other than display a different message...

Something like rdma_event_msg(), but using event->status.

Hello Steve,

It won't be possible to let rdma_event_msg() decode the NVME_RDMA_CM_* status unless a callback function that performs such decoding is passed to rdma_event_msg(). Since such an approach would work for translating a status into a message but not for any more advanced CM status handling my preference is to unify the calling conventions for IB/RoCE and iWARP CM reject callbacks.

Quoted 5 lines

I think for this particular case, mapping event->status to a string is all that nvme needs. And having a status to string mapping would be easy to do
in the core. I agree though, that unifying the status codes is a "good thing".

What about something along these lines? (untested and mangled by my emailer, but you get the point)

And here is a proposed change to nvme_rdma to use rdma_reject_msg(). If you like this idea, I'll send out these two patches, or send them to you to include in your series.

drivers/nvme/host/rdma.c +2 -5

index 601aecf..41a2d4c 100644
@@ -1237,18 +1237,19 @@ out_destroy_queue_ib:
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
                struct rdma_cm_event *ev)
 {
+       short nvme_status = -1;
+
        if (ev->param.conn.private_data_len) {
                struct nvme_rdma_cm_rej *rej =
                        (struct nvme_rdma_cm_rej *)ev->param.conn.private_data;
 
-               dev_err(queue->ctrl->ctrl.device,
-                       "Connect rejected, status %d.", le16_to_cpu(rej->sts));
-               /* XXX: Think of something clever to do here... */
-       } else {
-               dev_err(queue->ctrl->ctrl.device,

Hello Steve,

Had you already realized that the description of my patch 6/6 shows that it is not guaranteed on an IB setup that ev->param.conn.private_data_len == 0 for other cases than IB_CM_REJ_CONSUMER_DEFINED?

Quoted 5 lines

Hello Steve,

Had you already realized that the description of my patch 6/6 shows that it is not guaranteed on an IB setup that ev->param.conn.private_data_len == 0 for other cases than IB_CM_REJ_CONSUMER_DEFINED?

Sorry, I missed that. :(

Hello Keith,

This patch series is what I came up with while I was using the NVMeOF host and target drivers to test my pending block layer and dm-mq patches. It would be appreciated if you would consider these patches for inclusion in the upstream kernel. The individual patches in this series are:

Hello Keith,

This patch series is what I came up with while I was using the NVMeOF host and target drivers to test my pending block layer and dm-mq patches. It would be appreciated if you would consider these patches for inclusion in the upstream kernel. The individual patches in this series are:

0001-nvme-scsi-Remove-set-but-not-used-variables.patch 0002-nvme-fabrics-Adjust-source-code-indentation.patch 0003-nvme-fabrics-Fix-memory-leaks-in-nvmf_parse_options.patch 0004-nvme-fabrics-Fix-a-memory-leak-in-an-nvmf_create_ctr.patch 0005-nvme-fabrics-Print-network-address-if-address-resolu.patch 0006-nvme-rdma-Make-nvme_rdma_conn_rejected-more-informat.patch

up until now, nvme over fabrics related changes went through our tree (and to jens in turn) at:
http://git.infradead.org/nvme-fabrics.git

Sorry but that's something I wasn't aware of. In the MAINTAINERS file I found Keith and Jens as maintainers for the code under drivers/nvme/host/. Does this mean that a new entry should be added for drivers/nvme/host/fabrics.[ch]?

Maybe. Or we should just add Sagi and me to the general NVMe antry and replace the Fabrics git tree with a general NVMe one.

Keith, Jens - any preferences?

Quoted 5 lines

Maybe. Or we should just add Sagi and me to the general NVMe antry and replace the Fabrics git tree with a general NVMe one.

Keith, Jens - any preferences?

I've no preference. I assumed, though, fabrics developers/users found the public staging tree useful since it seems to be changing faster than the rest of the driver.

We don't have an nvme specific tree, so
if everyone's okay going through the general block, then let's add the maintainer entries.

Hey Bart,

Queued patches 1-4 to nvmf-4.9-rc branch

I'll wait for 5/6 and Steve has generalized 6/6 to RDMA core so I guess it will go out of Doug's tree.

Steve, your set is probably a 4.10 material, can you please verify it applies cleanly on nvmf-4.9-rc (or give Doug a heads-up if not)

Thanks!