Skip to contentSkip to conversation

iscsi bidi & varlen support

PATCH In review Boaz Harrosh 4 participants 17 messages Jan 31, 2008 – Feb 18, 2008 2 messages not in the archive

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

cover iscsi bidi & varlen support

Cheers after 1.3 years these can go in.

[PATCH 1/3] iscsi: extended cdb support
The varlen support is not yet in mainline for
block and scsi-ml. But the API for drivers will not change. All LLD need to do is max_command to the it's maximum and be ready for bigger commands. This is what's done here. Once these commands start coming iscsi will be ready for them.

[PATCH 2/3] iscsi: bidi support - libiscsi [PATCH 3/3] iscsi: bidi support - iscsi_tcp
bidirectional commands support in iscsi. iSER is not yet ready, but it will not break. There is already a mechanism in libiscsi that will return error if bidi commands are sent iSER way.

Pete please send me the iSER bits so we can port them to this latest version.

Mike these patches are ontop of iscs branch of the iscsi git tree, they will apply but for compilation you will need to sync with Linus mainline. The patches are for the in-tree iscsi code. I own you the compat patch for the out-off-tree code, but this I will only be Sunday.

If we do it fast it might get accepted to 2.6.25 merge window

Everybody is invited to a party at Shila ben-yhuda 52 Tel-Aviv 9:45 pm. Drinks and wonderful see-food on us :)

Boaz

#1 lore raw

PATCH 1/3 iscsi: extended cdb support

Support for extended CDBs in iscsi.
All we need is to check if command spills over 16 bytes then allocate an iscsi-extended-header for the leftovers.

---
 drivers/scsi/iscsi_tcp.c   |    2 +-
 drivers/scsi/libiscsi.c    |   56 ++++++++++++++++++++++++++++++++++++++++----
 include/scsi/iscsi_proto.h |    6 +++-
 3 files changed, 56 insertions(+), 8 deletions(-)
drivers/scsi/iscsi_tcp.c +1 -1
index 8a17867..ee7acfa 100644
@@ -1975,7 +1975,7 @@ static struct iscsi_transport iscsi_tcp_transport = {
 	.host_template		= &iscsi_sht,
 	.conndata_size		= sizeof(struct iscsi_conn),
 	.max_conn		= 1,
-	.max_cmd_len		= 16,
+	.max_cmd_len		= 260,
 	/* session management */
 	.create_session		= iscsi_tcp_session_create,
 	.destroy_session	= iscsi_tcp_session_destroy,
drivers/scsi/libiscsi.c +51 -5

74 lines not on this page · raw message

include/scsi/iscsi_proto.h +4 -2
index 5ffec8a..e0593bf 100644
@@ -112,6 +112,7 @@ struct iscsi_ahs_hdr {
 
 #define ISCSI_AHSTYPE_CDB		1
 #define ISCSI_AHSTYPE_RLENGTH		2
+#define ISCSI_CDB_SIZE			16
 
 /* iSCSI PDU Header */
 struct iscsi_cmd {
@@ -125,7 +126,7 @@ struct iscsi_cmd {
 	__be32 data_length;
 	__be32 cmdsn;
 	__be32 exp_statsn;
-	uint8_t cdb[16];	/* SCSI Command Block */
+	uint8_t cdb[ISCSI_CDB_SIZE];	/* SCSI Command Block */
 	/* Additional Data (Command Dependent) */
 };
 
@@ -154,7 +155,8 @@ struct iscsi_ecdb_ahdr {
 	__be16 ahslength;	/* CDB length - 15, including reserved byte */
 	uint8_t ahstype;
 	uint8_t reserved;
-	uint8_t ecdb[260 - 16];	/* 4-byte aligned extended CDB spillover */
+	/* 4-byte aligned extended CDB spillover */
+	uint8_t ecdb[260 - ISCSI_CDB_SIZE];
 };
 
 /* SCSI Response Header */
Signature · 1 line
1.5.3.3
#2 lore raw

PATCH 2/3 iscsi: bidi support - libiscsi

In reply to Boaz Harrosh

iscsi bidi support at the generic libiscsi level
- prepare the additional bidi_read rlength header.
- access the right scsi_in() and/or scsi_out() side of things. also for resid.
- Handle BIDI underflow overflow from target

---
 drivers/scsi/libiscsi.c |   82 ++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 71 insertions(+), 11 deletions(-)
drivers/scsi/libiscsi.c +71 -11
index 498cea3..df81f6f 100644
@@ -176,6 +176,31 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask)
 	return 0;
 }
 
+static int iscsi_prep_bidi_ahs(struct iscsi_cmd_task *ctask)
+{
+	struct scsi_cmnd *sc = ctask->sc;
+	struct iscsi_rlength_ahdr *rlen_ahdr;
+	int rc;
+
+	rlen_ahdr = iscsi_next_hdr(ctask);
+	rc = iscsi_add_hdr(ctask, sizeof(*rlen_ahdr));
+	if (rc)
+		return rc;
+
+	rlen_ahdr->ahslength =
+		cpu_to_be16(sizeof(rlen_ahdr->read_length) +
+			            sizeof(rlen_ahdr->reserved));
+	rlen_ahdr->ahstype = ISCSI_AHSTYPE_RLENGTH;
+	rlen_ahdr->reserved = 0;
+	rlen_ahdr->read_length = cpu_to_be32(scsi_in(sc)->length);
+
+	debug_scsi("bidi-in rlen_ahdr->read_length(%d) "
+		   "rlen_ahdr->ahslength(%d)\n",
+		   be32_to_cpu(rlen_ahdr->read_length),
+		   be16_to_cpu(rlen_ahdr->ahslength));
+	return 0;
+}
+
 /**
  * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu
  * @ctask: iscsi cmd task
@@ -200,7 +225,6 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	hdr->flags = ISCSI_ATTR_SIMPLE;
 	int_to_scsilun(sc->device->lun, (struct scsi_lun *)hdr->lun);
 	hdr->itt = build_itt(ctask->itt, session->age);
-	hdr->data_length = cpu_to_be32(scsi_bufflen(sc));
 	hdr->cmdsn = cpu_to_be32(session->cmdsn);
 	session->cmdsn++;
 	hdr->exp_statsn = cpu_to_be32(conn->exp_statsn);
@@ -217,7 +241,15 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	memcpy(hdr->cdb, sc->cmnd, cmd_len);
 
 	ctask->imm_count = 0;
+	if (scsi_bidi_cmnd(sc)) {
+		hdr->flags |= ISCSI_FLAG_CMD_READ;
+		rc = iscsi_prep_bidi_ahs(ctask);
+		if (rc)
+			return rc;
+	}
 	if (sc->sc_data_direction == DMA_TO_DEVICE) {
+		unsigned out_len = scsi_out(sc)->length;
+		hdr->data_length = cpu_to_be32(out_len);
 		hdr->flags |= ISCSI_FLAG_CMD_WRITE;
 		/*
 		 * Write counters:
@@ -238,19 +270,19 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 		ctask->unsol_datasn = 0;
 
 		if (session->imm_data_en) {
-			if (scsi_bufflen(sc) >= session->first_burst)
+			if (out_len >= session->first_burst)
 				ctask->imm_count = min(session->first_burst,
 							conn->max_xmit_dlength);
 			else
-				ctask->imm_count = min(scsi_bufflen(sc),
+				ctask->imm_count = min(out_len,
 							conn->max_xmit_dlength);
 			hton24(hdr->dlength, ctask->imm_count);
 		} else
 			zero_data(hdr->dlength);
 
 		if (!session->initial_r2t_en) {
-			ctask->unsol_count = min((session->first_burst),
-				(scsi_bufflen(sc))) - ctask->imm_count;
+			ctask->unsol_count = min(session->first_burst, out_len)
+							     - ctask->imm_count;
 			ctask->unsol_offset = ctask->imm_count;
 		}
 
@@ -260,6 +292,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	} else {
 		hdr->flags |= ISCSI_FLAG_CMD_FINAL;
 		zero_data(hdr->dlength);
+		hdr->data_length = cpu_to_be32(scsi_in(sc)->length);
 
 		if (sc->sc_data_direction == DMA_FROM_DEVICE)
 			hdr->flags |= ISCSI_FLAG_CMD_READ;
@@ -278,10 +311,12 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 		return EIO;
 
 	conn->scsicmd_pdus_cnt++;
-	debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x len %d "
-		"cmdsn %d win %d]\n",
-		sc->sc_data_direction == DMA_TO_DEVICE ? "write" : "read",
-		conn->id, sc, sc->cmnd[0], ctask->itt, scsi_bufflen(sc),
+	debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x "
+		"len %d bidi_len %d cmdsn %d win %d]\n",
+		scsi_bidi_cmnd(sc) ? "bidirectional" :
+		    sc->sc_data_direction == DMA_TO_DEVICE ? "write" : "read",
+		conn->id, sc, sc->cmnd[0], ctask->itt,
+		scsi_bufflen(sc), scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
 		session->cmdsn, session->max_cmdsn - session->exp_cmdsn + 1);
 	return 0;
 }
@@ -344,7 +379,12 @@ static void fail_command(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask,
 		conn->session->tt->cleanup_cmd_task(conn, ctask);
 
 	sc->result = err;
-	scsi_set_resid(sc, scsi_bufflen(sc));
+	if (!scsi_bidi_cmnd(sc))
+		scsi_set_resid(sc, scsi_bufflen(sc));
+	else {
+		scsi_out(sc)->resid = scsi_out(sc)->length;
+		scsi_in(sc)->resid = scsi_in(sc)->length;
+	}
 	if (conn->ctask == ctask)
 		conn->ctask = NULL;
 	/* release ref from queuecommand */
@@ -479,6 +519,20 @@ invalid_datalen:
 			   min_t(uint16_t, senselen, SCSI_SENSE_BUFFERSIZE));
 	}
 
+	if (scsi_bidi_cmnd(sc) &&
+		(rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
+					ISCSI_FLAG_CMD_BIDI_OVERFLOW))) {
+		int res_count = be32_to_cpu(rhdr->bi_residual_count);
+
+		if (res_count > 0 &&
+			(rhdr->flags & ISCSI_FLAG_CMD_BIDI_OVERFLOW ||
+			res_count <= scsi_in(sc)->length))
+			scsi_in(sc)->resid = res_count;
+		else
+			sc->result =
+				(DID_BAD_TARGET << 16) | rhdr->cmd_status;
+	}
+
 	if (rhdr->flags & (ISCSI_FLAG_CMD_UNDERFLOW |
 	                   ISCSI_FLAG_CMD_OVERFLOW)) {
 		int res_count = be32_to_cpu(rhdr->residual_count);
@@ -486,6 +540,7 @@ invalid_datalen:
 		if (res_count > 0 &&
 		    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
 		     res_count <= scsi_bufflen(sc)))
+			/* write side for bidi or uni-io set_resid */
 			scsi_set_resid(sc, res_count);
 		else
 			sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
@@ -1149,7 +1204,12 @@ reject:
 fault:
 	spin_unlock(&session->lock);
 	debug_scsi("iscsi: cmd 0x%x is not queued (%d)\n", sc->cmnd[0], reason);
-	scsi_set_resid(sc, scsi_bufflen(sc));
+	if (!scsi_bidi_cmnd(sc))
+		scsi_set_resid(sc, scsi_bufflen(sc));
+	else {
+		scsi_out(sc)->resid = scsi_out(sc)->length;
+		scsi_in(sc)->resid = scsi_in(sc)->length;
+	}
 	sc->scsi_done(sc);
 	spin_lock(host->host_lock);
 	return 0;
Signature · 1 line
1.5.3.3

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

#4 lore raw

Re: [PATCH 2/3] iscsi: bidi support - libiscsi

[email protected] wrote on Thu, 31 Jan 2008 22:29 +0200:

Quoted 7 lines

iscsi bidi support at the generic libiscsi level
- prepare the additional bidi_read rlength header.
- access the right scsi_in() and/or scsi_out() side of things. also for resid.
- Handle BIDI underflow overflow from target

I see you do this a bit differently than in your previous patch set. In particular, the residual handling in libiscsi.c. (I'm editing in a bit more context to the patch below.)

Quoted 18 lines
drivers/scsi/libiscsi.c +14 -0
+	if (scsi_bidi_cmnd(sc) &&
+		(rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
+					ISCSI_FLAG_CMD_BIDI_OVERFLOW))) {
+		int res_count = be32_to_cpu(rhdr->bi_residual_count);
+
+		if (res_count > 0 &&
+			(rhdr->flags & ISCSI_FLAG_CMD_BIDI_OVERFLOW ||
+			res_count <= scsi_in(sc)->length))
+			scsi_in(sc)->resid = res_count;
+		else
+			sc->result =
+				(DID_BAD_TARGET << 16) | rhdr->cmd_status;
+	}
+
 	if (rhdr->flags & (ISCSI_FLAG_CMD_UNDERFLOW |
 	                   ISCSI_FLAG_CMD_OVERFLOW)) {
 		int res_count = be32_to_cpu(rhdr->residual_count);
 

drivers/scsi/libiscsi.c invalid_datalen:

Quoted 10 lines
drivers/scsi/libiscsi.c +1 -0
 		if (res_count > 0 &&
 		    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
 		     res_count <= scsi_bufflen(sc)))
+			/* write side for bidi or uni-io set_resid */
 			scsi_set_resid(sc, res_count);
 		else
 			sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
      } else if (rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
                                ISCSI_FLAG_CMD_BIDI_OVERFLOW))
              sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;

I haven't tested this, but, consider a bidi command that results in an overflow on the read part, and no overflow on the write part. E.g., the user supplied a data-in buffer that wasn't big enough to hold the returned data from the target, but the data-out buffer was just right.

Then this code will set scsi_in(sc)->resid properly, informing the caller that there were extra bytes that were not transferred. But the "else" clause at the bottom will also set sc->result to be bad. I don't think we want this.

Your earlier patch got rid of the second bidi_overflow handler and just did all the logic for both bidi and non-bidi cases in a single if clause. Seemed better.

		-- Pete
#5 lore raw

Re: [PATCH 2/3] iscsi: bidi support - libiscsi

Quoted 53 lines · Pete Wyckoff

[email protected] wrote on Thu, 31 Jan 2008 22:29 +0200:

Quoted 7 lines

iscsi bidi support at the generic libiscsi level
- prepare the additional bidi_read rlength header.
- access the right scsi_in() and/or scsi_out() side of things. also for resid.
- Handle BIDI underflow overflow from target

I see you do this a bit differently than in your previous patch set. In particular, the residual handling in libiscsi.c. (I'm editing in a bit more context to the patch below.)

Quoted 18 lines
drivers/scsi/libiscsi.c +14 -0
+	if (scsi_bidi_cmnd(sc) &&
+		(rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
+					ISCSI_FLAG_CMD_BIDI_OVERFLOW))) {
+		int res_count = be32_to_cpu(rhdr->bi_residual_count);
+
+		if (res_count > 0 &&
+			(rhdr->flags & ISCSI_FLAG_CMD_BIDI_OVERFLOW ||
+			res_count <= scsi_in(sc)->length))
+			scsi_in(sc)->resid = res_count;
+		else
+			sc->result =
+				(DID_BAD_TARGET << 16) | rhdr->cmd_status;
+	}
+
 	if (rhdr->flags & (ISCSI_FLAG_CMD_UNDERFLOW |
 	                   ISCSI_FLAG_CMD_OVERFLOW)) {
 		int res_count = be32_to_cpu(rhdr->residual_count);
 

drivers/scsi/libiscsi.c invalid_datalen:

Quoted 10 lines
drivers/scsi/libiscsi.c +1 -0
 		if (res_count > 0 &&
 		    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
 		     res_count <= scsi_bufflen(sc)))
+			/* write side for bidi or uni-io set_resid */
 			scsi_set_resid(sc, res_count);
 		else
 			sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
      } else if (rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
                                ISCSI_FLAG_CMD_BIDI_OVERFLOW))
              sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;

I haven't tested this, but, consider a bidi command that results in an overflow on the read part, and no overflow on the write part. E.g., the user supplied a data-in buffer that wasn't big enough to hold the returned data from the target, but the data-out buffer was just right.

Then this code will set scsi_in(sc)->resid properly, informing the caller that there were extra bytes that were not transferred. But the "else" clause at the bottom will also set sc->result to be bad. I don't think we want this.

Quoted 5 lines

Your earlier patch got rid of the second bidi_overflow handler and just did all the logic for both bidi and non-bidi cases in a single if clause. Seemed better.

		-- Pete

You are most probably right I will investigate what happened. It looks like I went back to some old version right? or a merge fallout Thanks for reviewing.

Please also test latest head-of-line code if possible + iscsi patches
+ last varlen I sent.

Is there any new patches I need for 2.6.24 or head-of-line for my osd-dev tree?

Boaz

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

#7 lore raw

Re: [PATCH 2/3] iscsi: bidi support - libiscsi

[email protected] wrote on Mon, 11 Feb 2008 18:05 +0200:

Quoted 9 lines

You are most probably right I will investigate what happened. It looks like I went back to some old version right? or a merge fallout Thanks for reviewing.

Please also test latest head-of-line code if possible + iscsi patches
+ last varlen I sent.

Is there any new patches I need for 2.6.24 or head-of-line for my osd-dev tree?

Testing now. My patch set is actually shrinking (!) thanks to some merges. Some rebasing was required to apply your three varlen patches and three bidi patches, but I'm sure you'll update your tree and push those upstream soon enough. I'll take a look at iser bidi then update my patch list and let you know soon.

		-- Pete
#8 lore raw

PATCH 3/3 iscsi: bidi support - iscsi_tcp

In reply to Boaz Harrosh

bidi support for iscsi_tcp
- access the right scsi_in() and/or scsi_out() side of things. also for resid

---
 drivers/scsi/iscsi_tcp.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)
drivers/scsi/iscsi_tcp.c +17 -14
index ee7acfa..073dea7 100644
@@ -528,6 +528,7 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 	struct iscsi_session *session = conn->session;
 	struct scsi_cmnd *sc = ctask->sc;
 	int datasn = be32_to_cpu(rhdr->datasn);
+	unsigned total_in_length = scsi_in(sc)->length;
 
 	iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr);
 	if (tcp_conn->in.datalen == 0)
@@ -542,10 +543,10 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 	tcp_ctask->exp_datasn++;
 
 	tcp_ctask->data_offset = be32_to_cpu(rhdr->offset);
-	if (tcp_ctask->data_offset + tcp_conn->in.datalen > scsi_bufflen(sc)) {
+	if (tcp_ctask->data_offset + tcp_conn->in.datalen > total_in_length) {
 		debug_tcp("%s: data_offset(%d) + data_len(%d) > total_length_in(%d)\n",
 		          __FUNCTION__, tcp_ctask->data_offset,
-		          tcp_conn->in.datalen, scsi_bufflen(sc));
+		          tcp_conn->in.datalen, total_in_length);
 		return ISCSI_ERR_DATA_OFFSET;
 	}
 
@@ -558,8 +559,8 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 
 			if (res_count > 0 &&
 			    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
-			     res_count <= scsi_bufflen(sc)))
-				scsi_set_resid(sc, res_count);
+			     res_count <= total_in_length))
+				scsi_in(sc)->resid = res_count;
 			else
 				sc->result = (DID_BAD_TARGET << 16) |
 					rhdr->cmd_status;
@@ -670,11 +671,11 @@ iscsi_r2t_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 			    r2t->data_length, session->max_burst);
 
 	r2t->data_offset = be32_to_cpu(rhdr->data_offset);
-	if (r2t->data_offset + r2t->data_length > scsi_bufflen(ctask->sc)) {
+	if (r2t->data_offset + r2t->data_length > scsi_out(ctask->sc)->length) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "invalid R2T with data len %u at offset %u "
 				  "and total length %d\n", r2t->data_length,
-				  r2t->data_offset, scsi_bufflen(ctask->sc));
+				  r2t->data_offset, scsi_out(ctask->sc)->length);
 		__kfifo_put(tcp_ctask->r2tpool.queue, (void*)&r2t,
 			    sizeof(void*));
 		return ISCSI_ERR_DATALEN;
@@ -771,6 +772,7 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 		if (tcp_conn->in.datalen) {
 			struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data;
 			struct hash_desc *rx_hash = NULL;
+			struct scsi_data_buffer *sdb = scsi_in(ctask->sc);
 
 			/*
 			 * Setup copy of Data-In into the Scsi_Cmnd
@@ -788,8 +790,8 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 				  tcp_ctask->data_offset,
 				  tcp_conn->in.datalen);
 			return iscsi_segment_seek_sg(&tcp_conn->in.segment,
-						     scsi_sglist(ctask->sc),
-						     scsi_sg_count(ctask->sc),
+						     sdb->table.sgl,
+						     sdb->table.nents,
 						     tcp_ctask->data_offset,
 						     tcp_conn->in.datalen,
 						     iscsi_tcp_process_data_in,
@@ -1332,7 +1334,8 @@ iscsi_tcp_ctask_init(struct iscsi_cmd_task *ctask)
 		return 0;
 
 	/* If we have immediate data, attach a payload */
-	err = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc), scsi_sg_count(sc),
+	err = iscsi_tcp_send_data_prep(conn, scsi_out(sc)->table.sgl,
+				       scsi_out(sc)->table.nents,
 				       0, ctask->imm_count);
 	if (err)
 		return err;
@@ -1386,6 +1389,7 @@ iscsi_tcp_ctask_xmit(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 {
 	struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data;
 	struct scsi_cmnd *sc = ctask->sc;
+	struct scsi_data_buffer *sdb = scsi_out(sc);
 	int rc = 0;
 
 flush:
@@ -1412,9 +1416,8 @@ flush:
 				ctask->itt, tcp_ctask->sent, ctask->data_count);
 
 		iscsi_tcp_send_hdr_prep(conn, hdr, sizeof(*hdr));
-		rc = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc),
-					      scsi_sg_count(sc),
-					      tcp_ctask->sent,
+		rc = iscsi_tcp_send_data_prep(conn, sdb->table.sgl,
+					      sdb->table.nents, tcp_ctask->sent,
 					      ctask->data_count);
 		if (rc)
 			goto fail;
@@ -1460,8 +1463,8 @@ flush:
 		iscsi_tcp_send_hdr_prep(conn, &r2t->dtask.hdr,
 					sizeof(struct iscsi_hdr));
 
-		rc = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc),
-					      scsi_sg_count(sc),
+		rc = iscsi_tcp_send_data_prep(conn, sdb->table.sgl,
+					      sdb->table.nents,
 					      r2t->data_offset + r2t->sent,
 					      r2t->data_count);
 		if (rc)
Signature · 1 line
1.5.3.3
#9 lore raw
In reply to Boaz Harrosh

[email protected] wrote on Thu, 31 Jan 2008 20:08 +0200:

Quoted 26 lines

Cheers after 1.3 years these can go in.

[PATCH 1/3] iscsi: extended cdb support
The varlen support is not yet in mainline for
block and scsi-ml. But the API for drivers will not change. All LLD need to do is max_command to the it's maximum and be ready for bigger commands. This is what's done here. Once these commands start coming iscsi will be ready for them.

[PATCH 2/3] iscsi: bidi support - libiscsi [PATCH 3/3] iscsi: bidi support - iscsi_tcp
bidirectional commands support in iscsi. iSER is not yet ready, but it will not break. There is already a mechanism in libiscsi that will return error if bidi commands are sent iSER way.

Pete please send me the iSER bits so we can port them to this latest version.

Mike these patches are ontop of iscs branch of the iscsi git tree, they will apply but for compilation you will need to sync with Linus mainline. The patches are for the in-tree iscsi code. I own you the compat patch for the out-off-tree code, but this I will only be Sunday.

Quoted 4 lines

If we do it fast it might get accepted to 2.6.25 merge window

Everybody is invited to a party at Shila ben-yhuda 52 Tel-Aviv 9:45 pm. Drinks and wonderful see-food on us :)

Here's the patch to add bidi support to iSER too. It works with my setup, but could use more testing. Note that this does rely on the 3 patches quoted above.

		-- Pete

From: Pete Wyckoff <[email protected]>

Support bidirectional SCSI commands in iSCSI iSER. The handling of data buffers for each direction was already mostly separated. It was necessary to separate the unmapping of data bufs when one was found to be unaligned. This was done by adding a "direction" parameter to iser_dma_unmap_task_data() and calling it appropriately. Also iser_ctask_rdma_finalize() used local variables in such a way that it assumed a unidirectional command; that was split up to make it cleaner and order the operations correctly for bidi.

---
 drivers/infiniband/ulp/iser/iscsi_iser.h     |    3 +-
 drivers/infiniband/ulp/iser/iser_initiator.c |   79 +++++++++++---------------
 drivers/infiniband/ulp/iser/iser_memory.c    |    9 ++-
 3 files changed, 41 insertions(+), 50 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.h +2 -1
index 66905df..f5497b0 100644
@@ -356,5 +356,6 @@ int iser_dma_map_task_data(struct iscsi_iser_cmd_task *iser_ctask,
 			    enum   iser_data_dir       iser_dir,
 			    enum   dma_data_direction  dma_dir);
 
-void iser_dma_unmap_task_data(struct iscsi_iser_cmd_task *iser_ctask);
+void iser_dma_unmap_task_data(struct iscsi_iser_cmd_task *iser_ctask,
+			      enum iser_data_dir cmd_dir);
 #endif
drivers/infiniband/ulp/iser/iser_initiator.c +34 -45

125 lines not on this page · raw message

drivers/infiniband/ulp/iser/iser_memory.c +5 -4
index 4a17743..aa3f299 100644
@@ -393,19 +393,20 @@ int iser_dma_map_task_data(struct iscsi_iser_cmd_task *iser_ctask,
 	return 0;
 }
 
-void iser_dma_unmap_task_data(struct iscsi_iser_cmd_task *iser_ctask)
+void iser_dma_unmap_task_data(struct iscsi_iser_cmd_task *iser_ctask,
+			      enum iser_data_dir cmd_dir)
 {
 	struct ib_device *dev;
 	struct iser_data_buf *data;
 
 	dev = iser_ctask->iser_conn->ib_conn->device->ib_device;
 
-	if (iser_ctask->dir[ISER_DIR_IN]) {
+	if (cmd_dir == ISER_DIR_IN && iser_ctask->dir[ISER_DIR_IN]) {
 		data = &iser_ctask->data[ISER_DIR_IN];
 		ib_dma_unmap_sg(dev, data->buf, data->size, DMA_FROM_DEVICE);
 	}
 
-	if (iser_ctask->dir[ISER_DIR_OUT]) {
+	if (cmd_dir == ISER_DIR_OUT && iser_ctask->dir[ISER_DIR_OUT]) {
 		data = &iser_ctask->data[ISER_DIR_OUT];
 		ib_dma_unmap_sg(dev, data->buf, data->size, DMA_TO_DEVICE);
 	}
@@ -439,7 +440,7 @@ int iser_reg_rdma_mem(struct iscsi_iser_cmd_task *iser_ctask,
 		iser_data_buf_dump(mem, ibdev);
 
 		/* unmap the command data before accessing it */
-		iser_dma_unmap_task_data(iser_ctask);
+		iser_dma_unmap_task_data(iser_ctask, cmd_dir);
 
 		/* allocate copy buf, if we are writing, copy the */
 		/* unaligned scatterlist, dma map the copy        */
Signature · 1 line
1.5.3.8
#10 lore raw

[email protected] wrote on Tue, 12 Feb 2008 15:12 -0500:

Quoted 27 lines

[email protected] wrote on Thu, 31 Jan 2008 20:08 +0200:

Quoted 17 lines

Cheers after 1.3 years these can go in.

[PATCH 1/3] iscsi: extended cdb support
The varlen support is not yet in mainline for
block and scsi-ml. But the API for drivers will not change. All LLD need to do is max_command to the it's maximum and be ready for bigger commands. This is what's done here. Once these commands start coming iscsi will be ready for them.

[PATCH 2/3] iscsi: bidi support - libiscsi [PATCH 3/3] iscsi: bidi support - iscsi_tcp
bidirectional commands support in iscsi. iSER is not yet ready, but it will not break. There is already a mechanism in libiscsi that will return error if bidi commands are sent iSER way.

Quoted 8 lines

Pete please send me the iSER bits so we can port them to this latest version.

Mike these patches are ontop of iscs branch of the iscsi git tree, they will apply but for compilation you will need to sync with Linus mainline. The patches are for the in-tree iscsi code. I own you the compat patch for the out-off-tree code, but this I will only be Sunday.

Quoted 3 lines

Here's the patch to add bidi support to iSER too. It works with my setup, but could use more testing. Note that this does rely on the 3 patches quoted above.

Similar, for varlen support to iSER. Probably apply this one before the bidi one I just sent.

		-- Pete

From: Pete Wyckoff <[email protected]>
Subject: [PATCH] iscsi iser: varlen

Handle variable-length CDBs in iSER.

---
 drivers/infiniband/ulp/iser/iscsi_iser.c     |    5 +++--
 drivers/infiniband/ulp/iser/iscsi_iser.h     |    2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c |   16 ++++++++++------
 3 files changed, 14 insertions(+), 9 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +3 -2
index 5f2284d..9dfc310 100644
@@ -401,7 +401,8 @@ iscsi_iser_session_create(struct iscsi_transport *iscsit,
 		ctask      = session->cmds[i];
 		iser_ctask = ctask->dd_data;
 		ctask->hdr = (struct iscsi_cmd *)&iser_ctask->desc.iscsi_header;
-		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header);
+		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header) +
+				 sizeof(iser_ctask->desc.hdrextbuf);
 	}
 
 	for (i = 0; i < session->mgmtpool_max; i++) {
@@ -604,7 +605,7 @@ static struct iscsi_transport iscsi_iser_transport = {
 	.host_template          = &iscsi_iser_sht,
 	.conndata_size		= sizeof(struct iscsi_conn),
 	.max_lun                = ISCSI_ISER_MAX_LUN,
-	.max_cmd_len            = ISCSI_ISER_MAX_CMD_LEN,
+	.max_cmd_len            = 260,
 	/* session management */
 	.create_session         = iscsi_iser_session_create,
 	.destroy_session        = iscsi_session_teardown,
drivers/infiniband/ulp/iser/iscsi_iser.h +1 -1
index db8f81a..66905df 100644
@@ -90,7 +90,6 @@
 /* FMR space for 1 MB of 4k-page transfers, plus 1 if not page aligned */
 #define ISCSI_ISER_SG_TABLESIZE         (((1<<20) >> SHIFT_4K) + 1)
 #define ISCSI_ISER_MAX_LUN		256
-#define ISCSI_ISER_MAX_CMD_LEN		16
 
 /* QP settings */
 /* Maximal bounds on received asynchronous PDUs */
@@ -217,6 +216,7 @@ enum iser_desc_type {
 struct iser_desc {
 	struct iser_hdr              iser_header;
 	struct iscsi_hdr             iscsi_header;
+	char			     hdrextbuf[ISCSI_MAX_AHS_SIZE];
 	struct iser_regd_buf         hdr_regd_buf;
 	void                         *data;         /* used by RX & TX_CONTROL */
 	struct iser_regd_buf         data_regd_buf; /* used by RX & TX_CONTROL */
drivers/infiniband/ulp/iser/iser_initiator.c +10 -6
index 83247f1..ea3f5dc 100644
@@ -246,9 +246,13 @@ post_rx_kmalloc_failure:
 	return err;
 }
 
-/* creates a new tx descriptor and adds header regd buffer */
+/**
+ * iser_create_send_desc - creates a new tx descriptor and adds header regd buffer
+ * @iscsi_hdr_len: length of &struct iscsi_hdr and any AHSes in hdrextbuf
+ */
 static void iser_create_send_desc(struct iscsi_iser_conn *iser_conn,
-				  struct iser_desc       *tx_desc)
+				  struct iser_desc       *tx_desc,
+				  int iscsi_hdr_len)
 {
 	struct iser_regd_buf *regd_hdr = &tx_desc->hdr_regd_buf;
 	struct iser_dto      *send_dto = &tx_desc->dto;
@@ -256,7 +260,7 @@ static void iser_create_send_desc(struct iscsi_iser_conn *iser_conn,
 	memset(regd_hdr, 0, sizeof(struct iser_regd_buf));
 	regd_hdr->device  = iser_conn->ib_conn->device;
 	regd_hdr->virt_addr  = tx_desc; /* == &tx_desc->iser_header */
-	regd_hdr->data_size  = ISER_TOTAL_HEADERS_LEN;
+	regd_hdr->data_size  = sizeof(struct iser_hdr) + iscsi_hdr_len;
 
 	send_dto->ib_conn         = iser_conn->ib_conn;
 	send_dto->notify_enable   = 1;
@@ -342,7 +346,7 @@ int iser_send_command(struct iscsi_conn     *conn,
 	iser_ctask->desc.type = ISCSI_TX_SCSI_COMMAND;
 	send_dto = &iser_ctask->desc.dto;
 	send_dto->ctask = iser_ctask;
-	iser_create_send_desc(iser_conn, &iser_ctask->desc);
+	iser_create_send_desc(iser_conn, &iser_ctask->desc, ctask->hdr_len);
 
 	if (hdr->flags & ISCSI_FLAG_CMD_READ)
 		data_buf = &iser_ctask->data[ISER_DIR_IN];
@@ -435,7 +439,7 @@ int iser_send_data_out(struct iscsi_conn     *conn,
 	/* build the tx desc regd header and add it to the tx desc dto */
 	send_dto = &tx_desc->dto;
 	send_dto->ctask = iser_ctask;
-	iser_create_send_desc(iser_conn, tx_desc);
+	iser_create_send_desc(iser_conn, tx_desc, ctask->hdr_len);
 
 	iser_reg_single(iser_conn->ib_conn->device,
 			send_dto->regd[0], DMA_TO_DEVICE);
@@ -492,7 +496,7 @@ int iser_send_control(struct iscsi_conn *conn,
 	mdesc->type = ISCSI_TX_CONTROL;
 	send_dto = &mdesc->dto;
 	send_dto->ctask = NULL;
-	iser_create_send_desc(iser_conn, mdesc);
+	iser_create_send_desc(iser_conn, mdesc, sizeof(struct iscsi_hdr));
 
 	device = iser_conn->ib_conn->device;
 
Signature · 1 line
1.5.3.8
#11 lore raw
Quoted 64 lines · Pete Wyckoff

[email protected] wrote on Tue, 12 Feb 2008 15:12 -0500:

Quoted 21 lines

[email protected] wrote on Thu, 31 Jan 2008 20:08 +0200:

Quoted 20 lines

Cheers after 1.3 years these can go in.

[PATCH 1/3] iscsi: extended cdb support
The varlen support is not yet in mainline for
block and scsi-ml. But the API for drivers will not change. All LLD need to do is max_command to the it's maximum and be ready for bigger commands. This is what's done here. Once these commands start coming iscsi will be ready for them.

[PATCH 2/3] iscsi: bidi support - libiscsi [PATCH 3/3] iscsi: bidi support - iscsi_tcp
bidirectional commands support in iscsi. iSER is not yet ready, but it will not break. There is already a mechanism in libiscsi that will return error if bidi commands are sent iSER way.

Pete please send me the iSER bits so we can port them to this latest version.

Quoted 8 lines
Quoted 5 lines

Mike these patches are ontop of iscs branch of the iscsi git tree, they will apply but for compilation you will need to sync with Linus mainline. The patches are for the in-tree iscsi code. I own you the compat patch for the out-off-tree code, but this I will only be Sunday.

Here's the patch to add bidi support to iSER too. It works with my setup, but could use more testing. Note that this does rely on the 3 patches quoted above.

Similar, for varlen support to iSER. Probably apply this one before the bidi one I just sent.

		-- Pete

From: Pete Wyckoff <[email protected]>
Subject: [PATCH] iscsi iser: varlen

Handle variable-length CDBs in iSER.

---
 drivers/infiniband/ulp/iser/iscsi_iser.c     |    5 +++--
 drivers/infiniband/ulp/iser/iscsi_iser.h     |    2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c |   16 ++++++++++------
 3 files changed, 14 insertions(+), 9 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +2 -1
index 5f2284d..9dfc310 100644
@@ -401,7 +401,8 @@ iscsi_iser_session_create(struct iscsi_transport *iscsit,
 		ctask      = session->cmds[i];
 		iser_ctask = ctask->dd_data;
 		ctask->hdr = (struct iscsi_cmd *)&iser_ctask->desc.iscsi_header;
-		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header);
+		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header) +
+				 sizeof(iser_ctask->desc.hdrextbuf);
 	}
 
 	for (i = 0; i < session->mgmtpool_max; i++) {

drivers/infiniband/ulp/iser/iscsi_iser.c static struct iscsi_transport iscsi_iser_transport = { · line 608

Quoted 5 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1
@@ -604,7 +605,7 @@ static struct iscsi_transport iscsi_iser_transport = {
 	.host_template          = &iscsi_iser_sht,
 	.conndata_size		= sizeof(struct iscsi_conn),
 	.max_lun                = ISCSI_ISER_MAX_LUN,
-	.max_cmd_len            = ISCSI_ISER_MAX_CMD_LEN,
+	.max_cmd_len            = 260,

Same bug I had. .max_cmd_len is still char, before the varlen patch to scsi-ml. So it must be at most 252, Until that patch is introduced and it can return to the correct 260 or better yet SCSI_MAX_VARLEN_CDB_SIZE. That also is only defined in the scsi-ml varlen patch.

Quoted 71 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +0 -0
 	/* session management */
 	.create_session         = iscsi_iser_session_create,
 	.destroy_session        = iscsi_session_teardown,
drivers/infiniband/ulp/iser/iscsi_iser.h +1 -1
index db8f81a..66905df 100644
@@ -90,7 +90,6 @@
 /* FMR space for 1 MB of 4k-page transfers, plus 1 if not page aligned */
 #define ISCSI_ISER_SG_TABLESIZE         (((1<<20) >> SHIFT_4K) + 1)
 #define ISCSI_ISER_MAX_LUN		256
-#define ISCSI_ISER_MAX_CMD_LEN		16
 
 /* QP settings */
 /* Maximal bounds on received asynchronous PDUs */
@@ -217,6 +216,7 @@ enum iser_desc_type {
 struct iser_desc {
 	struct iser_hdr              iser_header;
 	struct iscsi_hdr             iscsi_header;
+	char			     hdrextbuf[ISCSI_MAX_AHS_SIZE];
 	struct iser_regd_buf         hdr_regd_buf;
 	void                         *data;         /* used by RX & TX_CONTROL */
 	struct iser_regd_buf         data_regd_buf; /* used by RX & TX_CONTROL */
drivers/infiniband/ulp/iser/iser_initiator.c +9 -5
index 83247f1..ea3f5dc 100644
@@ -246,9 +246,13 @@ post_rx_kmalloc_failure:
 	return err;
 }
 
-/* creates a new tx descriptor and adds header regd buffer */
+/**
+ * iser_create_send_desc - creates a new tx descriptor and adds header regd buffer
+ * @iscsi_hdr_len: length of &struct iscsi_hdr and any AHSes in hdrextbuf
+ */
 static void iser_create_send_desc(struct iscsi_iser_conn *iser_conn,
-				  struct iser_desc       *tx_desc)
+				  struct iser_desc       *tx_desc,
+				  int iscsi_hdr_len)
 {
 	struct iser_regd_buf *regd_hdr = &tx_desc->hdr_regd_buf;
 	struct iser_dto      *send_dto = &tx_desc->dto;
@@ -256,7 +260,7 @@ static void iser_create_send_desc(struct iscsi_iser_conn *iser_conn,
 	memset(regd_hdr, 0, sizeof(struct iser_regd_buf));
 	regd_hdr->device  = iser_conn->ib_conn->device;
 	regd_hdr->virt_addr  = tx_desc; /* == &tx_desc->iser_header */
-	regd_hdr->data_size  = ISER_TOTAL_HEADERS_LEN;
+	regd_hdr->data_size  = sizeof(struct iser_hdr) + iscsi_hdr_len;
 
 	send_dto->ib_conn         = iser_conn->ib_conn;
 	send_dto->notify_enable   = 1;
@@ -342,7 +346,7 @@ int iser_send_command(struct iscsi_conn     *conn,
 	iser_ctask->desc.type = ISCSI_TX_SCSI_COMMAND;
 	send_dto = &iser_ctask->desc.dto;
 	send_dto->ctask = iser_ctask;
-	iser_create_send_desc(iser_conn, &iser_ctask->desc);
+	iser_create_send_desc(iser_conn, &iser_ctask->desc, ctask->hdr_len);
 
 	if (hdr->flags & ISCSI_FLAG_CMD_READ)
 		data_buf = &iser_ctask->data[ISER_DIR_IN];
@@ -435,7 +439,7 @@ int iser_send_data_out(struct iscsi_conn     *conn,
 	/* build the tx desc regd header and add it to the tx desc dto */
 	send_dto = &tx_desc->dto;
 	send_dto->ctask = iser_ctask;
-	iser_create_send_desc(iser_conn, tx_desc);
+	iser_create_send_desc(iser_conn, tx_desc, ctask->hdr_len);
 
 	iser_reg_single(iser_conn->ib_conn->device,
 			send_dto->regd[0], DMA_TO_DEVICE);

drivers/infiniband/ulp/iser/iser_initiator.c int iser_send_control(struct iscsi_conn *conn, · line 502

Quoted 8 lines
drivers/infiniband/ulp/iser/iser_initiator.c +1 -1
@@ -492,7 +496,7 @@ int iser_send_control(struct iscsi_conn *conn,
 	mdesc->type = ISCSI_TX_CONTROL;
 	send_dto = &mdesc->dto;
 	send_dto->ctask = NULL;
-	iser_create_send_desc(iser_conn, mdesc);
+	iser_create_send_desc(iser_conn, mdesc, sizeof(struct iscsi_hdr));
 
 	device = iser_conn->ib_conn->device;
 

I'm afraid the varlen patches to block and scsi-ml are waiting because of me. There are more things I need to check, before they can get approved.

Once I do that, and varlen gets accepted, iSER and iscsi_tcp can go up to 260 for the .max_cmd_len as they should.

Boaz

#12 lore raw

[email protected] wrote on Mon, 18 Feb 2008 17:39 +0200:

Quoted 26 lines · Pete Wyckoff
Quoted 25 lines

From: Pete Wyckoff <[email protected]>
Subject: [PATCH] iscsi iser: varlen

Handle variable-length CDBs in iSER.

---
 drivers/infiniband/ulp/iser/iscsi_iser.c     |    5 +++--
 drivers/infiniband/ulp/iser/iscsi_iser.h     |    2 +-
 drivers/infiniband/ulp/iser/iser_initiator.c |   16 ++++++++++------
 3 files changed, 14 insertions(+), 9 deletions(-)
drivers/infiniband/ulp/iser/iscsi_iser.c +2 -1
index 5f2284d..9dfc310 100644
@@ -401,7 +401,8 @@ iscsi_iser_session_create(struct iscsi_transport *iscsit,
 		ctask      = session->cmds[i];
 		iser_ctask = ctask->dd_data;
 		ctask->hdr = (struct iscsi_cmd *)&iser_ctask->desc.iscsi_header;
-		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header);
+		ctask->hdr_max = sizeof(iser_ctask->desc.iscsi_header) +
+				 sizeof(iser_ctask->desc.hdrextbuf);
 	}
 
Quoted 12 lines

drivers/infiniband/ulp/iser/iscsi_iser.c static struct iscsi_transport iscsi_iser_transport = { · line 608

Quoted 7 lines
drivers/infiniband/ulp/iser/iscsi_iser.c +1 -1
 	for (i = 0; i < session->mgmtpool_max; i++) {
@@ -604,7 +605,7 @@ static struct iscsi_transport iscsi_iser_transport = {
 	.host_template          = &iscsi_iser_sht,
 	.conndata_size		= sizeof(struct iscsi_conn),
 	.max_lun                = ISCSI_ISER_MAX_LUN,
-	.max_cmd_len            = ISCSI_ISER_MAX_CMD_LEN,
+	.max_cmd_len            = 260,

Same bug I had. .max_cmd_len is still char, before the varlen patch to scsi-ml. So it must be at most 252, Until that patch is introduced and it can return to the correct 260 or better yet SCSI_MAX_VARLEN_CDB_SIZE. That also is only defined in the scsi-ml varlen patch.

Ah, that is unfortunate.

Quoted 5 lines

I'm afraid the varlen patches to block and scsi-ml are waiting because of me. There are more things I need to check, before they can get approved.

Once I do that, and varlen gets accepted, iSER and iscsi_tcp can go up to 260 for the .max_cmd_len as they should.

I will sit on these iser changes until we get core varlen resolved, then. Or you can just sequence it all cleverly through the various maintainers.

		-- Pete
#13 lore raw

cover iscsi bidi & varlen support

In reply to Boaz Harrosh
Quoted 26 lines · Boaz Harrosh

Cheers after 1.3 years these can go in.

[PATCH 1/3] iscsi: extended cdb support
The varlen support is not yet in mainline for
block and scsi-ml. But the API for drivers will not change. All LLD need to do is max_command to the it's maximum and be ready for bigger commands. This is what's done here. Once these commands start coming iscsi will be ready for them.

[PATCH 2/3] iscsi: bidi support - libiscsi [PATCH 3/3] iscsi: bidi support - iscsi_tcp
bidirectional commands support in iscsi. iSER is not yet ready, but it will not break. There is already a mechanism in libiscsi that will return error if bidi commands are sent iSER way.

Pete please send me the iSER bits so we can port them to this latest version.

Mike these patches are ontop of iscs branch of the iscsi git tree, they will apply but for compilation you will need to sync with Linus mainline. The patches are for the in-tree iscsi code. I own you the compat patch for the out-off-tree code, but this I will only be Sunday.

Quoted 8 lines

If we do it fast it might get accepted to 2.6.25 merge window

Everybody is invited to a party at Shila ben-yhuda 52 Tel-Aviv 9:45 pm. Drinks and wonderful see-food on us :)

Boaz

-

Everything the same as before. But working this time. Also Pete's comment about second patch, was correct and code is now fixed.

I have got Mike's Signed-off-by, on these they were tested and approved by him. So they are for scsi-misc. But ... James? is there any chance these can go into scsi-rc-fixes for the 2.6.25 kernel? The reason they are so late was mainly because of a fallout in the merge process and a bug that was introduced because of that, but they were intended to go together with bidi into 2.6.25. Also as an important client code to the bidi-api that is introduced in 2.6.25 kernel.

Thanks
Boaz

#14 lore raw

PATCH 1/3 iscsi: extended cdb support

Support for extended CDBs in iscsi.
All we need is to check if command spills over 16 bytes then allocate an iscsi-extended-header for the leftovers.

---
 drivers/scsi/libiscsi.c    |   55 ++++++++++++++++++++++++++++++++++++++++----
 include/scsi/iscsi_proto.h |    6 +++-
 2 files changed, 54 insertions(+), 7 deletions(-)
drivers/scsi/libiscsi.c +50 -5
index 59f8445..a43b8ee 100644
@@ -137,6 +137,45 @@ static int iscsi_add_hdr(struct iscsi_cmd_task *ctask, unsigned len)
 	return 0;
 }
 
+/*
+ * make an extended cdb AHS
+ */
+static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask)
+{
+	struct scsi_cmnd *cmd = ctask->sc;
+	unsigned rlen, pad_len;
+	unsigned short ahslength;
+	struct iscsi_ecdb_ahdr *ecdb_ahdr;
+	int rc;
+
+	ecdb_ahdr = iscsi_next_hdr(ctask);
+	rlen = cmd->cmd_len - ISCSI_CDB_SIZE;
+
+	BUG_ON(rlen > sizeof(ecdb_ahdr->ecdb));
+	ahslength = rlen + sizeof(ecdb_ahdr->reserved);
+
+	pad_len = iscsi_padding(rlen);
+
+	rc = iscsi_add_hdr(ctask, sizeof(ecdb_ahdr->ahslength) +
+	                   sizeof(ecdb_ahdr->ahstype) + ahslength + pad_len);
+	if (rc)
+		return rc;
+
+	if (pad_len)
+		memset(&ecdb_ahdr->ecdb[rlen], 0, pad_len);
+
+	ecdb_ahdr->ahslength = cpu_to_be16(ahslength);
+	ecdb_ahdr->ahstype = ISCSI_AHSTYPE_CDB;
+	ecdb_ahdr->reserved = 0;
+	memcpy(ecdb_ahdr->ecdb, cmd->cmnd + ISCSI_CDB_SIZE, rlen);
+
+	debug_scsi("iscsi_prep_ecdb_ahs: varlen_cdb_len %d "
+		   "rlen %d pad_len %d ahs_length %d iscsi_headers_size %u\n",
+		   cmd->cmd_len, rlen, pad_len, ahslength, ctask->hdr_len);
+
+	return 0;
+}
+
 /**
  * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu
  * @ctask: iscsi cmd task
@@ -150,7 +189,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	struct iscsi_session *session = conn->session;
 	struct iscsi_cmd *hdr = ctask->hdr;
 	struct scsi_cmnd *sc = ctask->sc;
-	unsigned hdrlength;
+	unsigned hdrlength, cmd_len;
 	int rc;
 
 	ctask->hdr_len = 0;
@@ -165,10 +204,16 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	hdr->cmdsn = cpu_to_be32(session->cmdsn);
 	session->cmdsn++;
 	hdr->exp_statsn = cpu_to_be32(conn->exp_statsn);
-	memcpy(hdr->cdb, sc->cmnd, sc->cmd_len);
-	if (sc->cmd_len < MAX_COMMAND_SIZE)
-		memset(&hdr->cdb[sc->cmd_len], 0,
-			MAX_COMMAND_SIZE - sc->cmd_len);
+	cmd_len = sc->cmd_len;
+	if (cmd_len < ISCSI_CDB_SIZE)
+		memset(&hdr->cdb[cmd_len], 0, ISCSI_CDB_SIZE - cmd_len);
+	else if (cmd_len > ISCSI_CDB_SIZE) {
+		rc = iscsi_prep_ecdb_ahs(ctask);
+		if (rc)
+			return rc;
+		cmd_len = ISCSI_CDB_SIZE;
+	}
+	memcpy(hdr->cdb, sc->cmnd, cmd_len);
 
 	ctask->imm_count = 0;
 	if (sc->sc_data_direction == DMA_TO_DEVICE) {
include/scsi/iscsi_proto.h +4 -2
index 5ffec8a..e0593bf 100644
@@ -112,6 +112,7 @@ struct iscsi_ahs_hdr {
 
 #define ISCSI_AHSTYPE_CDB		1
 #define ISCSI_AHSTYPE_RLENGTH		2
+#define ISCSI_CDB_SIZE			16
 
 /* iSCSI PDU Header */
 struct iscsi_cmd {
@@ -125,7 +126,7 @@ struct iscsi_cmd {
 	__be32 data_length;
 	__be32 cmdsn;
 	__be32 exp_statsn;
-	uint8_t cdb[16];	/* SCSI Command Block */
+	uint8_t cdb[ISCSI_CDB_SIZE];	/* SCSI Command Block */
 	/* Additional Data (Command Dependent) */
 };
 
@@ -154,7 +155,8 @@ struct iscsi_ecdb_ahdr {
 	__be16 ahslength;	/* CDB length - 15, including reserved byte */
 	uint8_t ahstype;
 	uint8_t reserved;
-	uint8_t ecdb[260 - 16];	/* 4-byte aligned extended CDB spillover */
+	/* 4-byte aligned extended CDB spillover */
+	uint8_t ecdb[260 - ISCSI_CDB_SIZE];
 };
 
 /* SCSI Response Header */
Signature · 1 line
1.5.3.3
#15 lore raw

PATCH 2/3 iscsi: bidi support - libiscsi

In reply to Boaz Harrosh

iscsi bidi support at the generic libiscsi level
- prepare the additional bidi_read rlength header.
- access the right scsi_in() and/or scsi_out() side of things. also for resid.
- Handle BIDI underflow overflow from target

---
 drivers/scsi/libiscsi.c |   85 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 70 insertions(+), 15 deletions(-)
drivers/scsi/libiscsi.c +70 -15
index a43b8ee..9c12915 100644
@@ -176,6 +176,31 @@ static int iscsi_prep_ecdb_ahs(struct iscsi_cmd_task *ctask)
 	return 0;
 }
 
+static int iscsi_prep_bidi_ahs(struct iscsi_cmd_task *ctask)
+{
+	struct scsi_cmnd *sc = ctask->sc;
+	struct iscsi_rlength_ahdr *rlen_ahdr;
+	int rc;
+
+	rlen_ahdr = iscsi_next_hdr(ctask);
+	rc = iscsi_add_hdr(ctask, sizeof(*rlen_ahdr));
+	if (rc)
+		return rc;
+
+	rlen_ahdr->ahslength =
+		cpu_to_be16(sizeof(rlen_ahdr->read_length) +
+						  sizeof(rlen_ahdr->reserved));
+	rlen_ahdr->ahstype = ISCSI_AHSTYPE_RLENGTH;
+	rlen_ahdr->reserved = 0;
+	rlen_ahdr->read_length = cpu_to_be32(scsi_in(sc)->length);
+
+	debug_scsi("bidi-in rlen_ahdr->read_length(%d) "
+		   "rlen_ahdr->ahslength(%d)\n",
+		   be32_to_cpu(rlen_ahdr->read_length),
+		   be16_to_cpu(rlen_ahdr->ahslength));
+	return 0;
+}
+
 /**
  * iscsi_prep_scsi_cmd_pdu - prep iscsi scsi cmd pdu
  * @ctask: iscsi cmd task
@@ -200,7 +225,6 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	hdr->flags = ISCSI_ATTR_SIMPLE;
 	int_to_scsilun(sc->device->lun, (struct scsi_lun *)hdr->lun);
 	hdr->itt = build_itt(ctask->itt, session->age);
-	hdr->data_length = cpu_to_be32(scsi_bufflen(sc));
 	hdr->cmdsn = cpu_to_be32(session->cmdsn);
 	session->cmdsn++;
 	hdr->exp_statsn = cpu_to_be32(conn->exp_statsn);
@@ -216,7 +240,15 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	memcpy(hdr->cdb, sc->cmnd, cmd_len);
 
 	ctask->imm_count = 0;
+	if (scsi_bidi_cmnd(sc)) {
+		hdr->flags |= ISCSI_FLAG_CMD_READ;
+		rc = iscsi_prep_bidi_ahs(ctask);
+		if (rc)
+			return rc;
+	}
 	if (sc->sc_data_direction == DMA_TO_DEVICE) {
+		unsigned out_len = scsi_out(sc)->length;
+		hdr->data_length = cpu_to_be32(out_len);
 		hdr->flags |= ISCSI_FLAG_CMD_WRITE;
 		/*
 		 * Write counters:
@@ -237,19 +269,19 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 		ctask->unsol_datasn = 0;
 
 		if (session->imm_data_en) {
-			if (scsi_bufflen(sc) >= session->first_burst)
+			if (out_len >= session->first_burst)
 				ctask->imm_count = min(session->first_burst,
 							conn->max_xmit_dlength);
 			else
-				ctask->imm_count = min(scsi_bufflen(sc),
+				ctask->imm_count = min(out_len,
 							conn->max_xmit_dlength);
 			hton24(hdr->dlength, ctask->imm_count);
 		} else
 			zero_data(hdr->dlength);
 
 		if (!session->initial_r2t_en) {
-			ctask->unsol_count = min((session->first_burst),
-				(scsi_bufflen(sc))) - ctask->imm_count;
+			ctask->unsol_count = min(session->first_burst, out_len)
+							     - ctask->imm_count;
 			ctask->unsol_offset = ctask->imm_count;
 		}
 
@@ -259,6 +291,7 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 	} else {
 		hdr->flags |= ISCSI_FLAG_CMD_FINAL;
 		zero_data(hdr->dlength);
+		hdr->data_length = cpu_to_be32(scsi_in(sc)->length);
 
 		if (sc->sc_data_direction == DMA_FROM_DEVICE)
 			hdr->flags |= ISCSI_FLAG_CMD_READ;
@@ -277,10 +310,12 @@ static int iscsi_prep_scsi_cmd_pdu(struct iscsi_cmd_task *ctask)
 		return EIO;
 
 	conn->scsicmd_pdus_cnt++;
-	debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x len %d "
-		"cmdsn %d win %d]\n",
-		sc->sc_data_direction == DMA_TO_DEVICE ? "write" : "read",
-		conn->id, sc, sc->cmnd[0], ctask->itt, scsi_bufflen(sc),
+	debug_scsi("iscsi prep [%s cid %d sc %p cdb 0x%x itt 0x%x "
+		"len %d bidi_len %d cmdsn %d win %d]\n",
+		scsi_bidi_cmnd(sc) ? "bidirectional" :
+		     sc->sc_data_direction == DMA_TO_DEVICE ? "write" : "read",
+		conn->id, sc, sc->cmnd[0], ctask->itt,
+		scsi_bufflen(sc), scsi_bidi_cmnd(sc) ? scsi_in(sc)->length : 0,
 		session->cmdsn, session->max_cmdsn - session->exp_cmdsn + 1);
 	return 0;
 }
@@ -343,7 +378,12 @@ static void fail_command(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask,
 		conn->session->tt->cleanup_cmd_task(conn, ctask);
 
 	sc->result = err;
-	scsi_set_resid(sc, scsi_bufflen(sc));
+	if (!scsi_bidi_cmnd(sc))
+		scsi_set_resid(sc, scsi_bufflen(sc));
+	else {
+		scsi_out(sc)->resid = scsi_out(sc)->length;
+		scsi_in(sc)->resid = scsi_in(sc)->length;
+	}
 	if (conn->ctask == ctask)
 		conn->ctask = NULL;
 	/* release ref from queuecommand */
@@ -478,6 +518,18 @@ invalid_datalen:
 			   min_t(uint16_t, senselen, SCSI_SENSE_BUFFERSIZE));
 	}
 
+	if (rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
+			   ISCSI_FLAG_CMD_BIDI_OVERFLOW)) {
+		int res_count = be32_to_cpu(rhdr->bi_residual_count);
+
+		if (scsi_bidi_cmnd(sc) && res_count > 0 &&
+				(rhdr->flags & ISCSI_FLAG_CMD_BIDI_OVERFLOW ||
+				 res_count <= scsi_in(sc)->length))
+			scsi_in(sc)->resid = res_count;
+		else
+			sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
+	}
+
 	if (rhdr->flags & (ISCSI_FLAG_CMD_UNDERFLOW |
 	                   ISCSI_FLAG_CMD_OVERFLOW)) {
 		int res_count = be32_to_cpu(rhdr->residual_count);
@@ -485,13 +537,11 @@ invalid_datalen:
 		if (res_count > 0 &&
 		    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
 		     res_count <= scsi_bufflen(sc)))
+			/* write side for bidi or uni-io set_resid */
 			scsi_set_resid(sc, res_count);
 		else
 			sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
-	} else if (rhdr->flags & (ISCSI_FLAG_CMD_BIDI_UNDERFLOW |
-	                          ISCSI_FLAG_CMD_BIDI_OVERFLOW))
-		sc->result = (DID_BAD_TARGET << 16) | rhdr->cmd_status;
-
+	}
 out:
 	debug_scsi("done [sc %lx res %d itt 0x%x]\n",
 		   (long)sc, sc->result, ctask->itt);
@@ -1147,7 +1197,12 @@ reject:
 fault:
 	spin_unlock(&session->lock);
 	debug_scsi("iscsi: cmd 0x%x is not queued (%d)\n", sc->cmnd[0], reason);
-	scsi_set_resid(sc, scsi_bufflen(sc));
+	if (!scsi_bidi_cmnd(sc))
+		scsi_set_resid(sc, scsi_bufflen(sc));
+	else {
+		scsi_out(sc)->resid = scsi_out(sc)->length;
+		scsi_in(sc)->resid = scsi_in(sc)->length;
+	}
 	sc->scsi_done(sc);
 	spin_lock(host->host_lock);
 	return 0;
Signature · 1 line
1.5.3.3
#16 lore raw

PATCH 3/3 iscsi: bidi support - iscsi_tcp

In reply to Boaz Harrosh

bidi support for iscsi_tcp
- access the right scsi_in() and/or scsi_out() side of things. also for resid

---
 drivers/scsi/iscsi_tcp.c |   31 +++++++++++++++++--------------
 1 files changed, 17 insertions(+), 14 deletions(-)
drivers/scsi/iscsi_tcp.c +17 -14
index 8a17867..72b9b2a 100644
@@ -528,6 +528,7 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 	struct iscsi_session *session = conn->session;
 	struct scsi_cmnd *sc = ctask->sc;
 	int datasn = be32_to_cpu(rhdr->datasn);
+	unsigned total_in_length = scsi_in(sc)->length;
 
 	iscsi_update_cmdsn(session, (struct iscsi_nopin*)rhdr);
 	if (tcp_conn->in.datalen == 0)
@@ -542,10 +543,10 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 	tcp_ctask->exp_datasn++;
 
 	tcp_ctask->data_offset = be32_to_cpu(rhdr->offset);
-	if (tcp_ctask->data_offset + tcp_conn->in.datalen > scsi_bufflen(sc)) {
+	if (tcp_ctask->data_offset + tcp_conn->in.datalen > total_in_length) {
 		debug_tcp("%s: data_offset(%d) + data_len(%d) > total_length_in(%d)\n",
 		          __FUNCTION__, tcp_ctask->data_offset,
-		          tcp_conn->in.datalen, scsi_bufflen(sc));
+		          tcp_conn->in.datalen, total_in_length);
 		return ISCSI_ERR_DATA_OFFSET;
 	}
 
@@ -558,8 +559,8 @@ iscsi_data_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 
 			if (res_count > 0 &&
 			    (rhdr->flags & ISCSI_FLAG_CMD_OVERFLOW ||
-			     res_count <= scsi_bufflen(sc)))
-				scsi_set_resid(sc, res_count);
+			     res_count <= total_in_length))
+				scsi_in(sc)->resid = res_count;
 			else
 				sc->result = (DID_BAD_TARGET << 16) |
 					rhdr->cmd_status;
@@ -670,11 +671,11 @@ iscsi_r2t_rsp(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 			    r2t->data_length, session->max_burst);
 
 	r2t->data_offset = be32_to_cpu(rhdr->data_offset);
-	if (r2t->data_offset + r2t->data_length > scsi_bufflen(ctask->sc)) {
+	if (r2t->data_offset + r2t->data_length > scsi_out(ctask->sc)->length) {
 		iscsi_conn_printk(KERN_ERR, conn,
 				  "invalid R2T with data len %u at offset %u "
 				  "and total length %d\n", r2t->data_length,
-				  r2t->data_offset, scsi_bufflen(ctask->sc));
+				  r2t->data_offset, scsi_out(ctask->sc)->length);
 		__kfifo_put(tcp_ctask->r2tpool.queue, (void*)&r2t,
 			    sizeof(void*));
 		return ISCSI_ERR_DATALEN;
@@ -771,6 +772,7 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 		if (tcp_conn->in.datalen) {
 			struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data;
 			struct hash_desc *rx_hash = NULL;
+			struct scsi_data_buffer *sdb = scsi_in(ctask->sc);
 
 			/*
 			 * Setup copy of Data-In into the Scsi_Cmnd
@@ -788,8 +790,8 @@ iscsi_tcp_hdr_dissect(struct iscsi_conn *conn, struct iscsi_hdr *hdr)
 				  tcp_ctask->data_offset,
 				  tcp_conn->in.datalen);
 			return iscsi_segment_seek_sg(&tcp_conn->in.segment,
-						     scsi_sglist(ctask->sc),
-						     scsi_sg_count(ctask->sc),
+						     sdb->table.sgl,
+						     sdb->table.nents,
 						     tcp_ctask->data_offset,
 						     tcp_conn->in.datalen,
 						     iscsi_tcp_process_data_in,
@@ -1332,7 +1334,8 @@ iscsi_tcp_ctask_init(struct iscsi_cmd_task *ctask)
 		return 0;
 
 	/* If we have immediate data, attach a payload */
-	err = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc), scsi_sg_count(sc),
+	err = iscsi_tcp_send_data_prep(conn, scsi_out(sc)->table.sgl,
+				       scsi_out(sc)->table.nents,
 				       0, ctask->imm_count);
 	if (err)
 		return err;
@@ -1386,6 +1389,7 @@ iscsi_tcp_ctask_xmit(struct iscsi_conn *conn, struct iscsi_cmd_task *ctask)
 {
 	struct iscsi_tcp_cmd_task *tcp_ctask = ctask->dd_data;
 	struct scsi_cmnd *sc = ctask->sc;
+	struct scsi_data_buffer *sdb = scsi_out(sc);
 	int rc = 0;
 
 flush:
@@ -1412,9 +1416,8 @@ flush:
 				ctask->itt, tcp_ctask->sent, ctask->data_count);
 
 		iscsi_tcp_send_hdr_prep(conn, hdr, sizeof(*hdr));
-		rc = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc),
-					      scsi_sg_count(sc),
-					      tcp_ctask->sent,
+		rc = iscsi_tcp_send_data_prep(conn, sdb->table.sgl,
+					      sdb->table.nents, tcp_ctask->sent,
 					      ctask->data_count);
 		if (rc)
 			goto fail;
@@ -1460,8 +1463,8 @@ flush:
 		iscsi_tcp_send_hdr_prep(conn, &r2t->dtask.hdr,
 					sizeof(struct iscsi_hdr));
 
-		rc = iscsi_tcp_send_data_prep(conn, scsi_sglist(sc),
-					      scsi_sg_count(sc),
+		rc = iscsi_tcp_send_data_prep(conn, sdb->table.sgl,
+					      sdb->table.nents,
 					      r2t->data_offset + r2t->sent,
 					      r2t->data_count);
 		if (rc)
Signature · 1 line
1.5.3.3
maintainer
#17 lore raw
In reply to Boaz Harrosh
Quoted 7 lines · Boaz Harrosh

But ... James? is
there any chance these can go into scsi-rc-fixes for the 2.6.25 kernel? The reason they are so late was mainly because of a fallout in the merge process and a bug that was introduced because of that, but they were intended to go together with bidi into 2.6.25. Also as an important client code to the bidi-api that is introduced in 2.6.25 kernel.

Well, I think you know the answer to that one under Linus' rules for non merge window submission. It's not a bug fix; we haven't even put it into -mm for testing and it's a pretty invasive change.

James

#18 lore raw
Quoted 9 lines · James Bottomley
Quoted 7 lines · Boaz Harrosh

But ... James? is
there any chance these can go into scsi-rc-fixes for the 2.6.25 kernel? The reason they are so late was mainly because of a fallout in the merge process and a bug that was introduced because of that, but they were intended to go together with bidi into 2.6.25. Also as an important client code to the bidi-api that is introduced in 2.6.25 kernel.

Quoted 7 lines

Well, I think you know the answer to that one under Linus' rules for non merge window submission. It's not a bug fix; we haven't even put it into -mm for testing and it's a pretty invasive change.

James

It was extensively tested by all iscsi people. It has the Sign-off-by of the iscsi maintainer. They are not new patches.

But, yes you are right. I now remember the trouble we had with Linus last time. So it's 2.6.26 then. :-( . People that need it for 2.6.25 will just get it off the git tree.

Boaz