[PATCH 0/7] Fix an infinite loop in the SRP initiator

[PATCH 0/7] Fix an infinite loop in the SRP initiator

[Bart Van Assche]

Submitting a SCSI request through the SG_IO mechanism with a scatterlist 
that is longer than what is supported by the SRP initiator triggers an 
infinite loop. This patch series fixes that behavior.

The individual patches in this series are as follows:

0001-IB-srp-Fix-a-spelling-error.patch
0002-IB-srp-Document-srp_map_data-return-value.patch
0003-IB-srp-Rename-work-request-ID-labels.patch
0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch
0005-IB-srp-Fix-srp_map_data-error-paths.patch
0006-IB-srp-Introduce-target-mr_pool_size.patch
0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.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 1/7] IB/srp: Fix a spelling error

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 d01395b..1c94d93 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1407,7 +1407,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.1.4

--
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 2/7] IB/srp: Document srp_map_data() return value

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 1c94d93..c1faf70 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1497,6 +1497,15 @@ out:
 	return ret;
 }
 
+/**
+ * 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.1.4

--
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 3/7] IB/srp: Rename work request ID labels

[Bart Van Assche]

Work request IDs in the SRP initiator driver are either a pointer or a
value that is not a valid pointer. Since the local invalidate and fast
registration work requests IDs are not used as masks drop the suffix
"mask" from their name.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 8 ++++----
 drivers/infiniband/ulp/srp/ib_srp.h | 7 +++----
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index c1faf70..1aa9a4c 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1049,7 +1049,7 @@ static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)
 	struct ib_send_wr *bad_wr;
 	struct ib_send_wr wr = {
 		.opcode		    = IB_WR_LOCAL_INV,
-		.wr_id		    = LOCAL_INV_WR_ID_MASK,
+		.wr_id		    = LOCAL_INV_WR_ID,
 		.next		    = NULL,
 		.num_sge	    = 0,
 		.send_flags	    = 0,
@@ -1325,7 +1325,7 @@ static int srp_map_finish_fr(struct srp_map_state *state,
 
 	memset(&wr, 0, sizeof(wr));
 	wr.opcode = IB_WR_FAST_REG_MR;
-	wr.wr_id = FAST_REG_WR_ID_MASK;
+	wr.wr_id = FAST_REG_WR_ID;
 	wr.wr.fast_reg.iova_start = state->base_dma_addr;
 	wr.wr.fast_reg.page_list = desc->frpl;
 	wr.wr.fast_reg.page_list_len = state->npages;
@@ -1940,11 +1940,11 @@ static void srp_handle_qp_err(u64 wr_id, enum ib_wc_status wc_status,
 	}
 
 	if (ch->connected && !target->qp_in_error) {
-		if (wr_id & LOCAL_INV_WR_ID_MASK) {
+		if (wr_id == LOCAL_INV_WR_ID) {
 			shost_printk(KERN_ERR, target->scsi_host, PFX
 				     "LOCAL_INV failed with status %s (%d)\n",
 				     ib_wc_status_msg(wc_status), wc_status);
-		} else if (wr_id & FAST_REG_WR_ID_MASK) {
+		} else if (wr_id == FAST_REG_WR_ID) {
 			shost_printk(KERN_ERR, target->scsi_host, PFX
 				     "FAST_REG_MR failed status %s (%d)\n",
 				     ib_wc_status_msg(wc_status), wc_status);
diff --git a/drivers/infiniband/ulp/srp/ib_srp.h b/drivers/infiniband/ulp/srp/ib_srp.h
index 3608f2e..1c6a715 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -67,10 +67,9 @@ enum {
 
 	SRP_MAX_PAGES_PER_MR	= 512,
 
-	LOCAL_INV_WR_ID_MASK	= 1,
-	FAST_REG_WR_ID_MASK	= 2,
-
-	SRP_LAST_WR_ID		= 0xfffffffcU,
+	LOCAL_INV_WR_ID		= 1,
+	FAST_REG_WR_ID		= 2,
+	SRP_LAST_WR_ID		= 3,
 };
 
 enum srp_target_state {
-- 
2.1.4

--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Bart Van Assche]

Wait until memory registration has finished in the srp_queuecommand()
error path before invalidating memory regions to avoid a send queue
overflow.

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 41 ++++++++++++++++++++++++++++++-------
 1 file changed, 34 insertions(+), 7 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 1aa9a4c..6d17fe2 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1044,7 +1044,7 @@ static int srp_connect_ch(struct srp_rdma_ch *ch, bool multich)
 	}
 }
 
-static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)
+static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey, u32 send_flags)
 {
 	struct ib_send_wr *bad_wr;
 	struct ib_send_wr wr = {
@@ -1052,16 +1052,32 @@ static int srp_inv_rkey(struct srp_rdma_ch *ch, u32 rkey)
 		.wr_id		    = LOCAL_INV_WR_ID,
 		.next		    = NULL,
 		.num_sge	    = 0,
-		.send_flags	    = 0,
+		.send_flags	    = send_flags,
 		.ex.invalidate_rkey = rkey,
 	};
 
 	return ib_post_send(ch->qp, &wr, &bad_wr);
 }
 
+static bool srp_wait_until_done(struct srp_rdma_ch *ch, int i, long timeout)
+{
+	WARN_ON_ONCE(timeout <= 0);
+
+	for ( ; i > 0; i--) {
+		spin_lock_irq(&ch->lock);
+		srp_send_completion(ch->send_cq, ch);
+		spin_unlock_irq(&ch->lock);
+
+		if (wait_for_completion_timeout(&ch->done, timeout) > 0)
+			return true;
+	}
+	return false;
+}
+
 static void srp_unmap_data(struct scsi_cmnd *scmnd,
 			   struct srp_rdma_ch *ch,
-			   struct srp_request *req)
+			   struct srp_request *req,
+			   bool wait_for_first_unmap)
 {
 	struct srp_target_port *target = ch->target;
 	struct srp_device *dev = target->srp_host->srp_dev;
@@ -1077,13 +1093,19 @@ static void srp_unmap_data(struct scsi_cmnd *scmnd,
 		struct srp_fr_desc **pfr;
 
 		for (i = req->nmdesc, pfr = req->fr_list; i > 0; i--, pfr++) {
-			res = srp_inv_rkey(ch, (*pfr)->mr->rkey);
+			res = srp_inv_rkey(ch, (*pfr)->mr->rkey,
+					   wait_for_first_unmap ?
+					   IB_SEND_SIGNALED : 0);
 			if (res < 0) {
 				shost_printk(KERN_ERR, target->scsi_host, PFX
 				  "Queueing INV WR for rkey %#x failed (%d)\n",
 				  (*pfr)->mr->rkey, res);
 				queue_work(system_long_wq,
 					   &target->tl_err_work);
+			} else if (wait_for_first_unmap) {
+				wait_for_first_unmap = false;
+				WARN_ON_ONCE(!srp_wait_until_done(ch, 10,
+							msecs_to_jiffies(100)));
 			}
 		}
 		if (req->nmdesc)
@@ -1144,7 +1166,7 @@ static void srp_free_req(struct srp_rdma_ch *ch, struct srp_request *req,
 {
 	unsigned long flags;
 
-	srp_unmap_data(scmnd, ch, req);
+	srp_unmap_data(scmnd, ch, req, false);
 
 	spin_lock_irqsave(&ch->lock, flags);
 	ch->req_lim += req_lim_delta;
@@ -1982,7 +2004,12 @@ static void srp_send_completion(struct ib_cq *cq, void *ch_ptr)
 	struct srp_iu *iu;
 
 	while (ib_poll_cq(cq, 1, &wc) > 0) {
-		if (likely(wc.status == IB_WC_SUCCESS)) {
+		if (unlikely(wc.wr_id == LOCAL_INV_WR_ID)) {
+			complete(&ch->done);
+			if (wc.status != IB_WC_SUCCESS)
+				srp_handle_qp_err(wc.wr_id, wc.status, true,
+						  ch);
+		} else if (likely(wc.status == IB_WC_SUCCESS)) {
 			iu = (struct srp_iu *) (uintptr_t) wc.wr_id;
 			list_add(&iu->list, &ch->free_tx);
 		} else {
@@ -2084,7 +2111,7 @@ unlock_rport:
 	return ret;
 
 err_unmap:
-	srp_unmap_data(scmnd, ch, req);
+	srp_unmap_data(scmnd, ch, req, true);
 
 err_iu:
 	srp_put_tx_iu(ch, iu, SRP_IU_CMD);
-- 
2.1.4

--
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 5/7] 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: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 6d17fe2..fb6b654 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1473,7 +1473,6 @@ static int srp_map_sg(struct srp_map_state *state, struct srp_rdma_ch *ch,
 		}
 	}
 
-	req->nmdesc = state->nmdesc;
 	ret = 0;
 
 out:
@@ -1594,7 +1593,10 @@ 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));
-	srp_map_sg(&state, ch, req, scat, count);
+	ret = srp_map_sg(&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
@@ -1617,7 +1619,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);
@@ -1635,7 +1638,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 = target->global_mr->rkey;
@@ -1661,6 +1664,10 @@ map_complete:
 		cmd->buf_fmt = fmt;
 
 	return len;
+
+unmap:
+	srp_unmap_data(scmnd, ch, req, true);
+	return ret;
 }
 
 /*
-- 
2.1.4

--
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 6/7] 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>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 fb6b654..47c3a72 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -315,7 +315,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;
@@ -449,8 +449,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);
 }
 
@@ -3247,6 +3246,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 1c6a715..af084f7 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.h
+++ b/drivers/infiniband/ulp/srp/ib_srp.h
@@ -205,6 +205,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.1.4

--
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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Bart Van Assche]

Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 47c3a72..59d3ff9 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1666,6 +1666,8 @@ map_complete:
 
 unmap:
 	srp_unmap_data(scmnd, ch, req, true);
+	if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
+		ret = -E2BIG;
 	return ret;
 }
 
-- 
2.1.4

--
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/7] Fix an infinite loop in the SRP initiator

[Sagi Grimberg]

> Submitting a SCSI request through the SG_IO mechanism with a scatterlist
> that is longer than what is supported by the SRP initiator triggers an
> infinite loop. This patch series fixes that behavior.
>
> The individual patches in this series are as follows:
>
> 0001-IB-srp-Fix-a-spelling-error.patch
> 0002-IB-srp-Document-srp_map_data-return-value.patch
> 0003-IB-srp-Rename-work-request-ID-labels.patch
> 0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch
> 0005-IB-srp-Fix-srp_map_data-error-paths.patch
> 0006-IB-srp-Introduce-target-mr_pool_size.patch
> 0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch

Hi Bart,

I didn't look at the patches yet but are these patches on top of
the registration changes I posted for 4.4? I am hoping Doug will merge
them so if they aren't it might be a good idea to respin the series on
top of that.

Cheers,
Sagi.
--
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/7] Fix an infinite loop in the SRP initiator

[Bart Van Assche]

On 10/28/2015 03:32 AM, Sagi Grimberg wrote:
>> Submitting a SCSI request through the SG_IO mechanism with a scatterlist
>> that is longer than what is supported by the SRP initiator triggers an
>> infinite loop. This patch series fixes that behavior.
>>
>> The individual patches in this series are as follows:
>>
>> 0001-IB-srp-Fix-a-spelling-error.patch
>> 0002-IB-srp-Document-srp_map_data-return-value.patch
>> 0003-IB-srp-Rename-work-request-ID-labels.patch
>> 0004-IB-srp-Fix-a-potential-queue-overflow-in-an-error-pa.patch
>> 0005-IB-srp-Fix-srp_map_data-error-paths.patch
>> 0006-IB-srp-Introduce-target-mr_pool_size.patch
>> 0007-IB-srp-Avoid-that-mapping-failure-triggers-an-infini.patch
>
> I didn't look at the patches yet but are these patches on top of
> the registration changes I posted for 4.4? I am hoping Doug will merge
> them so if they aren't it might be a good idea to respin the series on
> top of that.

Hello Sagi,

These patches have been prepared on top of v4.3-rc7. Doug, did I see 
correctly that you have not yet queued Sagi's registration API changes 
for the v4.4 merge window ?

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 1/7] IB/srp: Fix a spelling error

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@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 2/7] IB/srp: Document srp_map_data() return value

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@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 3/7] IB/srp: Rename work request ID labels

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Sagi Grimberg]

On 28/10/2015 00:02, Bart Van Assche wrote:
> Wait until memory registration has finished in the srp_queuecommand()
> error path before invalidating memory regions to avoid a send queue
> overflow.

This looks backwards to me... Why do we even post anything on our
queue-pair to begin with if we got an unsupported sg list?

Can't we perform a simple sanity check on the sg list 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

Re: [PATCH 6/7] IB/srp: Introduce target->mr_pool_size

[Sagi Grimberg]

Looks good,

Reviewed-by: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Sagi Grimberg]

Can you spare a few words on this change in the change log?


> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 47c3a72..59d3ff9 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1666,6 +1666,8 @@ map_complete:
>
>   unmap:
>   	srp_unmap_data(scmnd, ch, req, true);
> +	if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
> +		ret = -E2BIG;
>   	return ret;
>   }

Why return E2BIG for ENOMEM as well?
--
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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Bart Van Assche]

On 11/03/2015 09:44 AM, Sagi Grimberg wrote:
> Can you spare a few words on this change in the change log?
>
>
>> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 47c3a72..59d3ff9 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -1666,6 +1666,8 @@ map_complete:
>>
>>    unmap:
>>    	srp_unmap_data(scmnd, ch, req, true);
>> +	if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
>> +		ret = -E2BIG;
>>    	return ret;
>>    }
>
> Why return E2BIG for ENOMEM as well?

Hello Sagi,

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. 
This is why this patch fixes an infinite loop. I will add this 
explanation to the patch description when I repost 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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Sagi Grimberg]

On 03/11/2015 20:56, Bart Van Assche wrote:
> On 11/03/2015 09:44 AM, Sagi Grimberg wrote:
>> Can you spare a few words on this change in the change log?
>>
>>
>>> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>>> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 47c3a72..59d3ff9 100644
>>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>>> @@ -1666,6 +1666,8 @@ map_complete:
>>>
>>>    unmap:
>>>        srp_unmap_data(scmnd, ch, req, true);
>>> +    if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
>>> +        ret = -E2BIG;
>>>        return ret;
>>>    }
>>
>> Why return E2BIG for ENOMEM as well?
>
> Hello Sagi,
>
> 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.

That's what I meant, ENOMEM is transient by nature. Why would you
not want the scsi layer to requeue? I do understand this for the nmdesc
condition but for the ENOMEM?
--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Bart Van Assche]

On 11/03/2015 09:36 AM, Sagi Grimberg wrote:
> On 28/10/2015 00:02, Bart Van Assche wrote:
>> Wait until memory registration has finished in the srp_queuecommand()
>> error path before invalidating memory regions to avoid a send queue
>> overflow.
>
> This looks backwards to me... Why do we even post anything on our
> queue-pair to begin with if we got an unsupported sg list?
>
> Can't we perform a simple sanity check on the sg list instead?

Hello Sagi,

There is one memory descriptor pool per RDMA channel and RDMA channels 
are typically used by more than one CPU. This means that memory 
registration for more than one command can happen concurrently from a 
single memory descriptor pool. This is why checking how many memory 
descriptors are left before memory registration occurs wouldn't be 
sufficient.

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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Bart Van Assche]

On 11/03/2015 10:59 AM, Sagi Grimberg wrote:
> On 03/11/2015 20:56, Bart Van Assche wrote:
>> On 11/03/2015 09:44 AM, Sagi Grimberg wrote:
>>> Can you spare a few words on this change in the change log?
>>>
>>>
>>>> Signed-off-by: Bart Van Assche <bart.vanassche-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
>>>> Cc: Sagi Grimberg <sagig-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>>>> Cc: Sebastian Parschauer <sebastian.riemer-EIkl63zCoXaH+58JC4qpiA@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 47c3a72..59d3ff9 100644
>>>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>>>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>>>> @@ -1666,6 +1666,8 @@ map_complete:
>>>>
>>>>     unmap:
>>>>         srp_unmap_data(scmnd, ch, req, true);
>>>> +    if (ret == -ENOMEM && req->nmdesc >= target->mr_pool_size)
>>>> +        ret = -E2BIG;
>>>>         return ret;
>>>>     }
>>>
>>> Why return E2BIG for ENOMEM as well?
>>
>> Hello Sagi,
>>
>> 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.
>
> That's what I meant, ENOMEM is transient by nature. Why would you
> not want the scsi layer to requeue? I do understand this for the nmdesc
> condition but for the ENOMEM?

Hello Sagi,

Since the ret == -ENOMEM test is redundant in the above code that test 
can be left out. Would that make this patch more clear to you ?

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 7/7] IB/srp: Avoid that mapping failure triggers an infinite loop

[Sagi Grimberg]

> Hello Sagi,
>
> Since the ret == -ENOMEM test is redundant in the above code that test
> can be left out. Would that make this patch more clear to you ?

It will.

Thanks,
Sagi.
--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Sagi Grimberg]

On 03/11/2015 21:04, Bart Van Assche wrote:
> On 11/03/2015 09:36 AM, Sagi Grimberg wrote:
>> On 28/10/2015 00:02, Bart Van Assche wrote:
>>> Wait until memory registration has finished in the srp_queuecommand()
>>> error path before invalidating memory regions to avoid a send queue
>>> overflow.
>>
>> This looks backwards to me... Why do we even post anything on our
>> queue-pair to begin with if we got an unsupported sg list?
>>
>> Can't we perform a simple sanity check on the sg list instead?
>
> Hello Sagi,
>
> There is one memory descriptor pool per RDMA channel and RDMA channels
> are typically used by more than one CPU. This means that memory
> registration for more than one command can happen concurrently from a
> single memory descriptor pool.
> This is why checking how many memory
> descriptors are left before memory registration occurs wouldn't be
> sufficient.

How harm would it do to make the entire SG list mapping channel-wide
atomic? I'm asking because it's really a non-trivial flow you're
introducing.

Sagi.
--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Bart Van Assche]

On 11/03/2015 11:56 AM, Sagi Grimberg wrote:
> On 03/11/2015 21:04, Bart Van Assche wrote:
>> On 11/03/2015 09:36 AM, Sagi Grimberg wrote:
>>> On 28/10/2015 00:02, Bart Van Assche wrote:
>>>> Wait until memory registration has finished in the srp_queuecommand()
>>>> error path before invalidating memory regions to avoid a send queue
>>>> overflow.
>>>
>>> This looks backwards to me... Why do we even post anything on our
>>> queue-pair to begin with if we got an unsupported sg list?
>>>
>>> Can't we perform a simple sanity check on the sg list instead?
>>
>> Hello Sagi,
>>
>> There is one memory descriptor pool per RDMA channel and RDMA channels
>> are typically used by more than one CPU. This means that memory
>> registration for more than one command can happen concurrently from a
>> single memory descriptor pool.
>> This is why checking how many memory
>> descriptors are left before memory registration occurs wouldn't be
>> sufficient.
>
> How harm would it do to make the entire SG list mapping channel-wide
> atomic? I'm asking because it's really a non-trivial flow you're
> introducing.

Hello Sagi,

Sorry but I strongly prefer not to introduce new contention points in 
the SRP initiator driver.

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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Sagi Grimberg]

> Hello Sagi,
>
> Sorry but I strongly prefer not to introduce new contention points in
> the SRP initiator driver.

Yea... Me neither.

What if you just have a simple check on the SG list and reserve the
required descriptors up front? Would that work?
--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Bart Van Assche]

On 11/03/2015 12:13 PM, Sagi Grimberg wrote:
>> Sorry but I strongly prefer not to introduce new contention points in
>> the SRP initiator driver.
>
> Yea... Me neither.
>
> What if you just have a simple check on the SG list and reserve the
> required descriptors up front? Would that work?

Such a check wouldn't be that simple because the only way to perform 
such a check is either by doubling the number of ib_map_mr_sg() calls or 
by performing additional memory allocations.

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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Christoph Hellwig]

On Tue, Nov 03, 2015 at 12:50:59PM -0800, Bart Van Assche wrote:
> Such a check wouldn't be that simple because the only way to perform such a
> check is either by doubling the number of ib_map_mr_sg() calls or by
> performing additional memory allocations.

The other option woud be to disallow gappy SG lists unless supported by
the hardware with a single MR similar to iSER.  While this would leave
the SRP protocol support for multiple SG entries per command unused it
would significantly simplify the driver.
--
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 4/7] IB/srp: Fix a potential queue overflow in an error path

[Bart Van Assche]

On 11/03/2015 08:03 PM, Christoph Hellwig wrote:
> On Tue, Nov 03, 2015 at 12:50:59PM -0800, Bart Van Assche wrote:
>> Such a check wouldn't be that simple because the only way to perform such a
>> check is either by doubling the number of ib_map_mr_sg() calls or by
>> performing additional memory allocations.
>
> The other option woud be to disallow gappy SG lists unless supported by
> the hardware with a single MR similar to iSER.  While this would leave
> the SRP protocol support for multiple SG entries per command unused it
> would significantly simplify the driver.

Hello Christoph,

I will have a look into this after the kernel 4.4 merge window has been 
closed.

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