Skip to contentSkip to conversation

IB/srp: Implement SRP_CRED_REQ

PATCH Superseded Bart Van Assche 2 participants 5 messages Aug 10, 2010 – Aug 14, 2010 3 messages not in the archive

Subsystems (3)

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore
#0 lore raw

PATCH v4 2/3 IB/srp: Implement SRP_CRED_REQ

Implements SRP_CRED_REQ, which is an information unit defined in the SRP (draft) standard and that allows an SRP target to inform an SRP initiator that more requests may be sent by the initiator. Adds declarations for the SRP_CRED_REQ and SRP_CRED_RSP information units to include/scsi/srp.h.

Cc Roland Dreier <[email protected]>
Cc David Dillow <dave-i1Mk8JYDVaaSihdK6806/[email protected]>
---
 drivers/infiniband/ulp/srp/ib_srp.c |  222 +++++++++++++++++++++++++----------
 drivers/infiniband/ulp/srp/ib_srp.h |    7 +-
 include/scsi/srp.h                  |   14 +++
 3 files changed, 177 insertions(+), 66 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +159 -63
index 6946012..cc6a713 100644
@@ -898,6 +898,157 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
 }
 
+/*
+ * Must be called with target->scsi_host->host_lock held to protect
+ * tx_head.  Lock cannot be dropped between call here and call to
+ * __srp_post_send_iu().
+ *
+ * Note:
+ * An upper limit for the number of allocated information units for each
+ * request type is:
+ * - SRP_TX_IU_REQ_NORMAL: SRP_REQ_SQ_SIZE - SRP_TSK_MGMT_RSV, since the
+ *   SCSI mid-layer never queues more than Scsi_Host.can_queue requests.
+ * - SRP_TX_IU_REQ_TASK_MGMT: SRP_TSK_MGMT_RSV.
+ * - SRP_TX_IU_RSP: 1, since a conforming SRP target never sends more than
+ *   one unanswered SRP request to an initiator.
+ */
+static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
+				      enum srp_tx_iu_type tx_iu_type)
+{
+	s32 rsv;
+
+	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_SQ_MASK + 1);
+
+	srp_send_completion(target->send_cq, target);
+
+	rsv = (tx_iu_type == SRP_TX_IU_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
+
+	if (SRP_SQ_SIZE - (target->tx_head - target->tx_tail) <= rsv)
+		return NULL;
+
+	if (tx_iu_type != SRP_TX_IU_RSP && target->req_lim <= rsv) {
+		++target->zero_req_lim;
+		return NULL;
+	}
+
+	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held to protect tx_head.
+ */
+static int __srp_post_send_iu(struct srp_target_port *target,
+			      struct srp_iu *iu, int len)
+{
+	struct ib_sge list;
+	struct ib_send_wr wr, *bad_wr;
+	int ret = 0;
+
+	list.addr   = iu->dma;
+	list.length = len;
+	list.lkey   = target->srp_host->srp_dev->mr->lkey;
+
+	wr.next       = NULL;
+	wr.wr_id      = target->tx_head & SRP_SQ_MASK;
+	wr.sg_list    = &list;
+	wr.num_sge    = 1;
+	wr.opcode     = IB_WR_SEND;
+	wr.send_flags = IB_SEND_SIGNALED;
+
+	ret = ib_post_send(target->qp, &wr, &bad_wr);
+
+	if (!ret)
+		++target->tx_head;
+
+	return ret;
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held to protect req_lim.
+ */
+static int __srp_post_send_req(struct srp_target_port *target,
+			       struct srp_iu *iu, int len)
+{
+	int ret;
+
+	ret = __srp_post_send_iu(target, iu, len);
+	if (ret == 0)
+		--target->req_lim;
+	return ret;
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock held.
+ */
+static int __srp_post_send_rsp(struct srp_target_port *target,
+			       struct srp_iu *iu, int len)
+{
+	return __srp_post_send_iu(target, iu, len);
+}
+
+/*
+ * Must be called with target->scsi_host->host_lock locked to protect
+ * target->req_lim.
+ */
+static int srp_handle_cred_req(struct srp_target_port *target,
+			       struct srp_cred_req *req,
+			       struct srp_cred_rsp *rsp)
+{
+	target->req_lim += be32_to_cpu(req->req_lim_delta);
+
+	memset(rsp, 0, sizeof *rsp);
+	rsp->opcode = SRP_CRED_RSP;
+	rsp->tag    = req->tag;
+
+	return 0;
+}
+
+static void srp_handle_req(struct srp_target_port *target,
+			   struct srp_iu *req_iu)
+{
+	struct ib_device *dev;
+	u8 *req_buf;
+	unsigned long flags;
+	struct srp_iu *rsp_iu;
+	u8 *rsp_buf;
+	int res;
+
+	dev = target->srp_host->srp_dev->dev;
+	req_buf = req_iu->buf;
+
+	spin_lock_irqsave(target->scsi_host->host_lock, flags);
+
+	rsp_iu = __srp_get_tx_iu(target, SRP_TX_IU_RSP);
+	if (!rsp_iu)
+		goto out_unlock;
+
+	rsp_buf = rsp_iu->buf;
+
+	res = -EINVAL;
+
+	switch (req_buf[0]) {
+	case SRP_CRED_REQ:
+		res = srp_handle_cred_req(target,
+					  (struct srp_cred_req *)req_buf,
+					  (struct srp_cred_rsp *)rsp_buf);
+		break;
+	}
+
+	if (res)
+		goto out_unlock;
+
+	ib_dma_sync_single_for_device(dev, rsp_iu->dma, srp_max_iu_len,
+				      DMA_TO_DEVICE);
+
+	res = __srp_post_send_rsp(target, rsp_iu, sizeof *rsp_iu);
+	if (res)
+		shost_printk(KERN_ERR, target->scsi_host,
+			     PFX "Sending response failed -- res = %d\n", res);
+
+out_unlock:
+	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
+}
+
 static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
@@ -931,6 +1082,10 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 			     PFX "Got target logout request\n");
 		break;
 
+	case SRP_CRED_REQ:
+		srp_handle_req(target, iu);
+		break;
+
 	default:
 		shost_printk(KERN_WARNING, target->scsi_host,
 			     PFX "Unhandled SRP opcode 0x%02x\n", opcode);
@@ -983,65 +1138,6 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 	}
 }
 
-/*
- * 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
- * call to __srp_post_send().
- */
-static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
-					enum srp_request_type req_type)
-{
-	s32 rsv;
-
-	BUILD_BUG_ON_NOT_POWER_OF_2(SRP_SQ_MASK + 1);
-
-	rsv = (req_type == SRP_REQ_NORMAL) ? SRP_TSK_MGMT_RSV : 0;
-
-	srp_send_completion(target->send_cq, target);
-
-	if (target->tx_head - target->tx_tail >= SRP_REQ_SQ_SIZE - rsv)
-		return NULL;
-
-	if (target->req_lim <= rsv) {
-		++target->zero_req_lim;
-		return NULL;
-	}
-
-	return target->tx_ring[target->tx_head & SRP_SQ_MASK];
-}
-
-/*
- * Must be called with target->scsi_host->host_lock held to protect
- * req_lim and tx_head.
- */
-static int __srp_post_send(struct srp_target_port *target,
-			   struct srp_iu *iu, int len)
-{
-	struct ib_sge list;
-	struct ib_send_wr wr, *bad_wr;
-	int ret = 0;
-
-	list.addr   = iu->dma;
-	list.length = len;
-	list.lkey   = target->srp_host->srp_dev->mr->lkey;
-
-	wr.next       = NULL;
-	wr.wr_id      = target->tx_head & SRP_SQ_MASK;
-	wr.sg_list    = &list;
-	wr.num_sge    = 1;
-	wr.opcode     = IB_WR_SEND;
-	wr.send_flags = IB_SEND_SIGNALED;
-
-	ret = ib_post_send(target->qp, &wr, &bad_wr);
-
-	if (!ret) {
-		++target->tx_head;
-		--target->req_lim;
-	}
-
-	return ret;
-}
-
 static int srp_queuecommand(struct scsi_cmnd *scmnd,
 			    void (*done)(struct scsi_cmnd *))
 {
@@ -1062,7 +1158,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 		return 0;
 	}
 
-	iu = __srp_get_tx_iu(target, SRP_REQ_NORMAL);
+	iu = __srp_get_tx_iu(target, SRP_TX_IU_REQ_NORMAL);
 	if (!iu)
 		goto err;
 
@@ -1099,7 +1195,7 @@ static int srp_queuecommand(struct scsi_cmnd *scmnd,
 	ib_dma_sync_single_for_device(dev, iu->dma, srp_max_iu_len,
 				      DMA_TO_DEVICE);
 
-	if (__srp_post_send(target, iu, len)) {
+	if (__srp_post_send_req(target, iu, len)) {
 		shost_printk(KERN_ERR, target->scsi_host, PFX "Send failed\n");
 		goto err_unmap;
 	}
@@ -1369,7 +1465,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 
 	init_completion(&req->done);
 
-	iu = __srp_get_tx_iu(target, SRP_REQ_TASK_MGMT);
+	iu = __srp_get_tx_iu(target, SRP_TX_IU_REQ_TASK_MGMT);
 	if (!iu)
 		goto out;
 
@@ -1382,7 +1478,7 @@ static int srp_send_tsk_mgmt(struct srp_target_port *target,
 	tsk_mgmt->tsk_mgmt_func = func;
 	tsk_mgmt->task_tag 	= req->index;
 
-	if (__srp_post_send(target, iu, sizeof *tsk_mgmt))
+	if (__srp_post_send_req(target, iu, sizeof *tsk_mgmt))
 		goto out;
 
 	req->tsk_mgmt = iu;
drivers/infiniband/ulp/srp/ib_srp.h +4 -3
index 3ff38b2..3588923 100644
@@ -81,9 +81,10 @@ enum srp_target_state {
 	SRP_TARGET_REMOVED
 };
 
-enum srp_request_type {
-	SRP_REQ_NORMAL,
-	SRP_REQ_TASK_MGMT,
+enum srp_tx_iu_type {
+	SRP_TX_IU_REQ_NORMAL,
+	SRP_TX_IU_REQ_TASK_MGMT,
+	SRP_TX_IU_RSP,
 };
 
 struct srp_device {
include/scsi/srp.h +14 -0
index ad178fa..535eb4f 100644
@@ -239,4 +239,18 @@ struct srp_rsp {
 	u8	data[0];
 } __attribute__((packed));
 
+struct srp_cred_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+};
+
+struct srp_cred_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
 #endif /* SCSI_SRP_H */
Signature · 6 lines
1.6.4.2

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

#2 lore raw
Quoted 4 lines · Bart Van Assche

Implements SRP_CRED_REQ, which is an information unit defined in the SRP (draft) standard and that allows an SRP target to inform an SRP initiator that more requests may be sent by the initiator. Adds declarations for the SRP_CRED_REQ and SRP_CRED_RSP information units to include/scsi/srp.h.

I don't seem to be communicating my comments very clearly, so let me try the language of code. Here's a proposed alternative version. Compile tested only, based on your patch 1/3 v4, with the __srp_get_tx_iu() changes partially reverted. It would need to be rebased once you send a revised 1/3 and would need testing.

And, again, I don't care who's name this goes in under.
---
IB/srp: minimal support for SRP_CRED_REQ and SRP_AER_REQ

This patch adds support for SRP_CRED_REQ to avoid a lockup by targets that use that mechanism to return credits to the initiator. This prevents a lockup observed in the field where we would never add the credits from the SRP_CRED_REQ to our current count, and would therefore never send another command to the target.

Minimal support for SRP_AER_REQ is also added, as these messages can also be used to convey additional credits to the initiator.

Based upon extensive debugging work and initial code by Bart Van Assche and a bug report by Chris Worley.

 drivers/infiniband/ulp/srp/ib_srp.c |   82 ++++++++++++++++++++++++++++++++++-
 drivers/infiniband/ulp/srp/ib_srp.h |    1 +
 include/scsi/srp.h                  |   38 ++++++++++++++++
 3 files changed, 119 insertions(+), 2 deletions(-)
drivers/infiniband/ulp/srp/ib_srp.c +80 -2
index 9716e72..91e5a54 100644
@@ -83,6 +83,8 @@ 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_cm_handler(struct ib_cm_id *cm_id, struct ib_cm_event *event);
+static int srp_response_common(struct srp_target_port *target, s32 req_delta,
+			       void *rsp, int len);
 
 static struct scsi_transport_template *ib_srp_transport_template;
 
@@ -896,6 +898,37 @@ static void srp_process_rsp(struct srp_target_port *target, struct srp_rsp *rsp)
 	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
 }
 
+static void srp_process_cred_req(struct srp_target_port *target,
+				 struct srp_cred_req *req)
+{
+	struct srp_cred_rsp rsp = {
+		.opcode = SRP_CRED_RSP,
+		.tag = req->tag,
+	};
+	s32 delta = be32_to_cpu(req->req_lim_delta);
+
+	if (srp_response_common(target, delta, &rsp, sizeof rsp))
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "problems processing SRP_CRED_REQ\n");
+}
+
+static void srp_process_aer_req(struct srp_target_port *target,
+				struct srp_aer_req *req)
+{
+	struct srp_aer_rsp rsp = {
+		.opcode = SRP_AER_RSP,
+		.tag = req->tag,
+	};
+	s32 delta = be32_to_cpu(req->req_lim_delta);
+
+	shost_printk(KERN_ERR, target->scsi_host, PFX
+		     "ignoring AER for LUN %llu\n", be64_to_cpu(req->lun));
+
+	if (srp_response_common(target, delta, &rsp, sizeof rsp))
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "problems processing SRP_AER_REQ\n");
+}
+
 static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 {
 	struct ib_device *dev;
@@ -923,6 +956,14 @@ static void srp_handle_recv(struct srp_target_port *target, struct ib_wc *wc)
 		srp_process_rsp(target, iu->buf);
 		break;
 
+	case SRP_CRED_REQ:
+		srp_process_cred_req(target, iu->buf);
+		break;
+
+	case SRP_AER_REQ:
+		srp_process_aer_req(target, iu->buf);
+		break;
+
 	case SRP_T_LOGOUT:
 		/* XXX Handle target logout */
 		shost_printk(KERN_WARNING, target->scsi_host,
@@ -996,7 +1037,8 @@ static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
 		return NULL;
 
-	if (target->req_lim < min) {
+	/* Initiator responses to target requests do not consume credits */
+	if (req_type != SRP_RESPONSE && target->req_lim < min) {
 		++target->zero_req_lim;
 		return NULL;
 	}
@@ -1013,6 +1055,7 @@ static int __srp_post_send(struct srp_target_port *target,
 {
 	struct ib_sge list;
 	struct ib_send_wr wr, *bad_wr;
+	u8 opcode = *(u8 *)iu->buf;
 	int ret = 0;
 
 	list.addr   = iu->dma;
@@ -1030,7 +1073,8 @@ static int __srp_post_send(struct srp_target_port *target,
 
 	if (!ret) {
 		++target->tx_head;
-		--target->req_lim;
+		if (opcode == SRP_CMD || opcode == SRP_TSK_MGMT)
+			--target->req_lim;
 	}
 
 	return ret;
@@ -1109,6 +1153,40 @@ err:
 	return SCSI_MLQUEUE_HOST_BUSY;
 }
 
+static int srp_response_common(struct srp_target_port *target, s32 req_delta,
+			       void *rsp, int len)
+{
+	struct ib_device *dev;
+	unsigned long flags;
+	struct srp_iu *iu;
+	int err = 1;
+
+	dev = target->srp_host->srp_dev->dev;
+
+	spin_lock_irqsave(target->scsi_host->host_lock, flags);
+	target->req_lim += req_delta;
+
+	iu = __srp_get_tx_iu(target, SRP_RESPONSE);
+	if (!iu) {
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "no IU available to send response\n");
+		goto out;
+	}
+
+	ib_dma_sync_single_for_cpu(dev, iu->dma, len, DMA_TO_DEVICE);
+	memcpy(iu->buf, rsp, len);
+	ib_dma_sync_single_for_device(dev, iu->dma, len, DMA_TO_DEVICE);
+
+	err = __srp_post_send(target, iu, len);
+	if (err)
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "unable to post response: %d\n", err);
+
+out:
+	spin_unlock_irqrestore(target->scsi_host->host_lock, flags);
+	return err;
+}
+
 static int srp_alloc_iu_bufs(struct srp_target_port *target)
 {
 	int i;
drivers/infiniband/ulp/srp/ib_srp.h +1 -0
index 3ff38b2..6226479 100644
@@ -84,6 +84,7 @@ enum srp_target_state {
 enum srp_request_type {
 	SRP_REQ_NORMAL,
 	SRP_REQ_TASK_MGMT,
+	SRP_RESPONSE,
 };
 
 struct srp_device {
include/scsi/srp.h +38 -0
index ad178fa..f0de85d 100644
@@ -237,6 +237,44 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
+};
+
+struct srp_cred_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+};
+
+struct srp_cred_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
+/*
+ * The SRP spec defines the fixed portion of the AER_REQ structure to be
+ * 36 bytes, so it needs to be packed to avoid having it padded to 40 bytes
+ * on 64-bit architectures.
+ */
+struct srp_aer_req {
+	u8	opcode;
+	u8	sol_not;
+	u8	reserved[2];
+	__be32	req_lim_delta;
+	u64	tag;
+	u32	reserved2;
+	__be64	lun;
+	__be32	sense_data_len;
+	u32	reserved3;
+	u8	sense_data[0];
 } __attribute__((packed));
 
+struct srp_aer_rsp {
+	u8	opcode;
+	u8	reserved[7];
+	u64	tag;
+};
+
 #endif /* SCSI_SRP_H */

--
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): 1281729320.2437.142.camel-FqX9LgGZnHWDB2HL1qBt2PIbXMQ5te18@public.gmane.org

#4 lore raw
Quoted 7 lines · David Dillow
Quoted 4 lines · Bart Van Assche

Implements SRP_CRED_REQ, which is an information unit defined in the SRP (draft) standard and that allows an SRP target to inform an SRP initiator that more requests may be sent by the initiator. Adds declarations for the SRP_CRED_REQ and SRP_CRED_RSP information units to include/scsi/srp.h.

Compile tested only

Indeed, I broke it.

include/scsi/srp.h struct srp_rsp { · line 241

Quoted 8 lines
include/scsi/srp.h +2 -0
@@ -237,6 +237,44 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
+};
+

Oops, accidentally removed __attribute__((packed)) from struct srp_rsp.

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

#5 lore raw
In reply to a message not in the archive
Quoted 7 lines · David Dillow
Quoted 4 lines · Bart Van Assche

Implements SRP_CRED_REQ, which is an information unit defined in the SRP (draft) standard and that allows an SRP target to inform an SRP initiator that more requests may be sent by the initiator. Adds declarations for the SRP_CRED_REQ and SRP_CRED_RSP information units to include/scsi/srp.h.

Quoted 5 lines

I don't seem to be communicating my comments very clearly, so let me try the language of code. Here's a proposed alternative version. Compile tested only, based on your patch 1/3 v4, with the __srp_get_tx_iu() changes partially reverted. It would need to be rebased once you send a revised 1/3 and would need testing.

It's not that your communication is not clear but that I'm not sure whether it's a good idea to ignore the sense data included in an SRP_AER_REQ message. Supporting SRP_AER_REQ properly does not only mean processing the credit data included in that message but also processing the sense data included in that message.

Bart.
--
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): AANLkTi=Wy50xcE=-+Y67HB+GbOyLmWe72jZLYu8Cjoep-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org

#7 lore raw
Quoted 14 lines · Bart Van Assche
Quoted 7 lines · David Dillow
Quoted 4 lines · Bart Van Assche

Implements SRP_CRED_REQ, which is an information unit defined in the SRP (draft) standard and that allows an SRP target to inform an SRP initiator that more requests may be sent by the initiator. Adds declarations for the SRP_CRED_REQ and SRP_CRED_RSP information units to include/scsi/srp.h.

Quoted 5 lines

I don't seem to be communicating my comments very clearly, so let me try the language of code. Here's a proposed alternative version. Compile tested only, based on your patch 1/3 v4, with the __srp_get_tx_iu() changes partially reverted. It would need to be rebased once you send a revised 1/3 and would need testing.

Quoted 5 lines

It's not that your communication is not clear but that I'm not sure whether it's a good idea to ignore the sense data included in an SRP_AER_REQ message. Supporting SRP_AER_REQ properly does not only mean processing the credit data included in that message but also processing the sense data included in that message.

As far as I know, there is no current way to inject the sense data into the SCSI mid-layer. This at least gives us a starting point to work on that, while allowing us to recover the credits transferred to the initiator by the message. We don't have to do it all in one go, and now we'll at least know when a target uses SRP_AER_REQ.

Dave

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