[PATCH 0/11] More SRP patches for kernel v4.7

[PATCH 0/11] More SRP patches for kernel v4.7

[Bart Van Assche]

Hello Doug,

This patch series is what I came up with while retesting the SRP 
initiator. Please consider these patches for inclusion in kernel v4.7. 
Several of the patches in this series had already been posted in October 
2015. See also http://thread.gmane.org/gmane.linux.drivers.rdma/30216.

The individual patches in this series are:

0001-IB-srp-Fix-a-spelling-error-in-a-source-code-comment.patch
0002-IB-srp-Fix-a-comment.patch
0003-IB-srp-Document-srp_map_data-return-value.patch
0004-IB-srp-Fix-srp_map_data-error-paths.patch
0005-IB-srp-Introduce-target-mr_pool_size.patch
0006-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
0007-IB-srp-Move-code-out-of-a-loop.patch
0008-IB-srp-Move-common-code-into-the-caller.patch
0009-IB-srp-Fix-a-NULL-pointer-dereference.patch
0010-IB-srp-Do-not-register-memory-if-register_always-1.patch
0011-IB-srp-Prevent-mapping-failures.patch
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/11] IB/srp: Fix a spelling error in a source code comment

[Bart Van Assche]

Change one occurrence of "boundries" into "boundaries".

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index cff7ec7..9b4f21e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1398,7 +1398,7 @@ static int srp_map_sg_entry(struct srp_map_state *state,
 	/*
 	 * If the last entry of the MR wasn't a full page, then we need to
 	 * close it out and start a new one -- we can only merge at page
-	 * boundries.
+	 * boundaries.
 	 */
 	ret = 0;
 	if (len != dev->mr_page_size)
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 01/11] IB/srp: Fix a spelling error in a source code comment

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 02/11] IB/srp: Fix a comment

[Bart Van Assche]

The free request list was removed through patch "IB/srp: Use block layer tags".
Hence update a comment that refers to that free request list.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 9b4f21e..965b070 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1112,7 +1112,7 @@ static struct scsi_cmnd *srp_claim_req(struct srp_rdma_ch *ch,
 }
 
 /**
- * srp_free_req() - Unmap data and add request to the free request list.
+ * srp_free_req() - Unmap data and adjust ch->req_lim.
  * @ch:     SRP RDMA channel.
  * @req:    Request to be freed.
  * @scmnd:  SCSI command associated with @req.
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/11] IB/srp: Fix a comment

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 02/11] IB/srp: Fix a comment

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 03/11] IB/srp: Document srp_map_data() return value

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 965b070..2f8ab5c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1534,6 +1534,15 @@ static int srp_map_idb(struct srp_rdma_ch *ch, struct srp_request *req,
 	return 0;
 }
 
+/**
+ * srp_map_data() - map SCSI data buffer onto an SRP request
+ * @scmnd: SCSI command to map
+ * @ch: SRP RDMA channel
+ * @req: SRP request
+ *
+ * Returns the length in bytes of the SRP_CMD IU or a negative value if
+ * mapping failed.
+ */
 static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 			struct srp_request *req)
 {
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 03/11] IB/srp: Document srp_map_data() return value

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 04/11] IB/srp: Fix srp_map_data() error paths

[Bart Van Assche]

Ensure that req->nmdesc is set correctly in srp_map_sg() if mapping
fails. Avoid that mapping failure causes a memory descriptor leak.
Report srp_map_sg() failure to the caller.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 2f8ab5c..1354a09 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1428,8 +1428,6 @@ static int srp_map_sg_fmr(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	if (ret)
 		return ret;
 
-	req->nmdesc = state->nmdesc;
-
 	return 0;
 }
 
@@ -1454,8 +1452,6 @@ static int srp_map_sg_fr(struct srp_map_state *state, struct srp_rdma_ch *ch,
 			state->sg = sg_next(state->sg);
 	}
 
-	req->nmdesc = state->nmdesc;
-
 	return 0;
 }
 
@@ -1475,8 +1471,6 @@ static int srp_map_sg_dma(struct srp_map_state *state, struct srp_rdma_ch *ch,
 			     target->global_mr->rkey);
 	}
 
-	req->nmdesc = state->nmdesc;
-
 	return 0;
 }
 
@@ -1610,11 +1604,14 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 
 	memset(&state, 0, sizeof(state));
 	if (dev->use_fast_reg)
-		srp_map_sg_fr(&state, ch, req, scat, count);
+		ret = srp_map_sg_fr(&state, ch, req, scat, count);
 	else if (dev->use_fmr)
-		srp_map_sg_fmr(&state, ch, req, scat, count);
+		ret = srp_map_sg_fmr(&state, ch, req, scat, count);
 	else
-		srp_map_sg_dma(&state, ch, req, scat, count);
+		ret = srp_map_sg_dma(&state, ch, req, scat, count);
+	req->nmdesc = state.nmdesc;
+	if (ret < 0)
+		goto unmap;
 
 	/* We've mapped the request, now pull as much of the indirect
 	 * descriptor table as we can into the command buffer. If this
@@ -1637,7 +1634,8 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 						!target->allow_ext_sg)) {
 		shost_printk(KERN_ERR, target->scsi_host,
 			     "Could not fit S/G list into SRP_CMD\n");
-		return -EIO;
+		ret = -EIO;
+		goto unmap;
 	}
 
 	count = min(state.ndesc, target->cmd_sg_cnt);
@@ -1655,7 +1653,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 		ret = srp_map_idb(ch, req, state.gen.next, state.gen.end,
 				  idb_len, &idb_rkey);
 		if (ret < 0)
-			return ret;
+			goto unmap;
 		req->nmdesc++;
 	} else {
 		idb_rkey = cpu_to_be32(target->global_mr->rkey);
@@ -1681,6 +1679,10 @@ map_complete:
 		cmd->buf_fmt = fmt;
 
 	return len;
+
+unmap:
+	srp_unmap_data(scmnd, ch, req);
+	return ret;
 }
 
 /*
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/11] IB/srp: Fix srp_map_data() error paths

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/11] IB/srp: Fix srp_map_data() error paths

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/11] IB/srp: Introduce target->mr_pool_size

[Bart Van Assche]

This patch does not change any functionality.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 6 +++---
 drivers/infiniband/ulp/srp/ib_srp.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 1354a09..8fd1a51 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -316,7 +316,7 @@ static struct ib_fmr_pool *srp_alloc_fmr_pool(struct srp_target_port *target)
 	struct ib_fmr_pool_param fmr_param;
 
 	memset(&fmr_param, 0, sizeof(fmr_param));
-	fmr_param.pool_size	    = target->scsi_host->can_queue;
+	fmr_param.pool_size	    = target->mr_pool_size;
 	fmr_param.dirty_watermark   = fmr_param.pool_size / 4;
 	fmr_param.cache		    = 1;
 	fmr_param.max_pages_per_fmr = dev->max_pages_per_mr;
@@ -441,8 +441,7 @@ static struct srp_fr_pool *srp_alloc_fr_pool(struct srp_target_port *target)
 {
 	struct srp_device *dev = target->srp_host->srp_dev;
 
-	return srp_create_fr_pool(dev->dev, dev->pd,
-				  target->scsi_host->can_queue,
+	return srp_create_fr_pool(dev->dev, dev->pd, target->mr_pool_size,
 				  dev->max_pages_per_mr);
 }
 
@@ -3229,6 +3228,7 @@ static ssize_t srp_create_target(struct device *dev,
 	}
 
 	target_host->sg_tablesize = target->sg_tablesize;
+	target->mr_pool_size = target->scsi_host->can_queue;
 	target->indirect_size = target->sg_tablesize *
 				sizeof (struct srp_direct_buf);
 	target->max_iu_len = sizeof (struct srp_cmd) +
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 9e05ce4..a00914c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -202,6 +202,7 @@ struct srp_target_port {
 	char			target_name[32];
 	unsigned int		scsi_id;
 	unsigned int		sg_tablesize;
+	int			mr_pool_size;
 	int			queue_size;
 	int			req_ring_size;
 	int			comp_vector;
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/11] IB/srp: Introduce target->mr_pool_size

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 06/11] IB/srp: Avoid that mapping failure triggers an infinite loop

[Bart Van Assche]

The srp_queuecommand() function translates ENOMEM into QUEUE_FULL
which causes the SCSI mid-layer to retry the command. All other
error codes are translated into DID_ERROR which causes the SCSI
command to fail. Return E2BIG if mapping will always fail to
prevent that the SCSI mid-layer keeps resubmitting a command
forever.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8fd1a51..c0329cd 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1681,6 +1681,8 @@ map_complete:
 
 unmap:
 	srp_unmap_data(scmnd, ch, req);
+	if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
+		ret = -E2BIG;
 	return ret;
 }
 
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 06/11] IB/srp: Avoid that mapping failure triggers an infinite loop

[Christoph Hellwig]

That's a bit of an odd error, but given that it doesn't leak out
this is fine with me:

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 07/11] IB/srp: Move code out of a loop

[Bart Van Assche]

Since all srp_map_finish_fr() callers pass a non-zero value as
the fourth argument (sg_nents), the sg_nents == 0 check in that
function can be removed. Add a count == 0 check in the caller
of that function.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index c0329cd..d709428c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1315,9 +1315,6 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 
 	WARN_ON_ONCE(!dev->use_fast_reg);
 
-	if (sg_nents == 0)
-		return 0;
-
 	if (sg_nents == 1 && target->global_mr) {
 		srp_map_desc(state, sg_dma_address(state->sg),
 			     sg_dma_len(state->sg),
@@ -1439,6 +1436,9 @@ static int srp_map_sg_fr(struct srp_map_state *state, struct srp_rdma_ch *ch,
 	state->fr.end = req->fr_list + ch->target->cmd_sg_cnt;
 	state->sg = scat;
 
+	if (count == 0)
+		return 0;
+
 	while (count) {
 		int i, n;
 
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/11] IB/srp: Move code out of a loop

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/11] IB/srp: Move code out of a loop

[Christoph Hellwig]

On Fri, Apr 22, 2016 at 02:14:43PM -0700, Bart Van Assche wrote:
> Since all srp_map_finish_fr() callers pass a non-zero value as
> the fourth argument (sg_nents), the sg_nents == 0 check in that
> function can be removed. Add a count == 0 check in the caller
> of that function.

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 08/11] IB/srp: Move common code into the caller

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
  drivers/infiniband/ulp/srp/ib_srp.c | 4 +---
  1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c 
b/drivers/infiniband/ulp/srp/ib_srp.c
index d709428c..caefd1a 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1409,7 +1409,6 @@ static int srp_map_sg_fmr(struct srp_map_state 
*state, struct srp_rdma_ch *ch,
  	struct scatterlist *sg;
  	int i, ret;

-	state->desc = req->indirect_desc;
  	state->pages = req->map_page;
  	state->fmr.next = req->fmr_list;
  	state->fmr.end = req->fmr_list + ch->target->cmd_sg_cnt;
@@ -1431,7 +1430,6 @@ static int srp_map_sg_fr(struct srp_map_state 
*state, struct srp_rdma_ch *ch,
  			 struct srp_request *req, struct scatterlist *scat,
  			 int count)
  {
-	state->desc = req->indirect_desc;
  	state->fr.next = req->fr_list;
  	state->fr.end = req->fr_list + ch->target->cmd_sg_cnt;
  	state->sg = scat;
@@ -1463,7 +1461,6 @@ static int srp_map_sg_dma(struct srp_map_state 
*state, struct srp_rdma_ch *ch,
  	struct scatterlist *sg;
  	int i;

-	state->desc = req->indirect_desc;
  	for_each_sg(scat, sg, count, i) {
  		srp_map_desc(state, ib_sg_dma_address(dev->dev, sg),
  			     ib_sg_dma_len(dev->dev, sg),
@@ -1602,6 +1599,7 @@ static int srp_map_data(struct scsi_cmnd *scmnd, 
struct srp_rdma_ch *ch,
  				   target->indirect_size, DMA_TO_DEVICE);

  	memset(&state, 0, sizeof(state));
+	state.desc = req->indirect_desc;
  	if (dev->use_fast_reg)
  		ret = srp_map_sg_fr(&state, ch, req, scat, count);
  	else if (dev->use_fmr)
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/11] IB/srp: Move common code into the caller

[Sagi Grimberg]

Looks fine,

Reviewed-by: Sagi Grimberg <sai@grimberg.m>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 08/11] IB/srp: Move common code into the caller

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Bart Van Assche]

Avoid that running xfstests on top of the SRP initiator triggers
the call trace below. This patch has been tested by running the
following shell command on an initiator system that has access
to 3200 SRP LUNs:

/etc/init.d/multipathd start
while true; do
  /etc/init.d/srpd start
  sleep 400
  /etc/init.d/srpd stop
  for p in /sys/class/srp_remote_ports/*; do
    echo 1 >$p/delete &
  done
  wait
  dmsetup remove_all
done

BUG: unable to handle kernel NULL pointer dereference at           (null)
IP: [<ffffffffa0918bb1>] srp_claim_req+0x31/0x90 [ib_srp]
Call Trace:
 <IRQ>
 [<ffffffffa091c096>] srp_process_rsp+0xa6/0x2a0 [ib_srp]
 [<ffffffffa091c5ec>] srp_handle_recv+0x16c/0x340 [ib_srp]
 [<ffffffffa091c7f9>] srp_recv_completion+0x39/0x70 [ib_srp]
 [<ffffffffa0184442>] mlx4_ib_cq_comp+0x12/0x20 [mlx4_ib]
 [<ffffffffa005e86d>] mlx4_cq_completion+0x3d/0x80 [mlx4_core]
 [<ffffffffa006002b>] mlx4_eq_int+0x53b/0xd50 [mlx4_core]
 [<ffffffffa006084f>] mlx4_msi_x_interrupt+0xf/0x20 [mlx4_core]
 [<ffffffff810b67e0>] handle_irq_event_percpu+0x40/0x110
 [<ffffffff810b68ef>] handle_irq_event+0x3f/0x70
 [<ffffffff810ba829>] handle_edge_irq+0x79/0x120
 [<ffffffff81007f2d>] handle_irq+0x5d/0x130
 [<ffffffff810071ed>] do_IRQ+0x6d/0x130
 [<ffffffff8151c104>] common_interrupt+0x84/0x84
 <EOI>

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index caefd1a..f4003f6 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1814,7 +1814,8 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp)
 		scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
 		if (scmnd) {
 			req = (void *)scmnd->host_scribble;
-			scmnd = srp_claim_req(ch, req, NULL, scmnd);
+			scmnd = req ? srp_claim_req(ch, req, NULL, scmnd) :
+				NULL;
 		}
 		if (!scmnd) {
 			shost_printk(KERN_ERR, target->scsi_host,
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Sagi Grimberg]

> Avoid that running xfstests on top of the SRP initiator triggers
> the call trace below. This patch has been tested by running the
> following shell command on an initiator system that has access
> to 3200 SRP LUNs:

That's good to know, but the patch description needs to state where
the NULL deref originates i.e. when can req be NULL and why it is
OK to just assign to NULL and continue...

>   		scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
>   		if (scmnd) {
>   			req = (void *)scmnd->host_scribble;
> -			scmnd = srp_claim_req(ch, req, NULL, scmnd);
> +			scmnd = req ? srp_claim_req(ch, req, NULL, scmnd) :
> +				NULL;
>   		}
>   		if (!scmnd) {
>   			shost_printk(KERN_ERR, target->scsi_host,
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On Wed, Apr 27, 2016 at 12:04:52AM +0300, Sagi Grimberg wrote:
> >Avoid that running xfstests on top of the SRP initiator triggers
> >the call trace below. This patch has been tested by running the
> >following shell command on an initiator system that has access
> >to 3200 SRP LUNs:
> 
> That's good to know, but the patch description needs to state where
> the NULL deref originates i.e. when can req be NULL and why it is
> OK to just assign to NULL and continue...
> 
> >  		scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
> >  		if (scmnd) {
> >  			req = (void *)scmnd->host_scribble;
> >-			scmnd = srp_claim_req(ch, req, NULL, scmnd);
> >+			scmnd = req ? srp_claim_req(ch, req, NULL, scmnd) :
> >+				NULL;
> >  		}
> >  		if (!scmnd) {
> >  			shost_printk(KERN_ERR, target->scsi_host,
> >

And if it is OK to assign NULL to scmd, will the error print above still
valid?

> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Bart Van Assche]

On 04/26/16 23:20, Leon Romanovsky wrote:
> On Wed, Apr 27, 2016 at 12:04:52AM +0300, Sagi Grimberg wrote:
>>> Avoid that running xfstests on top of the SRP initiator triggers
>>> the call trace below. This patch has been tested by running the
>>> following shell command on an initiator system that has access
>>> to 3200 SRP LUNs:
>>
>> That's good to know, but the patch description needs to state where
>> the NULL deref originates i.e. when can req be NULL and why it is
>> OK to just assign to NULL and continue...
>>
>>>   		scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
>>>   		if (scmnd) {
>>>   			req = (void *)scmnd->host_scribble;
>>> -			scmnd = srp_claim_req(ch, req, NULL, scmnd);
>>> +			scmnd = req ? srp_claim_req(ch, req, NULL, scmnd) :
>>> +				NULL;
>>>   		}
>>>   		if (!scmnd) {
>>>   			shost_printk(KERN_ERR, target->scsi_host,
>>>
>
> And if it is OK to assign NULL to scmd, will the error print above still
> valid?

Sorry Leon but I don't understand your question. Have you noticed the if 
(!scmnd) above that error print?

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Bart Van Assche]

On 04/26/2016 11:20 PM, Leon Romanovsky wrote:
> On Wed, Apr 27, 2016 at 12:04:52AM +0300, Sagi Grimberg wrote:
>>> Avoid that running xfstests on top of the SRP initiator triggers
>>> the call trace below. This patch has been tested by running the
>>> following shell command on an initiator system that has access
>>> to 3200 SRP LUNs:
>>
>> That's good to know, but the patch description needs to state where
>> the NULL deref originates i.e. when can req be NULL and why it is
>> OK to just assign to NULL and continue...
>>
>>>   		scmnd = scsi_host_find_tag(target->scsi_host, rsp->tag);
>>>   		if (scmnd) {
>>>   			req = (void *)scmnd->host_scribble;
>>> -			scmnd = srp_claim_req(ch, req, NULL, scmnd);
>>> +			scmnd = req ? srp_claim_req(ch, req, NULL, scmnd) :
>>> +				NULL;
>>>   		}
>>>   		if (!scmnd) {
>>>   			shost_printk(KERN_ERR, target->scsi_host,
>>>
>
> And if it is OK to assign NULL to scmd, will the error print above still
> valid?

Yes. The purpose of that printk() statement is to print a message 
whenever an SRP response has been received but that response is not 
passed to the SCSI mid-layer through scsi_done().

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Bart Van Assche]

On 04/26/2016 02:04 PM, Sagi Grimberg wrote:
>> Avoid that running xfstests on top of the SRP initiator triggers
>> the call trace below. This patch has been tested by running the
>> following shell command on an initiator system that has access
>> to 3200 SRP LUNs:
>
> That's good to know, but the patch description needs to state where
> the NULL deref originates i.e. when can req be NULL and why it is
> OK to just assign to NULL and continue...

I think the issue fixed by this patch is a use-after-free of scmnd. If 
an rport is deleted srp_finish_req() calls scsi_done(). A concurrent 
scsi_queue_rq() call can reuse that scmnd before srp_process_rsp() gets 
called. This wouldn't occur if srp_wait_for_queuecommand() would also 
wait for scsi_queue_rq(). Since I expect it will be hard to convince the 
block layer maintainer to fix the root cause I came up with this patch.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Christoph Hellwig]

On Fri, Apr 22, 2016 at 02:15:30PM -0700, Bart Van Assche wrote:
> Avoid that running xfstests on top of the SRP initiator triggers
> the call trace below. This patch has been tested by running the
> following shell command on an initiator system that has access
> to 3200 SRP LUNs:

As mentioned by the others a better changelog would be useful.

> 
> /etc/init.d/multipathd start
> while true; do
>   /etc/init.d/srpd start
>   sleep 400
>   /etc/init.d/srpd stop
>   for p in /sys/class/srp_remote_ports/*; do
>     echo 1 >$p/delete &
>   done
>   wait
>   dmsetup remove_all
> done

Any chance you could have a git repo with all your srp tests?  Having them
avaiable in a single place for others to use would really help with
test coverage I think.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Bart Van Assche]

On 05/03/2016 02:30 AM, Christoph Hellwig wrote:
> Any chance you could have a git repo with all your srp tests?  Having them
> available in a single place for others to use would really help with
> test coverage I think.

I will try to free up some time to work on this. I will need an SRP 
testsuite anyway before I simplify the SRP initiator by removing support 
for SG-lists with gaps (by setting q->limits.virt_boundary_mask).

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

[Laurence Oberman]

----- Original Message -----
From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
To: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>
Cc: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Tuesday, May 3, 2016 4:57:02 PM
Subject: Re: [PATCH 09/11] IB/srp: Fix a NULL pointer dereference

On 05/03/2016 02:30 AM, Christoph Hellwig wrote:
> Any chance you could have a git repo with all your srp tests?  Having them
> available in a single place for others to use would really help with
> test coverage I think.

I will try to free up some time to work on this. I will need an SRP 
testsuite anyway before I simplify the SRP initiator by removing support 
for SG-lists with gaps (by setting q->limits.virt_boundary_mask).

Bart.

I am always happy to help test anything if needed.
I have a standing configuration in my lab always setup.

Thanks
Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 10/11] IB/srp: Do not register memory if register_always = -1

[Bart Van Assche]

This makes it easier to test the code path that does not use
memory registration (srp_map_sg_dma()).

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
Cc: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f4003f6..a173ec4 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -69,7 +69,7 @@ static unsigned int cmd_sg_entries;
 static unsigned int indirect_sg_entries;
 static bool allow_ext_sg;
 static bool prefer_fr = true;
-static bool register_always = true;
+static int register_always = true;
 static int topspin_workarounds = 1;
 
 module_param(srp_sg_tablesize, uint, 0444);
@@ -95,7 +95,7 @@ module_param(prefer_fr, bool, 0444);
 MODULE_PARM_DESC(prefer_fr,
 "Whether to use fast registration if both FMR and fast registration are supported");
 
-module_param(register_always, bool, 0444);
+module_param(register_always, int, 0444);
 MODULE_PARM_DESC(register_always,
 		 "Use memory registration even for contiguous memory regions");
 
@@ -1600,9 +1600,9 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_rdma_ch *ch,
 
 	memset(&state, 0, sizeof(state));
 	state.desc = req->indirect_desc;
-	if (dev->use_fast_reg)
+	if (dev->use_fast_reg && register_always >= 0)
 		ret = srp_map_sg_fr(&state, ch, req, scat, count);
-	else if (dev->use_fmr)
+	else if (dev->use_fmr && register_always >= 0)
 		ret = srp_map_sg_fmr(&state, ch, req, scat, count);
 	else
 		ret = srp_map_sg_dma(&state, ch, req, scat, count);
@@ -3430,12 +3430,13 @@ static void srp_add_one(struct ib_device *device)
 			    device->map_phys_fmr && device->unmap_fmr);
 	srp_dev->has_fr = (device->attrs.device_cap_flags &
 			   IB_DEVICE_MEM_MGT_EXTENSIONS);
-	if (!srp_dev->has_fmr && !srp_dev->has_fr)
+	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
-
-	srp_dev->use_fast_reg = (srp_dev->has_fr &&
-				 (!srp_dev->has_fmr || prefer_fr));
-	srp_dev->use_fmr = !srp_dev->use_fast_reg && srp_dev->has_fmr;
+	} else if (register_always >= 0) {
+		srp_dev->use_fast_reg = (srp_dev->has_fr &&
+					 (!srp_dev->has_fmr || prefer_fr));
+		srp_dev->use_fmr = !srp_dev->use_fast_reg && srp_dev->has_fmr;
+	}
 
 	/*
 	 * Use the smallest page size supported by the HCA, down to a
@@ -3468,7 +3469,7 @@ static void srp_add_one(struct ib_device *device)
 	if (IS_ERR(srp_dev->pd))
 		goto free_dev;
 
-	if (!register_always || (!srp_dev->has_fmr && !srp_dev->has_fr)) {
+	if (register_always <= 0 || (!srp_dev->has_fmr && !srp_dev->has_fr)) {
 		srp_dev->global_mr = ib_get_dma_mr(srp_dev->pd,
 						   IB_ACCESS_LOCAL_WRITE |
 						   IB_ACCESS_REMOTE_READ |
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/11] IB/srp: Do not register memory if register_always = -1

[Christoph Hellwig]

On Fri, Apr 22, 2016 at 02:16:00PM -0700, Bart Van Assche wrote:
> This makes it easier to test the code path that does not use
> memory registration (srp_map_sg_dma()).
> 
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>

How about a separate never_register parameter instead?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

If both max_sectors and the queue_depth are high enough it can
happen that the MR pool is depleted temporarily. This causes
the SRP initiator to report mapping failures. Although the SRP
initiator recovers from such mapping failures, prevent that
this can happen by limiting max_sectors.

Reported-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a173ec4..ebd4d90 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3172,6 +3172,7 @@ static ssize_t srp_create_target(struct device *dev,
 	struct srp_device *srp_dev = host->srp_dev;
 	struct ib_device *ibdev = srp_dev->dev;
 	int ret, node_idx, node, cpu, i;
+	unsigned int max_max_sectors;
 	bool multich = false;
 
 	target_host = scsi_host_alloc(&srp_template,
@@ -3228,6 +3229,25 @@ static ssize_t srp_create_target(struct device *dev,
 		target->sg_tablesize = target->cmd_sg_cnt;
 	}
 
+	if (srp_dev->use_fast_reg || srp_dev->use_fmr) {
+		/*
+		 * FR and FMR can only map one HCA page per entry. If the
+		 * start address is not aligned on a HCA page boundary two
+		 * entries will be used for the head and the tail although
+		 * these two entries combined contain at most one HCA page of
+		 * data. Hence the "- 1" in the calculation below.
+		 */
+		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
+				  (ilog2(srp_dev->mr_page_size) - 9);
+		if (target->scsi_host->max_sectors > max_max_sectors) {
+			shost_printk(KERN_WARNING, target->scsi_host,
+				     PFX "Reducing max_sectors from %d to %d\n",
+				     target->scsi_host->max_sectors,
+				     max_max_sectors);
+			target->scsi_host->max_sectors = max_max_sectors;
+		}
+	}
+
 	target_host->sg_tablesize = target->sg_tablesize;
 	target->mr_pool_size = target->scsi_host->can_queue;
 	target->indirect_size = target->sg_tablesize *
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

Hello Bart

I can confirm testing of this series and the specific patch for the sg map failures.
It prevents the sg mapping failures by reducing the max_sectors_kb and its stable.
That is what it was designed to do.

However it effectively prevents any max_sectors_kb from getting to 4MB in size.
This will also of course prevent 4MB I/O being set for DIRECT_IO.

Note that with DIRECT_IO we issue 4MB writes but the queue depth is lower as we dont have any buffering and the 4MB I/O are sent direct to the target.
The average queue depths are lower.

With buffered I/O we have dynamic queuing and I/O sizes are not set at 4MB.

Does that mean we cannot going forward be looking to get the larger max_sectors_kb 

[  221.378728] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4088
[  229.563814] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4088
[  245.620980] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4088
[  253.677979] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4088

[  221.303479] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  221.332902] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  229.563810] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  229.563811] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  245.544452] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  245.574899] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  253.677977] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  253.677978] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3

For the series Re: [PATCH 1/11] IB/srp to [PATCH 11/11] IB/srp

Tested-by Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
To: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Friday, April 22, 2016 5:16:31 PM
Subject: [PATCH 11/11] IB/srp: Prevent mapping failures

If both max_sectors and the queue_depth are high enough it can
happen that the MR pool is depleted temporarily. This causes
the SRP initiator to report mapping failures. Although the SRP
initiator recovers from such mapping failures, prevent that
this can happen by limiting max_sectors.

Reported-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a173ec4..ebd4d90 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3172,6 +3172,7 @@ static ssize_t srp_create_target(struct device *dev,
 	struct srp_device *srp_dev = host->srp_dev;
 	struct ib_device *ibdev = srp_dev->dev;
 	int ret, node_idx, node, cpu, i;
+	unsigned int max_max_sectors;
 	bool multich = false;
 
 	target_host = scsi_host_alloc(&srp_template,
@@ -3228,6 +3229,25 @@ static ssize_t srp_create_target(struct device *dev,
 		target->sg_tablesize = target->cmd_sg_cnt;
 	}
 
+	if (srp_dev->use_fast_reg || srp_dev->use_fmr) {
+		/*
+		 * FR and FMR can only map one HCA page per entry. If the
+		 * start address is not aligned on a HCA page boundary two
+		 * entries will be used for the head and the tail although
+		 * these two entries combined contain at most one HCA page of
+		 * data. Hence the "- 1" in the calculation below.
+		 */
+		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
+				  (ilog2(srp_dev->mr_page_size) - 9);
+		if (target->scsi_host->max_sectors > max_max_sectors) {
+			shost_printk(KERN_WARNING, target->scsi_host,
+				     PFX "Reducing max_sectors from %d to %d\n",
+				     target->scsi_host->max_sectors,
+				     max_max_sectors);
+			target->scsi_host->max_sectors = max_max_sectors;
+		}
+	}
+
 	target_host->sg_tablesize = target->sg_tablesize;
 	target->mr_pool_size = target->scsi_host->can_queue;
 	target->indirect_size = target->sg_tablesize *
-- 
2.8.1

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2883 bytes --]

On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
> If both max_sectors and the queue_depth are high enough it can
> happen that the MR pool is depleted temporarily. This causes
> the SRP initiator to report mapping failures. Although the SRP
> initiator recovers from such mapping failures, prevent that
> this can happen by limiting max_sectors.
> 
> Reported-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index a173ec4..ebd4d90 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -3172,6 +3172,7 @@ static ssize_t srp_create_target(struct device *dev,
>  	struct srp_device *srp_dev = host->srp_dev;
>  	struct ib_device *ibdev = srp_dev->dev;
>  	int ret, node_idx, node, cpu, i;
> +	unsigned int max_max_sectors;
>  	bool multich = false;
>  
>  	target_host = scsi_host_alloc(&srp_template,
> @@ -3228,6 +3229,25 @@ static ssize_t srp_create_target(struct device *dev,
>  		target->sg_tablesize = target->cmd_sg_cnt;
>  	}
>  
> +	if (srp_dev->use_fast_reg || srp_dev->use_fmr) {
> +		/*
> +		 * FR and FMR can only map one HCA page per entry. If the
> +		 * start address is not aligned on a HCA page boundary two
> +		 * entries will be used for the head and the tail although
> +		 * these two entries combined contain at most one HCA page of
> +		 * data. Hence the "- 1" in the calculation below.
> +		 */
> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> +				  (ilog2(srp_dev->mr_page_size) - 9);

From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
case device will advertise it.

Is this understanding correct?
Will the code work in such case?

> +		if (target->scsi_host->max_sectors > max_max_sectors) {
> +			shost_printk(KERN_WARNING, target->scsi_host,
> +				     PFX "Reducing max_sectors from %d to %d\n",
> +				     target->scsi_host->max_sectors,
> +				     max_max_sectors);
> +			target->scsi_host->max_sectors = max_max_sectors;
> +		}
> +	}
> +
>  	target_host->sg_tablesize = target->sg_tablesize;
>  	target->mr_pool_size = target->scsi_host->can_queue;
>  	target->indirect_size = target->sg_tablesize *
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

Hi Leon,
I never considered that because I have never seen it set to 1.

The values for my setup are shown below as 512.

In what situation would srp_dev->max_pages_per_mr=1.

I am using

[root@srptest ~]# cat /etc/modprobe.d/ib_srp.conf
options ib_srp cmd_sg_entries=64 indirect_sg_entries=512

[root@srptest ~]# cat /etc/modprobe.d/ib_srpt.conf
options ib_srpt srp_max_req_size=8296

[  221.378728] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4088
[  229.563814] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4088
[  245.620980] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4088
[  253.677979] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4088

[  221.303479] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  221.332902] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  229.563810] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  229.563811] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  245.544452] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  245.574899] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3
[  253.677977] RHDEBUG: srp_dev->max_pages_per_mr - 1 = 511
[  253.677978] RHDEBUG: srp_dev->mr_page_size = 4096 and ilog(srp_dev->mr_page_size) -9 = 3

I can test the defaults if you like

Laurence Oberman
Principal Software Maintenance Engineer
Red Hat Global Support Services

----- Original Message -----
From: "Leon Romanovsky" <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
To: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Sent: Sunday, April 24, 2016 4:35:38 AM
Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures

On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
> If both max_sectors and the queue_depth are high enough it can
> happen that the MR pool is depleted temporarily. This causes
> the SRP initiator to report mapping failures. Although the SRP
> initiator recovers from such mapping failures, prevent that
> this can happen by limiting max_sectors.
> 
> Reported-by: Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Christoph Hellwig <hch-jcswGhMUV9g@public.gmane.org>
> Cc: Sagi Grimberg <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index a173ec4..ebd4d90 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -3172,6 +3172,7 @@ static ssize_t srp_create_target(struct device *dev,
>  	struct srp_device *srp_dev = host->srp_dev;
>  	struct ib_device *ibdev = srp_dev->dev;
>  	int ret, node_idx, node, cpu, i;
> +	unsigned int max_max_sectors;
>  	bool multich = false;
>  
>  	target_host = scsi_host_alloc(&srp_template,
> @@ -3228,6 +3229,25 @@ static ssize_t srp_create_target(struct device *dev,
>  		target->sg_tablesize = target->cmd_sg_cnt;
>  	}
>  
> +	if (srp_dev->use_fast_reg || srp_dev->use_fmr) {
> +		/*
> +		 * FR and FMR can only map one HCA page per entry. If the
> +		 * start address is not aligned on a HCA page boundary two
> +		 * entries will be used for the head and the tail although
> +		 * these two entries combined contain at most one HCA page of
> +		 * data. Hence the "- 1" in the calculation below.
> +		 */
> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> +				  (ilog2(srp_dev->mr_page_size) - 9);

>From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
case device will advertise it.

Is this understanding correct?
Will the code work in such case?

> +		if (target->scsi_host->max_sectors > max_max_sectors) {
> +			shost_printk(KERN_WARNING, target->scsi_host,
> +				     PFX "Reducing max_sectors from %d to %d\n",
> +				     target->scsi_host->max_sectors,
> +				     max_max_sectors);
> +			target->scsi_host->max_sectors = max_max_sectors;
> +		}
> +	}
> +
>  	target_host->sg_tablesize = target->sg_tablesize;
>  	target->mr_pool_size = target->scsi_host->can_queue;
>  	target->indirect_size = target->sg_tablesize *
> -- 
> 2.8.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 04/24/16 01:35, Leon Romanovsky wrote:
> On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
>> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
>> +				  (ilog2(srp_dev->mr_page_size) - 9);
>
>  From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
> case device will advertise it.
>
> Is this understanding correct?
> Will the code work in such case?

Hello Leon,

I'm not sure a HCA driver exists for which max_pages_per_mr == 1. 
Additionally, I'm not sure such a driver would be useful because such a 
driver would limit the maximum data transfer size for the iSER protocol 
to 4 KB. That is too low to reach good performance.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1266 bytes --]

On Sun, Apr 24, 2016 at 05:48:53PM -0700, Bart Van Assche wrote:
> On 04/24/16 01:35, Leon Romanovsky wrote:
> >On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
> >>+		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> >>+				  (ilog2(srp_dev->mr_page_size) - 9);
> >
> > From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
> >case device will advertise it.
> >
> >Is this understanding correct?
> >Will the code work in such case?
> 
> Hello Leon,
> 
> I'm not sure a HCA driver exists for which max_pages_per_mr == 1.
> Additionally, I'm not sure such a driver would be useful because such a
> driver would limit the maximum data transfer size for the iSER protocol to 4
> KB. That is too low to reach good performance.

Bart and Laurence,
You are raising valuable arguments about sanity of setting srp_dev->max_pages_per_mr
to be equal 1. I totally agree with you that it is insane, however my
original question was "does the code support such value?"

> 
> Bart.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Sagi Grimberg]

>>>> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
>>>> +				  (ilog2(srp_dev->mr_page_size) - 9);
>>>
>>>  From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
>>> case device will advertise it.
>>>
>>> Is this understanding correct?
>>> Will the code work in such case?
>>
>> Hello Leon,
>>
>> I'm not sure a HCA driver exists for which max_pages_per_mr == 1.
>> Additionally, I'm not sure such a driver would be useful because such a
>> driver would limit the maximum data transfer size for the iSER protocol to 4
>> KB. That is too low to reach good performance.
>
> Bart and Laurence,
> You are raising valuable arguments about sanity of setting srp_dev->max_pages_per_mr
> to be equal 1. I totally agree with you that it is insane, however my
> original question was "does the code support such value?"

I don't see why we should worry about imaginary devices.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

On Mon, Apr 25, 2016 at 12:46:59PM +0300, Sagi Grimberg wrote:
> 
> >>>>+		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> >>>>+				  (ilog2(srp_dev->mr_page_size) - 9);
> >>>
> >>> From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
> >>>case device will advertise it.
> >>>
> >>>Is this understanding correct?
> >>>Will the code work in such case?
> >>
> >>Hello Leon,
> >>
> >>I'm not sure a HCA driver exists for which max_pages_per_mr == 1.
> >>Additionally, I'm not sure such a driver would be useful because such a
> >>driver would limit the maximum data transfer size for the iSER protocol to 4
> >>KB. That is too low to reach good performance.
> >
> >Bart and Laurence,
> >You are raising valuable arguments about sanity of setting srp_dev->max_pages_per_mr
> >to be equal 1. I totally agree with you that it is insane, however my
> >original question was "does the code support such value?"
> 
> I don't see why we should worry about imaginary devices.

It is called corner case and the code supports it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 04/25/2016 08:53 AM, Leon Romanovsky wrote:
> On Mon, Apr 25, 2016 at 12:46:59PM +0300, Sagi Grimberg wrote:
>>
>>>>>> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
>>>>>> +				  (ilog2(srp_dev->mr_page_size) - 9);
>>>>>
>>>>>  From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
>>>>> case device will advertise it.
>>>>>
>>>>> Is this understanding correct?
>>>>> Will the code work in such case?
>>>>
>>>> Hello Leon,
>>>>
>>>> I'm not sure a HCA driver exists for which max_pages_per_mr == 1.
>>>> Additionally, I'm not sure such a driver would be useful because such a
>>>> driver would limit the maximum data transfer size for the iSER protocol to 4
>>>> KB. That is too low to reach good performance.
>>>
>>> Bart and Laurence,
>>> You are raising valuable arguments about sanity of setting srp_dev->max_pages_per_mr
>>> to be equal 1. I totally agree with you that it is insane, however my
>>> original question was "does the code support such value?"
>>
>> I don't see why we should worry about imaginary devices.
>
> It is called corner case and the code supports it.

I can add a WARN_ON_ONCE() statement for max_pages_per_mr == 1. That 
will make it easy to figure out where to start looking if in the future 
a HCA driver would be added that causes max_pages_per_mr == 1.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1507 bytes --]

On Mon, Apr 25, 2016 at 09:16:18AM -0700, Bart Van Assche wrote:
> On 04/25/2016 08:53 AM, Leon Romanovsky wrote:
> >On Mon, Apr 25, 2016 at 12:46:59PM +0300, Sagi Grimberg wrote:
> >>
> >>>>>>+		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> >>>>>>+				  (ilog2(srp_dev->mr_page_size) - 9);
> >>>>>
> >>>>> From my understanding, srp_dev->max_pages_per_mr can be equal to 1, in
> >>>>>case device will advertise it.
> >>>>>
> >>>>>Is this understanding correct?
> >>>>>Will the code work in such case?
> >>>>
> >>>>Hello Leon,
> >>>>
> >>>>I'm not sure a HCA driver exists for which max_pages_per_mr == 1.
> >>>>Additionally, I'm not sure such a driver would be useful because such a
> >>>>driver would limit the maximum data transfer size for the iSER protocol to 4
> >>>>KB. That is too low to reach good performance.
> >>>
> >>>Bart and Laurence,
> >>>You are raising valuable arguments about sanity of setting srp_dev->max_pages_per_mr
> >>>to be equal 1. I totally agree with you that it is insane, however my
> >>>original question was "does the code support such value?"
> >>
> >>I don't see why we should worry about imaginary devices.
> >
> >It is called corner case and the code supports it.
> 
> I can add a WARN_ON_ONCE() statement for max_pages_per_mr == 1. That will
> make it easy to figure out where to start looking if in the future a HCA
> driver would be added that causes max_pages_per_mr == 1.
> 

Thanks Bart,
It is fair enough.

> Bart.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Sagi Grimberg]

>> I can add a WARN_ON_ONCE() statement for max_pages_per_mr == 1. That will
>> make it easy to figure out where to start looking if in the future a HCA
>> driver would be added that causes max_pages_per_mr == 1.

If we're on the imaginary path, you can flat out fail the host creation.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 04/26/2016 02:08 PM, Sagi Grimberg wrote:
>>> I can add a WARN_ON_ONCE() statement for max_pages_per_mr == 1. That 
>>> will
>>> make it easy to figure out where to start looking if in the future a HCA
>>> driver would be added that causes max_pages_per_mr == 1.
> 
> If we're on the imaginary path, you can flat out fail the host creation.

How about folding the following change into this patch - this change
disables memory registration if max_mr_size <= 1:

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 8ae1543..9072f7f 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3586,7 +3586,7 @@ static void srp_add_one(struct ib_device *device)
 			   IB_DEVICE_MEM_MGT_EXTENSIONS);
 	if (!srp_dev->has_fmr && !srp_dev->has_fr) {
 		dev_warn(&device->dev, "neither FMR nor FR is supported\n");
-	} else if (register_always >= 0) {
+	} else if (register_always >= 0 && device->attrs.max_mr_size > 1) {
 		srp_dev->use_fast_reg = (srp_dev->has_fr &&
 					 (!srp_dev->has_fmr || prefer_fr));
 		srp_dev->use_fmr = !srp_dev->use_fast_reg && srp_dev->has_fmr;

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagi@grimberg.>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Christoph Hellwig]

On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
> If both max_sectors and the queue_depth are high enough it can
> happen that the MR pool is depleted temporarily. This causes
> the SRP initiator to report mapping failures. Although the SRP
> initiator recovers from such mapping failures, prevent that
> this can happen by limiting max_sectors.

FYI, even with this patch I see tons of errors like:

[ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)



> +		/*
> +		 * FR and FMR can only map one HCA page per entry. If the
> +		 * start address is not aligned on a HCA page boundary two
> +		 * entries will be used for the head and the tail although
> +		 * these two entries combined contain at most one HCA page of
> +		 * data. Hence the "- 1" in the calculation below.
> +		 */
> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> +				  (ilog2(srp_dev->mr_page_size) - 9);
> +		if (target->scsi_host->max_sectors > max_max_sectors) {
> +			shost_printk(KERN_WARNING, target->scsi_host,
> +				     PFX "Reducing max_sectors from %d to %d\n",
> +				     target->scsi_host->max_sectors,
> +				     max_max_sectors);
> +			target->scsi_host->max_sectors = max_max_sectors;
> +		}

I don't think there is any good reason to printk a warning here -
limited hardware is a totally normal thing.  E.g. if we merge
your RDMA/CM support and someone runs SRP on chelsio hardware they'd
probably hit this all the time..
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/03/2016 02:33 AM, Christoph Hellwig wrote:
> On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
>> If both max_sectors and the queue_depth are high enough it can
>> happen that the MR pool is depleted temporarily. This causes
>> the SRP initiator to report mapping failures. Although the SRP
>> initiator recovers from such mapping failures, prevent that
>> this can happen by limiting max_sectors.
> 
> FYI, even with this patch I see tons of errors like:
> 
> [ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)

That's unintended. I can reproduce this and will analyze this further.
 
>> +		/*
>> +		 * FR and FMR can only map one HCA page per entry. If the
>> +		 * start address is not aligned on a HCA page boundary two
>> +		 * entries will be used for the head and the tail although
>> +		 * these two entries combined contain at most one HCA page of
>> +		 * data. Hence the "- 1" in the calculation below.
>> +		 */
>> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
>> +				  (ilog2(srp_dev->mr_page_size) - 9);
>> +		if (target->scsi_host->max_sectors > max_max_sectors) {
>> +			shost_printk(KERN_WARNING, target->scsi_host,
>> +				     PFX "Reducing max_sectors from %d to %d\n",
>> +				     target->scsi_host->max_sectors,
>> +				     max_max_sectors);
>> +			target->scsi_host->max_sectors = max_max_sectors;
>> +		}
> 
> I don't think there is any good reason to printk a warning here -
> limited hardware is a totally normal thing.  E.g. if we merge
> your RDMA/CM support and someone runs SRP on chelsio hardware they'd
> probably hit this all the time..

Are you sure? What I see in the v4.6-rc6 tree seems to indicate that Chelsio
hardware supports large page lists:

$ git grep -nHw T[34]_MAX_MR_SIZE
drivers/infiniband/hw/cxgb3/cxio_hal.h:58:#define T3_MAX_MR_SIZE 0x100000000ULL
drivers/infiniband/hw/cxgb3/iwch.c:125:	rnicp->attr.max_mr_size = T3_MAX_MR_SIZE;
drivers/infiniband/hw/cxgb4/provider.c:328:	props->max_mr_size = T4_MAX_MR_SIZE;
drivers/infiniband/hw/cxgb4/t4.h:41:#define T4_MAX_MR_SIZE (~0ULL)

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

----- Original Message -----
> From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> To: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>
> Cc: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, "Laurence Oberman"
> <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Tuesday, May 3, 2016 5:13:32 PM
> Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures
> 
> On 05/03/2016 02:33 AM, Christoph Hellwig wrote:
> > On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
> >> If both max_sectors and the queue_depth are high enough it can
> >> happen that the MR pool is depleted temporarily. This causes
> >> the SRP initiator to report mapping failures. Although the SRP
> >> initiator recovers from such mapping failures, prevent that
> >> this can happen by limiting max_sectors.
> > 
> > FYI, even with this patch I see tons of errors like:
> > 
> > [ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)
> 
> That's unintended. I can reproduce this and will analyze this further.
>  
> >> +		/*
> >> +		 * FR and FMR can only map one HCA page per entry. If the
> >> +		 * start address is not aligned on a HCA page boundary two
> >> +		 * entries will be used for the head and the tail although
> >> +		 * these two entries combined contain at most one HCA page of
> >> +		 * data. Hence the "- 1" in the calculation below.
> >> +		 */
> >> +		max_max_sectors = (srp_dev->max_pages_per_mr - 1) <<
> >> +				  (ilog2(srp_dev->mr_page_size) - 9);
> >> +		if (target->scsi_host->max_sectors > max_max_sectors) {
> >> +			shost_printk(KERN_WARNING, target->scsi_host,
> >> +				     PFX "Reducing max_sectors from %d to %d\n",
> >> +				     target->scsi_host->max_sectors,
> >> +				     max_max_sectors);
> >> +			target->scsi_host->max_sectors = max_max_sectors;
> >> +		}
> > 
> > I don't think there is any good reason to printk a warning here -
> > limited hardware is a totally normal thing.  E.g. if we merge
> > your RDMA/CM support and someone runs SRP on chelsio hardware they'd
> > probably hit this all the time..
> 
> Are you sure? What I see in the v4.6-rc6 tree seems to indicate that Chelsio
> hardware supports large page lists:
> 
> $ git grep -nHw T[34]_MAX_MR_SIZE
> drivers/infiniband/hw/cxgb3/cxio_hal.h:58:#define T3_MAX_MR_SIZE
> 0x100000000ULL
> drivers/infiniband/hw/cxgb3/iwch.c:125:	rnicp->attr.max_mr_size =
> T3_MAX_MR_SIZE;
> drivers/infiniband/hw/cxgb4/provider.c:328:	props->max_mr_size =
> T4_MAX_MR_SIZE;
> drivers/infiniband/hw/cxgb4/t4.h:41:#define T4_MAX_MR_SIZE (~0ULL)
> 
> Bart.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

With the latest patches from Bart, if I keep the max_sectors_kb throttled per the patch, 
I seem to avoid the "ib_srp: Failed to map data (-12)" messages.
Without that patch I can set it to 4M and then immediately will see the failures.
Granted I did not leave it running all of last weekend.

I will go back to it as I already gave a tested by:

Christoph: I think I fixed my mailer per your liking now, I hope so. :)

Thanks
Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Christoph Hellwig]

On Tue, May 03, 2016 at 02:13:32PM -0700, Bart Van Assche wrote:
> > I don't think there is any good reason to printk a warning here -
> > limited hardware is a totally normal thing.  E.g. if we merge
> > your RDMA/CM support and someone runs SRP on chelsio hardware they'd
> > probably hit this all the time..
> 
> Are you sure? What I see in the v4.6-rc6 tree seems to indicate that Chelsio
> hardware supports large page lists:

You're right - I was confused about the small data sizes it allows
without MRs.  Either way, limited hardware on the initiator side is
something totally normal that happens all the time especiall with
SCSI HBAs, so I'd prefer to not spew a warning about it.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/04/2016 02:09 AM, Christoph Hellwig wrote:
> Either way, limited hardware on the initiator side is
> something totally normal that happens all the time especially with
> SCSI HBAs, so I'd prefer to not spew a warning about it.

OK, I will change the shost_printk() into a pr_debug(). That will ensure 
that this message doesn't show up in the system log without enabling 
that message explicitly through configfs.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/03/2016 02:33 AM, Christoph Hellwig wrote:
> On Fri, Apr 22, 2016 at 02:16:31PM -0700, Bart Van Assche wrote:
>> If both max_sectors and the queue_depth are high enough it can
>> happen that the MR pool is depleted temporarily. This causes
>> the SRP initiator to report mapping failures. Although the SRP
>> initiator recovers from such mapping failures, prevent that
>> this can happen by limiting max_sectors.
> 
> FYI, even with this patch I see tons of errors like:
> 
> [ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)

Hello Christoph,

The patch below makes these messages disappear on my setup. Are
you OK with including this change in patch 11/11?

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index fc1e84b..53c8e08 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -3409,6 +3409,12 @@ static ssize_t srp_create_target(struct device *dev,
 
 	target_host->sg_tablesize = target->sg_tablesize;
 	target->mr_pool_size = target->scsi_host->can_queue;
+	/*
+	 * The indirect data buffer descriptor is contiguous so the memory for
+	 * that buffer will only be registered if register_always is true.
+	 */
+	if (register_always)
+		target->mr_pool_size *= 2;
 	target->indirect_size = target->sg_tablesize *
 				sizeof (struct srp_direct_buf);
 	target->max_iu_len = sizeof (struct srp_cmd) +

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Christoph Hellwig]

On Wed, May 04, 2016 at 04:44:53PM -0700, Bart Van Assche wrote:
> > FYI, even with this patch I see tons of errors like:
> > 
> > [ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)
> 
> Hello Christoph,
> 
> The patch below makes these messages disappear on my setup. Are
> you OK with including this change in patch 11/11?

Looks fine to me.

Btw, while we're at it - any chance you could move the fixes before
the cleanups in the series so that they are more easily backportabke to
-stable?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/05/2016 07:06 AM, Christoph Hellwig wrote:
> Btw, while we're at it - any chance you could move the fixes before
> the cleanups in the series so that they are more easily backportabke to
> -stable?

That sounds like a good idea to me. I will do that.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

----- Original Message -----
> From: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>
> To: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
> "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, May 5, 2016 10:06:16 AM
> Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures
> 
> On Wed, May 04, 2016 at 04:44:53PM -0700, Bart Van Assche wrote:
> > > FYI, even with this patch I see tons of errors like:
> > > 
> > > [ 2237.161106] scsi host7: ib_srp: Failed to map data (-12)
> > 
> > Hello Christoph,
> > 
> > The patch below makes these messages disappear on my setup. Are
> > you OK with including this change in patch 11/11?
> 
> Looks fine to me.
> 
> Btw, while we're at it - any chance you could move the fixes before
> the cleanups in the series so that they are more easily backportabke to
> -stable?
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

I added and tested this patch.
I left the warn message in as I need it.

[root@srptest ~]# dmesg | grep Reducing
[  551.526015] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
[  559.367609] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
[  570.449121] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
[  577.831703] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080

Its stable but only because we constrain the max_sectors.
I am writing 4MB buffered to 10 file systems (ext3) however the max_sectors is constrained to 2040k and its stable.

I have been told that RHEL6 and the Mofed stack happily does 4MB buffered I/O with mapping failures but I need to check that out.
That would make no sense to me unless the Mofed stack has code that differs enough to what we have upstream to make a difference.

I am about to install RHEL6 and Mofed to compare and see what happens with 4MB buffered.

Tested-by Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/05/2016 12:58 PM, Laurence Oberman wrote:
> I added and tested this patch.
> I left the warn message in as I need it.
>
> [root@srptest ~]# dmesg | grep Reducing
> [  551.526015] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
> [  559.367609] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> [  570.449121] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> [  577.831703] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080
>
> Its stable but only because we constrain the max_sectors.
> I am writing 4MB buffered to 10 file systems (ext3) however the max_sectors is constrained to 2040k and its stable.
>
> I have been told that RHEL6 and the Mofed stack happily does 4MB buffered I/O with mapping failures but I need to check that out.
> That would make no sense to me unless the Mofed stack has code that differs enough to what we have upstream to make a difference.
>
> I am about to install RHEL6 and Mofed to compare and see what happens with 4MB buffered.
>
> Tested-by Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

Thanks for the help with testing :-)

Can you try to increase SRP_MAX_PAGES_PER_MR in ib_srp.h from 512 to 
1024 and see whether that allows 4MB I/O?

Thanks,

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/05/2016 01:14 PM, Bart Van Assche wrote:
> On 05/05/2016 12:58 PM, Laurence Oberman wrote:
>> I added and tested this patch.
>> I left the warn message in as I need it.
>>
>> [root@srptest ~]# dmesg | grep Reducing
>> [  551.526015] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
>> [  559.367609] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
>> [  570.449121] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
>> [  577.831703] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080
>>
>> Its stable but only because we constrain the max_sectors.
>> I am writing 4MB buffered to 10 file systems (ext3) however the max_sectors is constrained to 2040k and its stable.
>>
>> I have been told that RHEL6 and the Mofed stack happily does 4MB buffered I/O with mapping failures but I need to check that out.
>> That would make no sense to me unless the Mofed stack has code that differs enough to what we have upstream to make a difference.
>>
>> I am about to install RHEL6 and Mofed to compare and see what happens with 4MB buffered.
>>
>> Tested-by Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> Thanks for the help with testing :-)
>
> Can you try to increase SRP_MAX_PAGES_PER_MR in ib_srp.h from 512 to
> 1024 and see whether that allows 4MB I/O?

(replying to my own e-mail)

That was a typo - I meant 1025 instead of 1024.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

----- Original Message -----
> From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> To: "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>
> Cc: "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, May 5, 2016 4:17:51 PM
> Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures
> 
> On 05/05/2016 01:14 PM, Bart Van Assche wrote:
> > On 05/05/2016 12:58 PM, Laurence Oberman wrote:
> >> I added and tested this patch.
> >> I left the warn message in as I need it.
> >>
> >> [root@srptest ~]# dmesg | grep Reducing
> >> [  551.526015] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
> >> [  559.367609] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> >> [  570.449121] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> >> [  577.831703] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080
> >>
> >> Its stable but only because we constrain the max_sectors.
> >> I am writing 4MB buffered to 10 file systems (ext3) however the
> >> max_sectors is constrained to 2040k and its stable.
> >>
> >> I have been told that RHEL6 and the Mofed stack happily does 4MB buffered
> >> I/O with mapping failures but I need to check that out.
> >> That would make no sense to me unless the Mofed stack has code that
> >> differs enough to what we have upstream to make a difference.
> >>
> >> I am about to install RHEL6 and Mofed to compare and see what happens with
> >> 4MB buffered.
> >>
> >> Tested-by Laurence Oberman <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> >
> > Thanks for the help with testing :-)
> >
> > Can you try to increase SRP_MAX_PAGES_PER_MR in ib_srp.h from 512 to
> > 1024 and see whether that allows 4MB I/O?
> 
> (replying to my own e-mail)
> 
> That was a typo - I meant 1025 instead of 1024.
> 
> Bart.
> 
> 
Set at 1025, rebuilt ib_srp module, rebooted client and:

Still get:

[root@srptest ~]# dmesg | grep Reducing
[  227.076550] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
[  235.316995] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
[  245.726223] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
[  253.197573] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080

And then of course we are constrained

[root@srptest queue]# cat max_sectors_kb
2040
[root@srptest queue]# echo 4096 > max_sectors_kb
-bash: echo: write error: Invalid argument

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/05/2016 01:56 PM, Laurence Oberman wrote:
> Set at 1025, rebuilt ib_srp module, rebooted client and:
>
> Still get:
>
> [root@srptest ~]# dmesg | grep Reducing
> [  227.076550] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
> [  235.316995] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> [  245.726223] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> [  253.197573] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080
>
> And then of course we are constrained

Sorry. I forgot that the maximum FR page list length for mlx4 is 511. I 
will come up with another solution.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

----- Original Message -----
> From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> To: "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
> linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, May 5, 2016 5:40:16 PM
> Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures
> 
> On 05/05/2016 01:56 PM, Laurence Oberman wrote:
> > Set at 1025, rebuilt ib_srp module, rebooted client and:
> >
> > Still get:
> >
> > [root@srptest ~]# dmesg | grep Reducing
> > [  227.076550] scsi host4: ib_srp: Reducing max_sectors from 8192 to 4080
> > [  235.316995] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> > [  245.726223] scsi host5: ib_srp: Reducing max_sectors from 8192 to 4080
> > [  253.197573] scsi host6: ib_srp: Reducing max_sectors from 8192 to 4080
> >
> > And then of course we are constrained
> 
> Sorry. I forgot that the maximum FR page list length for mlx4 is 511. I
> will come up with another solution.
> 
> Bart.
> 
> 

Hi Bart

Yep, something has changed upstream here to bring this out.
This time back to testing on mlx5.
I installed RHEL6 + Mofed stack to validate what I was told about RHEL6 + Mofed being stable.

kmod-mlnx-ofa_kernel-3.1-OFED.3.1.1.0.3.1.g9032737.rhel6u7.x86_64
libibverbs-devel-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
libibcm-1.0.5mlnx2-OFED.3.0.11.gd7d485d.x86_64
librdmacm-1.0.21mlnx-OFED.3.0.1.5.2.x86_64
mlnx-ofa_kernel-3.1-OFED.3.1.1.0.3.1.g9032737.rhel6u7.x86_64
libmlx4-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
mlnx-ofa_kernel-devel-3.1-OFED.3.1.1.0.3.1.g9032737.rhel6u7.x86_64
mlnx-ofa_kernel-modules-3.1-2.6.32_573.12.1.el6.x86_64_OFED.3.1.1.0.3.1.g9032737.x86_64
libibcm-devel-1.0.5mlnx2-OFED.3.0.11.gd7d485d.x86_64
libmlx5-devel-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64
librdmacm-devel-1.0.21mlnx-OFED.3.0.1.5.2.x86_64
dapl-devel-2.1.5mlnx-OFED.3.0.0.3.3.x86_64
ibacm-1.0.9mlnx1-OFED.3.0.10.ga105e8b.x86_64
libibverbs-utils-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
libibverbs-1.1.8mlnx1-OFED.3.1.1.0.0.x86_64
kmod-knem-mlnx-1.1.2.90mlnx-OFED.3.1.0.0.7.1.g4b084fc.rhel6u7.x86_64
libmlx4-devel-1.0.6mlnx1-OFED.3.1.1.0.0.x86_64
dapl-2.1.5mlnx-OFED.3.0.0.3.3.x86_64
dapl-utils-2.1.5mlnx-OFED.3.0.0.3.3.x86_64
ofed-scripts-3.1-OFED.3.1.1.0.3.x86_64
librdmacm-utils-1.0.21mlnx-OFED.3.0.1.5.2.x86_64
libmlx5-1.0.2mlnx1-OFED.3.1.1.0.3.x86_64

[root@dhcp40-159 ~]# uname -a
Linux xxxxxxxx 2.6.32-573.12.1.el6.x86_64 #1 SMP Mon Nov 23 12:55:32 EST 2015 x86_64 x86_64 x86_64 GNU/Linux

# cat /etc/redhat-release 
Red Hat Enterprise Linux Server release 6.7 (Santiago)

I can set the max_sector_kb to 4096

Setting max_sectors_kb to 8192 x 512
4096
4096
..
..
4096
4096

When doing buffered I/O I see it periodically get to 4096 and what is most important is that we have no mapping errors so far.
So the messaging I am getting about RHEL6.7 + Mofed stack and stability seems to be valid.

### RECORD  336 >>> dhcp40-159 <<< (1462485288.001) (Thu May  5 21:54:48 2016) ###
# DISK STATISTICS (/sec)
#                   <---------reads---------><---------writes---------><--------averages--------> Pct
#Time     Name       KBytes Merged  IOs Size  KBytes Merged  IOs Size  RWSize  QLen  Wait SvcTim Util
22:00:35 sdk              0      0    0    0  167936      0   41 4096    4096    12   314     24   986
22:00:35 sdg              0      0    0    0   69660      0   18 3870    3870     5   374     25   457
22:00:35 sdl              0      0    0    0  364544      0   92 3962    3962    48   358     10   997
22:00:35 sdc              0      0    0    0  184320      0   47 3922    3921    16   298     21   995

We need to figure out what is different upstream here to get us into trouble I guess with 4MB buffered I/O.
Of course kernel etc. is totally different to what we have on RHEL 6.7.

Thanks
Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/05/2016 03:01 PM, Laurence Oberman wrote:
> Yep, something has changed upstream here to bring this out.

I will modify the SRP initiator such that more mr's are allocated if 
max_sectors > max_sectors_per_mr. That should allow 4 MB I/O even with 
mlx4 and fast registration and without the risk of encountering a 
mapping failure.

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Laurence Oberman]

----- Original Message -----
> From: "Bart Van Assche" <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> To: "Laurence Oberman" <loberman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Cc: "Christoph Hellwig" <hch-jcswGhMUV9g@public.gmane.org>, "Doug Ledford" <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, "Sagi Grimberg" <sagi-NQWnxTmZq1alnMjI0IkVqw@public.gmane.org>,
> linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Sent: Thursday, May 5, 2016 6:46:49 PM
> Subject: Re: [PATCH 11/11] IB/srp: Prevent mapping failures
> 
> On 05/05/2016 03:01 PM, Laurence Oberman wrote:
> > Yep, something has changed upstream here to bring this out.
> 
> I will modify the SRP initiator such that more mr's are allocated if
> max_sectors > max_sectors_per_mr. That should allow 4 MB I/O even with
> mlx4 and fast registration and without the risk of encountering a
> mapping failure.
> 
> Bart.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Hello Bart

I spent the weekend testing the RHEL6 kernel + Mellanox Ofed. Its rock solid.

On both the latest upstream and the RHEL7.2 kernel we have mapping failures with 4MB max_sector_kb sizes.

In addition the latest RHEL 7.2 kernel + Mellanox Ofed sees the same mapping issues.
This seems to suggest its not confined to only code changes Mellanox has in Ofed ib_srp drivers

Are we looking at a combination of kernel changes and ib_srp changes here to correct this.
Or do you think this will be resolved in the end by just changes to the ib_srp driver code.

If there is any other data you want me to capture or debug please do let me know.

I have started investigating the full flow of I/O from the upper layers through the kernel to the ib_srp layer to see what is different in RHEL6 and why this 
issue is not apparent.

Thanks as always for your assistance here.

Laurence
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 11/11] IB/srp: Prevent mapping failures

[Bart Van Assche]

On 05/09/2016 03:50 PM, Laurence Oberman wrote:
> Are we looking at a combination of kernel changes and ib_srp changes here to correct this.

Hello Laurence,

I am working on a patch series that will support a max_sector size of 
4MB and that also will prevent mapping failures. I hope to finish and 
repost that patch series this week. I will CC you on that patch series.

Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/11] More SRP patches for kernel v4.7

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1279 bytes --]

On 04/22/2016 05:11 PM, Bart Van Assche wrote:
> Hello Doug,
> 
> This patch series is what I came up with while retesting the SRP
> initiator. Please consider these patches for inclusion in kernel v4.7.
> Several of the patches in this series had already been posted in October
> 2015. See also http://thread.gmane.org/gmane.linux.drivers.rdma/30216.
> 
> The individual patches in this series are:
> 
> 0001-IB-srp-Fix-a-spelling-error-in-a-source-code-comment.patch
> 0002-IB-srp-Fix-a-comment.patch
> 0003-IB-srp-Document-srp_map_data-return-value.patch
> 0004-IB-srp-Fix-srp_map_data-error-paths.patch
> 0005-IB-srp-Introduce-target-mr_pool_size.patch
> 0006-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
> 0007-IB-srp-Move-code-out-of-a-loop.patch
> 0008-IB-srp-Move-common-code-into-the-caller.patch
> 0009-IB-srp-Fix-a-NULL-pointer-dereference.patch
> 0010-IB-srp-Do-not-register-memory-if-register_always-1.patch
> 0011-IB-srp-Prevent-mapping-failures.patch

Hi Bart,

I've pulled in the first 8 of these patches.  The last three all had
comments.  When you respin those, just spin those and not the first 8.

Thanks!

-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH 0/11] More SRP patches for kernel v4.7

[Bart Van Assche]

On 05/05/2016 01:44 PM, Doug Ledford wrote:
> On 04/22/2016 05:11 PM, Bart Van Assche wrote:
>> Hello Doug,
>>
>> This patch series is what I came up with while retesting the SRP
>> initiator. Please consider these patches for inclusion in kernel v4.7.
>> Several of the patches in this series had already been posted in October
>> 2015. See also http://thread.gmane.org/gmane.linux.drivers.rdma/30216.
>>
>> The individual patches in this series are:
>>
>> 0001-IB-srp-Fix-a-spelling-error-in-a-source-code-comment.patch
>> 0002-IB-srp-Fix-a-comment.patch
>> 0003-IB-srp-Document-srp_map_data-return-value.patch
>> 0004-IB-srp-Fix-srp_map_data-error-paths.patch
>> 0005-IB-srp-Introduce-target-mr_pool_size.patch
>> 0006-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
>> 0007-IB-srp-Move-code-out-of-a-loop.patch
>> 0008-IB-srp-Move-common-code-into-the-caller.patch
>> 0009-IB-srp-Fix-a-NULL-pointer-dereference.patch
>> 0010-IB-srp-Do-not-register-memory-if-register_always-1.patch
>> 0011-IB-srp-Prevent-mapping-failures.patch
>
> Hi Bart,
>
> I've pulled in the first 8 of these patches.  The last three all had
> comments.  When you respin those, just spin those and not the first 8.

Hello Doug,

Can you check whether patch "IB/srp: Fix a debug kernel crash" is 
already in your tree 
(http://thread.gmane.org/gmane.linux.drivers.rdma/35409)?

Thanks,

Bart.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html