IB/srp receive buffer handling robustness improvement

PATCH In review Bart Van Assche 3 participants 3 messages Jul 30, 2010 – Aug 4, 2010 1 message not in the archive

Subsystems (2)

InfiniBand SCSI RDMA protocol (SRP) initiator

lore

Share & tools

lore

Bart Van Assche

author

2010-07-30 10:59 UTC #0 lore raw

PATCH 1/1 IB/srp receive buffer handling robustness improvement

The current strategy in ib_srp for posting receive buffers is as follows:
* Post one buffer after channel establishment.
* Post one buffer before sending an SRP_CMD or SRP_TSK_MGMT to the target. As a result, only the first non-SRP_RSP information unit from the target will be processed. If that first information unit is an SRP_T_LOGOUT, it will be processed. On the other hand, if the initiator receives an SRP_CRED_REQ or SRP_AER_REQ before it receives a SRP_T_LOGOUT, the SRP_T_LOGOUT won't be processed. This patch fixes this inconsistency by changing the strategy for posting receive buffers as follows:
* Post all receive buffers after channel establishment.
* After a receive buffer has been consumed and processed, post it again. A side effect is that the ib_post_recv() call is moved out of the SCSI command processing path.

Signed-off-by Bart Van Assche <[email protected]>

Cc Roland Dreier <[email protected]>

Cc David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

drivers/infiniband/ulp/srp/ib_srp.c +13 -6

index ed3f9eb..b9a38c2 100644
@@ -82,6 +82,7 @@ static void srp_add_one(struct ib_device *device);
 static void srp_remove_one(struct ib_device *device);
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr);
 static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
+static int srp_post_recv(struct srp_target_port *target);
 static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
 
 static struct scsi_transport_template *ib_srp_transport_template;

@@ -868,6 +869,7 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
 	struct srp_iu *iu;
+	int res;
 	u8 opcode;
 
 	iu = target->rx_ring[wc->wr_id];

@@ -915,6 +917,11 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 
 	ib_dma_sync_single_for_device(dev, iu->dma, target->max_ti_iu_len,
 				      DMA_FROM_DEVICE);
+
+	res = srp_post_recv(target);
+	if (res != 0)
+		shost_printk(KERN_ERR, target->scsi_host,
+			     PFX "Recv failed with error code %d\n", res);
 }
 
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)

@@ -1102,11 +1109,6 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		goto err;
 	}
 
-	if (__srp_post_recv(target)) {
-		shost_printk(KERN_ERR, target->scsi_host, PFX "Recv failed\n");
-		goto err_unmap;
-	}
-
 	ib_dma_sync_single_for_device(dev, iu->dma, srp_max_iu_len,
 				      DMA_TO_DEVICE);

@@ -1249,6 +1251,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 	int attr_mask = 0;
 	int comp = 0;
 	int opcode = 0;
+	int i;
 
 	switch (event->event) {
 	case IB_CM_REQ_ERROR:

@@ -1298,7 +1301,11 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 		if (target->status)
 			break;
 
-		target->status = srp_post_recv(target);
+		for (i = 0; i < SRP_RQ_SIZE; i++) {
+			target->status = srp_post_recv(target);
+			if (target->status)
+				break;
+		}
 		if (target->status)
 			break;

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

David Dillow

Acked-by

2010-07-30 15:36 UTC #2 lore raw

Quoted 5 lines · Bart Van Assche

The current strategy in ib_srp for posting receive buffers is as follows:
* Post one buffer after channel establishment.
* Post one buffer before sending an SRP_CMD or SRP_TSK_MGMT to the target. As a result, only the first non-SRP_RSP information unit from the target will be processed. If that first information unit is an SRP_T_LOGOUT, it will be

Quoted 8 lines

processed. On the other hand, if the initiator receives an SRP_CRED_REQ or SRP_AER_REQ before it receives a SRP_T_LOGOUT, the SRP_T_LOGOUT won't be processed. This patch fixes this inconsistency by changing the strategy for posting receive buffers as follows:
* Post all receive buffers after channel establishment.
* After a receive buffer has been consumed and processed, post it again. A side effect is that the ib_post_recv() call is moved out of the SCSI command processing path.

Some comments below, but fixed or no,

Acked-by David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

Quoted 2 lines

static void srp_send_completion(struct ib_cq *cq, void *target_ptr);
+static int srp_post_recv(struct srp_target_port *target);

I think you can avoid adding this declaration by hoisting __srp_post_recv() and srp_post_recv() above srp_handle_recv(). I believe Roland shares my preference to avoid forward declarations if possible.

drivers/infiniband/ulp/srp/ib_srp.c static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event) · line 1308

Quoted 10 lines

drivers/infiniband/ulp/srp/ib_srp.c +5 -1

@@ -1298,7 +1301,11 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 		if (target->status)
 			break;
 
-		target->status = srp_post_recv(target);
+		for (i = 0; i < SRP_RQ_SIZE; i++) {
+			target->status = srp_post_recv(target);
+			if (target->status)
+				break;
+		}

Hmm, target setup is getting a bit wordy in that switch statement. It's probably getting to be time to move it to a separate function, but that doesn't need to happen in this patch.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Roland Dreier

Acked-by David Dillow

2010-08-04 18:45 UTC #3 lore raw

In reply to a message not in the archive

Thanks, I combined these patches and also got rid of __srp_post_recv() since we don't need the split anymore... so I queued the patch below. (Bart, by the way, this patch didn't have a "---" after the changelog part, and also didn't have a diffstat for the patch. If you use git, then "git format-patch" will generate a patch without you having to worry about these trivial details)

IB/srp: Make receive buffer handling more robust

The current strategy in ib_srp for posting receive buffers is:

* Post one buffer after channel establishment.
* Post one buffer before sending an SRP_CMD or SRP_TSK_MGMT to the target.

As a result, only the first non-SRP_RSP information unit from the target will be processed. If that first information unit is an SRP_T_LOGOUT, it will be processed. On the other hand, if the initiator receives an SRP_CRED_REQ or SRP_AER_REQ before it receives a SRP_T_LOGOUT, the SRP_T_LOGOUT won't be processed.

We can fix this inconsistency by changing the strategy for posting receive buffers to:

* Post all receive buffers after channel establishment.
* After a receive buffer has been consumed and processed, post it again.

A side effect is that the ib_post_recv() call is moved out of the SCSI command processing path. Since __srp_post_recv() is not called directly any more, get rid of it and move the code directly into srp_post_recv(). Also, move srp_post_recv() up in the file to avoid a forward declaration.

Signed-off-by Bart Van Assche <[email protected]>

Acked-by David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>

Signed-off-by Roland Dreier <[email protected]>

---
 drivers/infiniband/ulp/srp/ib_srp.c |   89 +++++++++++++++++------------------
 1 files changed, 44 insertions(+), 45 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +44 ~~-45~~

index 4675def..ffdd2d1 100644
@@ -811,6 +811,38 @@ static int srp_map_data(struct scsi_cmnd *scmnd, struct srp_target_port *target,
 	return len;
 }
 
+static int srp_post_recv(struct srp_target_port *target)
+{
+	unsigned long flags;
+	struct srp_iu *iu;
+	struct ib_sge list;
+	struct ib_recv_wr wr, *bad_wr;
+	unsigned int next;
+	int ret;
+
+	spin_lock_irqsave(target->scsi_host->host_lock, flags);
+
+	next	 = target->rx_head & (SRP_RQ_SIZE - 1);
+	wr.wr_id = next;
+	iu	 = target->rx_ring[next];
+
+	list.addr   = iu->dma;
+	list.length = iu->size;
+	list.lkey   = target->srp_host->srp_dev->mr->lkey;
+
+	wr.next     = NULL;
+	wr.sg_list  = &list;
+	wr.num_sge  = 1;
+
+	ret = ib_post_recv(target->qp, &wr, &bad_wr);
+	if (!ret)
+		++target->rx_head;
+
+	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
+
+	return ret;
+}
+
 static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 {
 	struct srp_request *req;

@@ -868,6 +900,7 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
 	struct srp_iu *iu;
+	int res;
 	u8 opcode;
 
 	iu = target->rx_ring[wc->wr_id];

@@ -904,6 +937,11 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 
 	ib_dma_sync_single_for_device(dev, iu->dma, target->max_ti_iu_len,
 				      DMA_FROM_DEVICE);
+
+	res = srp_post_recv(target);
+	if (res != 0)
+		shost_printk(KERN_ERR, target->scsi_host,
+			     PFX "Recv failed with error code %d\n", res);
 }
 
 static void srp_recv_completion(struct ib_cq *cq, void *target_ptr)

@@ -943,45 +981,6 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 	}
 }
 
-static int __srp_post_recv(struct srp_target_port *target)
-{
-	struct srp_iu *iu;
-	struct ib_sge list;
-	struct ib_recv_wr wr, *bad_wr;
-	unsigned int next;
-	int ret;
-
-	next 	 = target->rx_head & (SRP_RQ_SIZE - 1);
-	wr.wr_id = next;
-	iu 	 = target->rx_ring[next];
-
-	list.addr   = iu->dma;
-	list.length = iu->size;
-	list.lkey   = target->srp_host->srp_dev->mr->lkey;
-
-	wr.next     = NULL;
-	wr.sg_list  = &list;
-	wr.num_sge  = 1;
-
-	ret = ib_post_recv(target->qp, &wr, &bad_wr);
-	if (!ret)
-		++target->rx_head;
-
-	return ret;
-}
-
-static int srp_post_recv(struct srp_target_port *target)
-{
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(target->scsi_host->host_lock, flags);
-	ret = __srp_post_recv(target);
-	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
-
-	return ret;
-}
-
 /*
  * Must be called with target->scsi_host->host_lock held to protect
  * req_lim and tx_head.  Lock cannot be dropped between call here and

@@ -1091,11 +1090,6 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		goto err;
 	}
 
-	if (__srp_post_recv(target)) {
-		shost_printk(KERN_ERR, target->scsi_host, PFX "Recv failed\n");
-		goto err_unmap;
-	}
-
 	ib_dma_sync_single_for_device(dev, iu->dma, srp_max_iu_len,
 				      DMA_TO_DEVICE);

@@ -1238,6 +1232,7 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 	int attr_mask = 0;
 	int comp = 0;
 	int opcode = 0;
+	int i;
 
 	switch (event->event) {
 	case IB_CM_REQ_ERROR:

@@ -1287,7 +1282,11 @@ static int srp_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event)
 		if (target->status)
 			break;
 
-		target->status = srp_post_recv(target);
+		for (i = 0; i < SRP_RQ_SIZE; i++) {
+			target->status = srp_post_recv(target);
+			if (target->status)
+				break;
+		}
 		if (target->status)
 			break;

Signature · 10 lines

1.7.2


-- 
Roland Dreier <[email protected]> || For corporate legal information go to:
http://www.cisco.com/web/about/doing_business/legal/cri/index.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html