[PATCH 0/6] Six NVMeOF-related patches

[PATCH 0/6] Six NVMeOF-related patches

[Bart Van Assche]

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

Thanks,

Bart.

[PATCH 1/6] nvme/scsi: Remove set-but-not-used variables

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
---
 drivers/nvme/host/scsi.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/scsi.c b/drivers/nvme/host/scsi.c
index c2a0a1c7..0671fe0 100644
--- a/drivers/nvme/host/scsi.c
+++ b/drivers/nvme/host/scsi.c
@@ -1280,10 +1280,6 @@ static inline void nvme_trans_modesel_get_bd_len(u8 *parm_list, u8 cdb10,
 static void nvme_trans_modesel_save_bd(struct nvme_ns *ns, u8 *parm_list,
 					u16 idx, u16 bd_len, u8 llbaa)
 {
-	u16 bd_num;
-
-	bd_num = bd_len / ((llbaa == 0) ?
-			SHORT_DESC_BLOCK : LONG_DESC_BLOCK);
 	/* Store block descriptor info if a FORMAT UNIT comes later */
 	/* TODO Saving 1st BD info; what to do if multiple BD received? */
 	if (llbaa == 0) {
@@ -1528,7 +1524,7 @@ static int nvme_trans_fmt_send_cmd(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	int nvme_sc;
 	struct nvme_id_ns *id_ns;
 	u8 i;
-	u8 flbas, nlbaf;
+	u8 nlbaf;
 	u8 selected_lbaf = 0xFF;
 	u32 cdw10 = 0;
 	struct nvme_command c;
@@ -1539,7 +1535,6 @@ static int nvme_trans_fmt_send_cmd(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 	if (res)
 		return res;
 
-	flbas = (id_ns->flbas) & 0x0F;
 	nlbaf = id_ns->nlbaf;
 
 	for (i = 0; i < nlbaf; i++) {
@@ -2168,12 +2163,10 @@ static int nvme_trans_synchronize_cache(struct nvme_ns *ns,
 static int nvme_trans_start_stop(struct nvme_ns *ns, struct sg_io_hdr *hdr,
 							u8 *cmd)
 {
-	u8 immed, pcmod, no_flush, start;
+	u8 immed, no_flush;
 
 	immed = cmd[1] & 0x01;
-	pcmod = cmd[3] & 0x0f;
 	no_flush = cmd[4] & 0x04;
-	start = cmd[4] & 0x01;
 
 	if (immed != 0) {
 		return nvme_trans_completion(hdr, SAM_STAT_CHECK_CONDITION,
-- 
2.10.1

[PATCH 1/6] nvme/scsi: Remove set-but-not-used variables

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:10:03PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/6] nvme/scsi: Remove set-but-not-used variables

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 2/6] nvme-fabrics: Adjust source code indentation

[Bart Van Assche]

Adjust indentation such that arguments are aligned.

Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
---
 drivers/nvme/host/fabrics.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 5a3f008..716d9e2 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -576,7 +576,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			nqnlen = strlen(opts->subsysnqn);
 			if (nqnlen >= NVMF_NQN_SIZE) {
 				pr_err("%s needs to be < %d bytes\n",
-				opts->subsysnqn, NVMF_NQN_SIZE);
+				       opts->subsysnqn, NVMF_NQN_SIZE);
 				ret = -EINVAL;
 				goto out;
 			}
@@ -665,7 +665,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			nqnlen = strlen(p);
 			if (nqnlen >= NVMF_NQN_SIZE) {
 				pr_err("%s needs to be < %d bytes\n",
-					p, NVMF_NQN_SIZE);
+				       p, NVMF_NQN_SIZE);
 				ret = -EINVAL;
 				goto out;
 			}
-- 
2.10.1

[PATCH 2/6] nvme-fabrics: Adjust source code indentation

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:10:24PM -0700, Bart Van Assche wrote:
> Adjust indentation such that arguments are aligned.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 2/6] nvme-fabrics: Adjust source code indentation

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 3/6] nvme-fabrics: Fix memory leaks in nvmf_parse_options()

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
---
 drivers/nvme/host/fabrics.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 716d9e2..3e37efa 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -666,10 +666,12 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			if (nqnlen >= NVMF_NQN_SIZE) {
 				pr_err("%s needs to be < %d bytes\n",
 				       p, NVMF_NQN_SIZE);
+				kfree(p);
 				ret = -EINVAL;
 				goto out;
 			}
 			opts->host = nvmf_host_add(p);
+			kfree(p);
 			if (!opts->host) {
 				ret = -ENOMEM;
 				goto out;
-- 
2.10.1

[PATCH 3/6] nvme-fabrics: Fix memory leaks in nvmf_parse_options()

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:10:46PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 3/6] nvme-fabrics: Fix memory leaks in nvmf_parse_options()

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 4/6] nvme-fabrics: Fix a memory leak in an nvmf_create_ctrl() error path

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
---
 drivers/nvme/host/fabrics.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 3e37efa..8985de6 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -827,8 +827,7 @@ nvmf_create_ctrl(struct device *dev, const char *buf, size_t count)
 out_unlock:
 	mutex_unlock(&nvmf_transports_mutex);
 out_free_opts:
-	nvmf_host_put(opts->host);
-	kfree(opts);
+	nvmf_free_options(opts);
 	return ERR_PTR(ret);
 }
 
-- 
2.10.1

[PATCH 4/6] nvme-fabrics: Fix a memory leak in an nvmf_create_ctrl() error path

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:11:03PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 4/6] nvme-fabrics: Fix a memory leak in an nvmf_create_ctrl() error path

[Christoph Hellwig]

On Tue, Oct 18, 2016@01:11:03PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>

Looks fine,

Reviewed-by: Christoph Hellwig <hch at lst.de>

[PATCH 5/6] nvme-fabrics: Print network address if address resolution fails

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 5a83881..9612ea0 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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,
-			"rdma_resolve_addr wait failed (%d).\n", ret);
+			 "rdma_resolve_addr(%pISpc) wait failed (%d).\n",
+			 &ctrl->addr, ret);
 		goto out_destroy_cm_id;
 	}
 
-- 
2.10.1

[PATCH 5/6] nvme-fabrics: Print network address if address resolution fails

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:11:28PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 5/6] nvme-fabrics: Print network address if address resolution fails

[Christoph Hellwig]

On Tue, Oct 18, 2016@01:11:28PM -0700, Bart Van Assche wrote:
> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> ---
>  drivers/nvme/host/rdma.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 5a83881..9612ea0 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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,
> -			"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"

[PATCH 5/6] nvme-fabrics: Print network address if address resolution fails

[Bart Van Assche]

On Wed, 2016-10-19@12:39 +0200, Christoph Hellwig wrote:
> On Tue, Oct 18, 2016@01:11:28PM -0700, Bart Van Assche wrote:
> > Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
> > ---
> > ?drivers/nvme/host/rdma.c | 6 ++++--
> > ?1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> > index 5a83881..9612ea0 100644
> > --- a/drivers/nvme/host/rdma.c
> > +++ b/drivers/nvme/host/rdma.c
> > @@ -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,
> > -			"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"

Hello Christoph,

Thanks for the review. I will update this patch as you proposed.

Bart.

[PATCH 5/6] nvme-fabrics: Print network address if address resolution fails

[Sagi Grimberg]

>> Signed-off-by: Bart Van Assche <bart.vanassche at sandisk.com>
>> ---
>>  drivers/nvme/host/rdma.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
>> index 5a83881..9612ea0 100644
>> --- a/drivers/nvme/host/rdma.c
>> +++ b/drivers/nvme/host/rdma.c
>> @@ -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,
>> -			"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"

failure is nvme_rdma_wait_for_cm() does not necessarily means that
a resolution failed, but the connection establishment failed.

Maybe:
		"Failed to connect %pISpc, error %d.\n"

?

Other than that, looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Bart Van Assche]

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 9612ea0..df7f599 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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));
+		}
+		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;
 }
 
-- 
2.10.1

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Johannes Thumshirn]

On Tue, Oct 18, 2016@01:12:03PM -0700, Bart Van Assche wrote:
> 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>
> ---

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Christoph Hellwig]

On Tue, Oct 18, 2016@01:12:03PM -0700, Bart Van Assche wrote:
> 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(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 9612ea0..df7f599 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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)

No need to reindent the prototype..

>  {
> +	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?

Otherwise the change looks fine to me.

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Bart Van Assche]

On 10/19/2016 03:41 AM, Christoph Hellwig wrote:
>>  {
>> +	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?

Hello Christoph,

That cast is not necessary according to the C language standard. I will 
remove it.

Bart.

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Sagi Grimberg]

> +	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));
> +		}

Can you add "consumer defined" to the string so we know its an
application error reason?


Otherwise looks fine,

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

> 
> 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(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 9612ea0..df7f599 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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));
> +		}
> +		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;

Hey Bart,

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


Steve.

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Bart Van Assche]

On 10/20/2016 07:35 AM, Steve Wise wrote:
> 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. :)

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

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 8a80a04..c1b9fd6 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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)
 {
 	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

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

> On 10/20/2016 07:35 AM, Steve Wise wrote:
> > 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. :)
> 
> 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.
>

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.  

 
> 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(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 8a80a04..c1b9fd6 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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)
>  {
>  	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
> 

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Bart Van Assche]

On 10/20/2016 10:45 AM, Steve Wise wrote:
> 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.

Bart.

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

> On 10/20/2016 10:45 AM, Steve Wise wrote:
> > 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.

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".  

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

> 
> > On 10/20/2016 10:45 AM, Steve Wise wrote:
> > > 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.
> 
> 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)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 5f65a78..40a2a6f 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -101,6 +101,67 @@ const char *__attribute_const__ rdma_event_msg(enum rdma_cm_event_type event)
 }
 EXPORT_SYMBOL(rdma_event_msg);

+static const char * const ib_rej_reason_strs[] = {
+       [IB_CM_REJ_NO_QP]                       = "no qp",
+       [IB_CM_REJ_NO_EEC]                      = "no eec",
+       [IB_CM_REJ_NO_RESOURCES]                = "no resources",
+       [IB_CM_REJ_TIMEOUT]                     = "timeout",
+       [IB_CM_REJ_UNSUPPORTED]                 = "unsupported",
+       [IB_CM_REJ_INVALID_COMM_ID]             = "invalid comm id",
+       [IB_CM_REJ_INVALID_COMM_INSTANCE]       = "invalid comm instance",
+       [IB_CM_REJ_INVALID_SERVICE_ID]          = "invalid service id",
+       [IB_CM_REJ_INVALID_TRANSPORT_TYPE]      = "invalid transport type",
+       [IB_CM_REJ_STALE_CONN]                  = "stale conn",
+       [IB_CM_REJ_RDC_NOT_EXIST]               = "rdc not exist",
+       [IB_CM_REJ_INVALID_GID]                 = "invalid gid",
+       [IB_CM_REJ_INVALID_LID]                 = "invalid lid",
+       [IB_CM_REJ_INVALID_SL]                  = "invalid sl",
+       [IB_CM_REJ_INVALID_TRAFFIC_CLASS]       = "invalid traffic class",
+       [IB_CM_REJ_INVALID_HOP_LIMIT]           = "invalid hop limit",
+       [IB_CM_REJ_INVALID_PACKET_RATE]         = "invalid packet rate",
+       [IB_CM_REJ_INVALID_ALT_GID]             = "invalid alt gid",
+       [IB_CM_REJ_INVALID_ALT_LID]             = "invalid alt lid",
+       [IB_CM_REJ_INVALID_ALT_SL]              = "invalid alt sl",
+       [IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]   = "invalid alt traffic class",
+       [IB_CM_REJ_INVALID_ALT_HOP_LIMIT]       = "invalid alt hop limit",
+       [IB_CM_REJ_INVALID_ALT_PACKET_RATE]     = "invalid alt packet rate",
+       [IB_CM_REJ_PORT_CM_REDIRECT]            = "port cm redirect",
+       [IB_CM_REJ_PORT_REDIRECT]               = "port redirect",
+       [IB_CM_REJ_INVALID_MTU]                 = "invalid mtu",
+       [IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES] = "insufficient resp resources",
+       [IB_CM_REJ_CONSUMER_DEFINED]            = "consumer defined",
+       [IB_CM_REJ_INVALID_RNR_RETRY]           = "invalid rnr retry",
+       [IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]     = "duplicate local comm id",
+       [IB_CM_REJ_INVALID_CLASS_VERSION]       = "invalid class version",
+       [IB_CM_REJ_INVALID_FLOW_LABEL]          = "invalid flow label",
+       [IB_CM_REJ_INVALID_ALT_FLOW_LABEL]      = "invalid alt flow label",
+};
+
+static const char * const iw_rej_reason_strs[] = {
+       [ECONNRESET]                            = "reset by remote host",
+       [ECONNREFUSED]                          = "refused by remote application",
+       [ETIMEDOUT]                             = "setup timeout",
+};
+
+const char *__attribute_const__ rdma_reject_status_msg(struct rdma_cm_id *cm_id, int reason)
+{
+       size_t index = reason;
+
+       if (rdma_protocol_ib(cm_id->device, cm_id->port_num))
+               return (index < ARRAY_SIZE(ib_rej_reason_strs) && ib_rej_reason_strs[index]) ?
+                       ib_rej_reason_strs[index] : "unrecognized reason";
+
+       if (rdma_protocol_iwarp(cm_id->device, cm_id->port_num)) {
+
+               /* iWARP uses negative errnos */
+               index = -index;
+               return (index < ARRAY_SIZE(iw_rej_reason_strs) && iw_rej_reason_strs[index]) ?
+                       iw_rej_reason_strs[index] : "unrecognized reason";
+       }
+       return "unrecognized reason";
+}
+EXPORT_SYMBOL(rdma_reject_status_msg);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

> -----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
> 
> >
> > > On 10/20/2016 10:45 AM, Steve Wise wrote:
> > > > 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.
> >
> > 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.



diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 601aecf..41a2d4c 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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,
-                       "Connect rejected, no private data.\n");
+               nvme_status = le16_to_cpu(rej->sts);
        }

+       dev_err(queue->ctrl->ctrl.device, "Connect rejected: status %d (%s) "
+               "nvme status %d.\n", ev->status,
+               rdma_reject_msg(queue->cm_id, ev->status), nvme_status);
+
        return -ECONNRESET;
 }

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Bart Van Assche]

On 10/20/2016 01:41 PM, Steve Wise wrote:
> 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.
>
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 601aecf..41a2d4c 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -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,
> -                       "Connect rejected, no private data.\n");
> +               nvme_status = le16_to_cpu(rej->sts);
>         }
>
> +       dev_err(queue->ctrl->ctrl.device, "Connect rejected: status %d (%s) "
> +               "nvme status %d.\n", ev->status,
> +               rdma_reject_msg(queue->cm_id, ev->status), nvme_status);
> +
>         return -ECONNRESET;
>  }

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?

Thanks,

Bart.

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

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

[PATCH 6/6] nvme/rdma: Make nvme_rdma_conn_rejected() more informative

[Steve Wise]

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

Perhaps another helper in addition to rdma_reject_msg():  rdma_reject_has_user_data()?

[PATCH 0/6] Six NVMeOF-related patches

[Keith Busch]

On Tue, Oct 18, 2016@01:09:39PM -0700, Bart Van Assche wrote:
> 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:

All look good to me. I'm still mainly focused on PCIe, but since these
are cleaning up code and error handling, I feel safe to review these.

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH 0/6] Six NVMeOF-related patches

[Sagi Grimberg]

Hey Bart,

> 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

Overall, these changes looks good.

I plan to collect patches for the next round of 4.9-rc over the weekend
so I'll wait for your respin on this.

Thanks,
Sagi.

[PATCH 0/6] Six NVMeOF-related patches

[Bart Van Assche]

On 10/20/2016 01:02 AM, Sagi Grimberg wrote:
> 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

Hello Sagi,

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]?

Thanks,

Bart.

[PATCH 0/6] Six NVMeOF-related patches

[Christoph Hellwig]

On Thu, Oct 20, 2016@10:38:39AM -0700, Bart Van Assche wrote:
> 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?

[PATCH 0/6] Six NVMeOF-related patches

[Keith Busch]

On Fri, Oct 21, 2016@06:32:52AM -0700, Christoph Hellwig wrote:
> 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.

[PATCH 0/6] Six NVMeOF-related patches

[Sagi Grimberg]

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

True,

However this tree is designed to share the load with Jens so he doesn't
need to closely monitor all the fabrics changes. IMO It worked pretty
well for the inclusion process and 4.8-rc fixes.

I was a bit busy lately but wanted to pick up on 4.9-rc and 4.10 related
stuff next week.

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

It all goes through Jens, I think Christoph was offering to increase the
scope of the nvmf tree to nvme in general. This way, you, Christoph and
me can offload from Jens some nvme related patch traffic and send him
pull requests. But if you are more comfortable with the way things are
we can leave it as is.

[PATCH 0/6] Six NVMeOF-related patches

[Sagi Grimberg]

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!

[PATCH 0/6] Six NVMeOF-related patches

[Steve Wise]

> 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!

I will.  I will submit another, potentially final, version including review tags
and also a few changes based on this round's comments.

Steve.