Skip to contentSkip to conversation

bidi, varlen, bsg success report

PATCH New Pete Wyckoff 2 participants 9 messages Dec 22, 2007 – Dec 23, 2007

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

I spent the last couple of days rebasing my working set of kernel patches from 2.6.22-rc5 to 2.6.24-rc6. I want to use the BSG interface from userspace to submit variable-length bidirectional SCSI commands to targets via iSCSI TCP.

Many thanks to all the work with scsi data accessors and iscsi that makes this much easier, and to Boaz for continuing to maintain patches. Here's what I needed to do to make it work.

Start with linus-2.6.24-rc6.
Merge scsi-misc.
Apply the following outstanding patches from Boaz:

bidi

    scsi: scsi_data_buffer
    http://marc.info/?l=linux-scsi&m=119754650614813&w=2

    scsi: bidi support
    http://marc.info/?l=linux-scsi&m=119754667915055&w=2

varlen

    [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer
    http://marc.info/?l=linux-scsi&m=119798523823750&w=2

    [PATCH 2/2] block layer varlen-cdb
    http://marc.info/?l=linux-scsi&m=119798548624218&w=2

    [PATCH 3/3] scsi: varlen extended and vendor-specific cdbs
    http://marc.info/?l=linux-scsi&m=119798563924476&w=2

Then a few more, two modified from earlier Boaz patches, and three new ones by me, mainly to make bsg work. They follow.

1. iscsi: extended cdb support
Adapted from Boaz
http://marc.info/?l=linux-scsi&m=119394074127857&w=2

2. iscsi: bidi support
Adapted from two old Boaz patches that can be found in this git:
http://www.bhalevy.com/git/gitweb.cgi?p=open-osd/.git;a=commitdiff;h=b5a97649eadb9434acbc4170a8aebd962af6d960
http://www.bhalevy.com/git/gitweb.cgi?p=open-osd/.git;a=commitdiff;h=cf3d4684a22bde6787f2eccee66d7b0ee9c9182c

3. bsg: accept varlen commands

4. bsg: bidi fix

5. iscsi tcp: bidi capable

Suggestions: Boaz is probably the best qualified to shepherd his outstanding five patches plus the two others I mention above (#1, #2). The first bsg patch (#3) won't apply without the varlen patches. The last two (#4, #5) are harmless and can go in now, but won't matter until the bidi patches are in. Comments on these welcome.

Hope this is helpful. I'm definitely looking forward to seeing all this merged into mainline eventually.

		-- Pete
#1 lore raw

PATCH 1/5 iscsi: extended cdb support

From: Boaz Harrosh <[email protected]>

Once varlen cdbs are supported by the block and scsi-ml layers we can apply this patch to support extended CDBs in iscsi.

---
 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 e5be5fd..9fde5ce 100644
@@ -1973,7 +1973,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		= SCSI_MAX_VARLEN_CDB_SIZE,
 	/* 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 318a909..8c591ac 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.6
#2 lore raw

PATCH 1/5 iscsi: extended cdb support

In reply to Pete Wyckoff

[Resend, date was in ancient history.]

From: Boaz Harrosh <[email protected]>

Once varlen cdbs are supported by the block and scsi-ml layers we can apply this patch to support extended CDBs in iscsi.

---
 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 e5be5fd..9fde5ce 100644
@@ -1973,7 +1973,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		= SCSI_MAX_VARLEN_CDB_SIZE,
 	/* 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 318a909..8c591ac 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.6
#3 lore raw

PATCH 2/5 iscsi bidi support

In reply to Pete Wyckoff

From: Boaz Harrosh <[email protected]>

Adapted from separate libiscsi and TCP patches by Boaz to update for iscsi changes and iscsi-tcp rewrite. Dropped some needless conversion to scsi_out() in some places. Original log follows.

Once bidi support is accepted at block and scsi-ml layers
below bidi support to iscsi can be applied.
The assumed bidi API from scsi-ml is based on scsi_sgtables
but it is not at all far from any other implementation as far
as iscsi code is concerned:
- scsi_bidi_cmd() - return true if the command is bidi. Note
that this is a deviation from previous RFCs
in that we no longer have sc_dma_direction
== DMA_BIDIRECTIONAL. The command direction is DMA_TO_DEVICE but with this flag on.
- scsi_{in,out}() - Which will return the right sg_table for IN or OUT

		    operation, in a generic way.
		    The small difference here due to sgtable implementation
		    is that in the past we had scsi_in()->sglist == NULL for
		    DMA_NONE and now we have scsi_in() == NULL.

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.

---
 drivers/scsi/iscsi_tcp.c |   14 ++++---
 drivers/scsi/libiscsi.c  |   81 +++++++++++++++++++++++++++++++++++++++------
 2 files changed, 78 insertions(+), 17 deletions(-)
drivers/scsi/iscsi_tcp.c +8 -6
index 9fde5ce..373639a 100644
@@ -542,10 +542,11 @@ 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 >
+							scsi_in(sc)->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, scsi_in(sc)->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 <= scsi_in(sc)->length))
+				scsi_in(sc)->resid = res_count;
 			else
 				sc->result = (DID_BAD_TARGET << 16) |
 					rhdr->cmd_status;
@@ -783,8 +784,9 @@ 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),
+						     scsi_in(ctask->sc)->sglist,
+						     scsi_in(ctask->sc)->
+								sg_count,
 						     tcp_ctask->data_offset,
 						     tcp_conn->in.datalen,
 						     iscsi_tcp_process_data_in,
drivers/scsi/libiscsi.c +70 -11

149 lines not on this page · raw message

Signature · 1 line
1.5.3.6
#4 lore raw

PATCH 3/5 bsg: accept varlen commands

In reply to Pete Wyckoff

Accept variable length SCSI commands through BSG.

---
 block/bsg.c |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)
block/bsg.c +25 -9
index 8e181ab..0d9364d 100644
@@ -175,14 +175,29 @@ unlock:
 static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
 				struct sg_io_v4 *hdr, int has_write_perm)
 {
-	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
-
-	if (copy_from_user(rq->cmd, (void *)(unsigned long)hdr->request,
-			   hdr->request_len))
-		return -EFAULT;
+	void __user *buf = (void __user *)(unsigned long) hdr->request;
+	int len = hdr->request_len;
+	unsigned char *cmd;
+
+	if (len > BLK_MAX_CDB) {
+		rq->cmd_len = 0;
+		rq->varlen_cdb_len = len;
+		rq->varlen_cdb = kmalloc(len, GFP_KERNEL);
+		if (rq->varlen_cdb == NULL)
+			return -ENOMEM;
+		if (copy_from_user(rq->varlen_cdb, buf, len))
+			return -EFAULT;
+		cmd = &rq->varlen_cdb[0];
+	} else {
+		rq->cmd_len = len;
+		memset(rq->cmd, 0, BLK_MAX_CDB);  /* for ATAPI */
+		if (copy_from_user(rq->cmd, buf, len))
+			return -EFAULT;
+		cmd = &rq->cmd[0];
+	}
 
 	if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
-		if (blk_verify_command(rq->cmd, has_write_perm))
+		if (blk_verify_command(cmd, has_write_perm))
 			return -EPERM;
 	} else if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
@@ -190,7 +205,6 @@ static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr->request_len;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	rq->timeout = (hdr->timeout * HZ) / 1000;
@@ -212,8 +226,6 @@ bsg_validate_sgv4_hdr(struct request_queue *q, struct sg_io_v4 *hdr, int *rw)
 
 	if (hdr->guard != 'Q')
 		return -EINVAL;
-	if (hdr->request_len > BLK_MAX_CDB)
-		return -EINVAL;
 	if (hdr->dout_xfer_len > (q->max_sectors << 9) ||
 	    hdr->din_xfer_len > (q->max_sectors << 9))
 		return -EIO;
@@ -302,6 +314,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 	}
 	return rq;
 out:
+	if (rq->varlen_cdb_len)
+		kfree(rq->varlen_cdb);
 	blk_put_request(rq);
 	if (next_rq) {
 		blk_rq_unmap_user(next_rq->bio);
@@ -446,6 +460,8 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 		hdr->dout_resid = rq->data_len;
 
 	blk_rq_unmap_user(bio);
+	if (rq->varlen_cdb_len)
+		kfree(rq->varlen_cdb);
 	blk_put_request(rq);
 
 	return ret;
Signature · 1 line
1.5.3.6
#5 lore raw

PATCH 3/5 bsg: accept varlen commands

In reply to Pete Wyckoff

[Resend, date was in ancient history.]

Accept variable length SCSI commands through BSG.

---
 block/bsg.c |   34 +++++++++++++++++++++++++---------
 1 files changed, 25 insertions(+), 9 deletions(-)
block/bsg.c +25 -9
index 8e181ab..0d9364d 100644
@@ -175,14 +175,29 @@ unlock:
 static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
 				struct sg_io_v4 *hdr, int has_write_perm)
 {
-	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
-
-	if (copy_from_user(rq->cmd, (void *)(unsigned long)hdr->request,
-			   hdr->request_len))
-		return -EFAULT;
+	void __user *buf = (void __user *)(unsigned long) hdr->request;
+	int len = hdr->request_len;
+	unsigned char *cmd;
+
+	if (len > BLK_MAX_CDB) {
+		rq->cmd_len = 0;
+		rq->varlen_cdb_len = len;
+		rq->varlen_cdb = kmalloc(len, GFP_KERNEL);
+		if (rq->varlen_cdb == NULL)
+			return -ENOMEM;
+		if (copy_from_user(rq->varlen_cdb, buf, len))
+			return -EFAULT;
+		cmd = &rq->varlen_cdb[0];
+	} else {
+		rq->cmd_len = len;
+		memset(rq->cmd, 0, BLK_MAX_CDB);  /* for ATAPI */
+		if (copy_from_user(rq->cmd, buf, len))
+			return -EFAULT;
+		cmd = &rq->cmd[0];
+	}
 
 	if (hdr->subprotocol == BSG_SUB_PROTOCOL_SCSI_CMD) {
-		if (blk_verify_command(rq->cmd, has_write_perm))
+		if (blk_verify_command(cmd, has_write_perm))
 			return -EPERM;
 	} else if (!capable(CAP_SYS_RAWIO))
 		return -EPERM;
@@ -190,7 +205,6 @@ static int blk_fill_sgv4_hdr_rq(struct request_queue *q, struct request *rq,
 	/*
 	 * fill in request structure
 	 */
-	rq->cmd_len = hdr->request_len;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 	rq->timeout = (hdr->timeout * HZ) / 1000;
@@ -212,8 +226,6 @@ bsg_validate_sgv4_hdr(struct request_queue *q, struct sg_io_v4 *hdr, int *rw)
 
 	if (hdr->guard != 'Q')
 		return -EINVAL;
-	if (hdr->request_len > BLK_MAX_CDB)
-		return -EINVAL;
 	if (hdr->dout_xfer_len > (q->max_sectors << 9) ||
 	    hdr->din_xfer_len > (q->max_sectors << 9))
 		return -EIO;
@@ -302,6 +314,8 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 	}
 	return rq;
 out:
+	if (rq->varlen_cdb_len)
+		kfree(rq->varlen_cdb);
 	blk_put_request(rq);
 	if (next_rq) {
 		blk_rq_unmap_user(next_rq->bio);
@@ -446,6 +460,8 @@ static int blk_complete_sgv4_hdr_rq(struct request *rq, struct sg_io_v4 *hdr,
 		hdr->dout_resid = rq->data_len;
 
 	blk_rq_unmap_user(bio);
+	if (rq->varlen_cdb_len)
+		kfree(rq->varlen_cdb);
 	blk_put_request(rq);
 
 	return ret;
Signature · 1 line
1.5.3.6
#6 lore raw

PATCH 4/5 bsg: bidi fix

In reply to Pete Wyckoff

Fixes for bsg to handle bidirectional commands. The next_rq part of a bidi command must be marked REQ_TYPE_BLOCK_PC for scsi_init_sgtable.

---
 block/bsg.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
block/bsg.c +1 -0
index 0d9364d..fc3a024 100644
@@ -291,6 +291,7 @@ bsg_map_hdr(struct bsg_device *bd, struct sg_io_v4 *hdr)
 			goto out;
 		}
 		rq->next_rq = next_rq;
+		next_rq->cmd_type = REQ_TYPE_BLOCK_PC;
 
 		dxferp = (void*)(unsigned long)hdr->din_xferp;
 		ret =  blk_rq_map_user(q, next_rq, dxferp, hdr->din_xfer_len);
Signature · 1 line
1.5.3.6
#7 lore raw

PATCH 5/5 iscsi tcp bidi capable

In reply to Pete Wyckoff

Mark iscsi_tcp as being capable of bidirectional transfers. Perhaps instead we could remove the check in bsg, as that is the only entry to SCSI that bothers to check the flag.

---
 drivers/scsi/iscsi_tcp.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
drivers/scsi/iscsi_tcp.c +7 -0
index 373639a..ecae470 100644
@@ -1915,6 +1915,12 @@ static void iscsi_tcp_session_destroy(struct iscsi_cls_session *cls_session)
 	iscsi_session_teardown(cls_session);
 }
 
+static int iscsi_tcp_slave_alloc(struct scsi_device *sdev)
+{
+	set_bit(QUEUE_FLAG_BIDI, &sdev->request_queue->queue_flags);
+	return 0;
+}
+
 static int iscsi_tcp_slave_configure(struct scsi_device *sdev)
 {
 	blk_queue_bounce_limit(sdev->request_queue, BLK_BOUNCE_ANY);
@@ -1936,6 +1942,7 @@ static struct scsi_host_template iscsi_sht = {
 	.eh_host_reset_handler	= iscsi_eh_host_reset,
 	.use_clustering         = DISABLE_CLUSTERING,
 	.use_sg_chaining	= ENABLE_SG_CHAINING,
+	.slave_alloc		= iscsi_tcp_slave_alloc,
 	.slave_configure        = iscsi_tcp_slave_configure,
 	.proc_name		= "iscsi_tcp",
 	.this_id		= -1,
Signature · 1 line
1.5.3.6
#8 lore raw
In reply to Pete Wyckoff
Quoted 55 lines · Pete Wyckoff

I spent the last couple of days rebasing my working set of kernel patches from 2.6.22-rc5 to 2.6.24-rc6. I want to use the BSG interface from userspace to submit variable-length bidirectional SCSI commands to targets via iSCSI TCP.

Many thanks to all the work with scsi data accessors and iscsi that makes this much easier, and to Boaz for continuing to maintain patches. Here's what I needed to do to make it work.

Start with linus-2.6.24-rc6.
Merge scsi-misc.
Apply the following outstanding patches from Boaz:

bidi

    scsi: scsi_data_buffer
    http://marc.info/?l=linux-scsi&m=119754650614813&w=2

    scsi: bidi support
    http://marc.info/?l=linux-scsi&m=119754667915055&w=2

varlen

    [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer
    http://marc.info/?l=linux-scsi&m=119798523823750&w=2

    [PATCH 2/2] block layer varlen-cdb
    http://marc.info/?l=linux-scsi&m=119798548624218&w=2

    [PATCH 3/3] scsi: varlen extended and vendor-specific cdbs
    http://marc.info/?l=linux-scsi&m=119798563924476&w=2

Then a few more, two modified from earlier Boaz patches, and three new ones by me, mainly to make bsg work. They follow.

1. iscsi: extended cdb support
Adapted from Boaz
http://marc.info/?l=linux-scsi&m=119394074127857&w=2

2. iscsi: bidi support
Adapted from two old Boaz patches that can be found in this git:
http://www.bhalevy.com/git/gitweb.cgi?p=open-osd/.git;a=commitdiff;h=b5a97649eadb9434acbc4170a8aebd962af6d960
http://www.bhalevy.com/git/gitweb.cgi?p=open-osd/.git;a=commitdiff;h=cf3d4684a22bde6787f2eccee66d7b0ee9c9182c

3. bsg: accept varlen commands

4. bsg: bidi fix

5. iscsi tcp: bidi capable

Suggestions: Boaz is probably the best qualified to shepherd his outstanding five patches plus the two others I mention above (#1, #2). The first bsg patch (#3) won't apply without the varlen patches. The last two (#4, #5) are harmless and can go in now, but won't matter until the bidi patches are in. Comments on these welcome.

Quoted 5 lines

Hope this is helpful. I'm definitely looking forward to seeing all this merged into mainline eventually.

		-- Pete

-

Dear peter.

This is grate to know. I know from passed experience that code might work well for your own set of tests, but otherwise hide bugs.

Please forgive me about the iSCSI patches, I have an updated set here in my trees, but I have not posted them, because I thought that no one cares, but me.

I did not yet get any principle OK about the varlen API. that is - scsi_cmnd->cmnd changes to a pointer that points to either a static buffer at request or a dynamic buffer at ULD - Because if this principle is accepted, than please Note that the patch "iscsi: extended cdb support" can be applied now, before the actual cleanups and implementation are applied.
I attributed the complete silence treatment from both block and scsi maintainers, to the holiday session. I hope they come with renewed strength next year ;)

I will compare our iscsi patches and put a freshen set at:
git://git.bhalevy.com/open-osd osd_dev
or on the web
http://git.bhalevy.com/git/gitweb.cgi?p=open-osd/.git;a=summary
(give me until the end of today)

I have inspected your other patches, and they all look good. I will include them in above git.

Happy holidays
Boaz