Is it time for varlen extended and vendor-specific cdbs

- add varlen_cdb and varlen_cdb_len to hold a large user cdb if needed. They start as empty. Allocation of buffer must be done by user and held until request execution is done.
- Since there can be either a fix_length command up to 16 bytes or a variable_length, larger then 16 bytes, commands but never both, we hold the two types in a union to save space. The

presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb mode.
- Please use added rq_{set,get}_varlen_cdb() to set every thing up in a way that will not confuse drivers that do not support varlen_cdb's.
- Note that this patch does not add any size to struct request since the unsigned cmd_len is split here to 2 ushorts, which is more then enough.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c       |    2 ++
 include/linux/blkdev.h |   27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

block/blk-core.c +0 -0

index 2a438a9..70d32f7 100644

block/blk-core.c +0 -0

 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -0

index 6f79d40..f829571 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */

include/linux/blkdev.h +6 -0

+	};
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -484,6 +491,22 @@ enum {
 #define rq_is_sync(rq)		(rq_data_dir((rq)) == READ || (rq)->cmd_flags & REQ_RW_SYNC)
 #define rq_is_meta(rq)		((rq)->cmd_flags & REQ_RW_META)
 
+static inline void rq_set_varlen_cdb(struct request *rq, u8 *cdb, short cdb_len)
+{
+	rq->cmd_len = 0; /* Make sure legacy drivers don't get confused */
+	rq->varlen_cdb_len = cdb_len;
+	rq->varlen_cdb = cdb;

Quoted 6 lines · Boaz Harrosh

- add varlen_cdb and varlen_cdb_len to hold a large user cdb if needed. They start as empty. Allocation of buffer must be done by user and held until request execution is done.
- Since there can be either a fix_length command up to 16 bytes or a variable_length, larger then 16 bytes, commands but never both, we hold the two types in a union to save space. The

Quoted 8 lines

presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb mode.
- Please use added rq_{set,get}_varlen_cdb() to set every thing up in a way that will not confuse drivers that do not support varlen_cdb's.
- Note that this patch does not add any size to struct request since the unsigned cmd_len is split here to 2 ushorts, which is more then enough.

My first observation is that the block stuff all looks a bit misnamed ... it's not about variable length commands it's about getting block to accept packet commands larger than BLK_MAX_CDB, be they SCSI OSD commands or ATA taskfiles.

Quoted 6 lines

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c       |    2 ++
 include/linux/blkdev.h |   27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

block/blk-core.c void rq_init(struct request_queue *q, struct request *rq) · line 145

Quoted 10 lines

block/blk-core.c +2 -0

index 2a438a9..70d32f7 100644
@@ -141,6 +141,8 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->end_io = NULL;
 	rq->end_io_data = NULL;
 	rq->next_rq = NULL;
+	rq->varlen_cdb_len = 0;
+	rq->varlen_cdb = NULL;

possibly s/varlen/large/

Quoted 12 lines

block/blk-core.c +0 -0

 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -0

index 6f79d40..f829571 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */

include/linux/blkdev.h struct request { · line 223

Quoted 10 lines

include/linux/blkdev.h +8 -2

-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short varlen_cdb_len;  /* length of varlen_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *varlen_cdb;/* an optional variable-length cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */

again s/varlen/large/

Quoted 13 lines

include/linux/blkdev.h +6 -0

+	};
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -484,6 +491,22 @@ enum {
 #define rq_is_sync(rq)		(rq_data_dir((rq)) == READ || (rq)->cmd_flags & REQ_RW_SYNC)
 #define rq_is_meta(rq)		((rq)->cmd_flags & REQ_RW_META)
 
+static inline void rq_set_varlen_cdb(struct request *rq, u8 *cdb, short cdb_len)
+{
+	rq->cmd_len = 0; /* Make sure legacy drivers don't get confused */
+	rq->varlen_cdb_len = cdb_len;
+	rq->varlen_cdb = cdb;

include/linux/blkdev.h enum { · line 508

Quoted 10 lines

include/linux/blkdev.h +10 -0

+}
+
+/* If ! a varlen_cdb than return NULL */
+static inline u8 *rq_get_varlen_cdb(struct request *rq)
+{
+	if (!rq->cmd_len && rq->varlen_cdb_len)
+		return rq->varlen_cdb;
+	else
+		return NULL;
+}

These accessors look wrong ... I think what we want is rq_get_cdb and rq_set_cdb that handles both <= BLK_MAX_CDB by simply a memcpy and > BLK_MAX_CDB by setting up the large_cdb pointer. That way we can accessorize the whole lot and the drivers don't have to care about what BLK_MAX_CDB is set to (it simply becomes an internal convenience of block ... and possibly allows us to eliminate the request carries command data entirely).

include/linux/blkdev.h enum { · line 512

Quoted 4 lines

include/linux/blkdev.h +0 -0

 static inline int blk_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)

James

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

Quoted 6 lines · Boaz Harrosh

- add varlen_cdb and varlen_cdb_len to hold a large user cdb if needed. They start as empty. Allocation of buffer must be done by user and held until request execution is done.
- Since there can be either a fix_length command up to 16 bytes or a variable_length, larger then 16 bytes, commands but never both, we hold the two types in a union to save space. The

Quoted 8 lines

presence of varlen_cdb_len and cmd_len==0 signals a varlen_cdb mode.
- Please use added rq_{set,get}_varlen_cdb() to set every thing up in a way that will not confuse drivers that do not support varlen_cdb's.
- Note that this patch does not add any size to struct request since the unsigned cmd_len is split here to 2 ushorts, which is more then enough.

My first observation is that the block stuff all looks a bit misnamed ... it's not about variable length commands it's about getting block to accept packet commands larger than BLK_MAX_CDB, be they SCSI OSD commands or ATA taskfiles.

Quoted 11 lines

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c       |    2 ++
 include/linux/blkdev.h |   27 +++++++++++++++++++++++++--
 2 files changed, 27 insertions(+), 2 deletions(-)

block/blk-core.c +0 -0

index 2a438a9..70d32f7 100644

block/blk-core.c void rq_init(struct request_queue *q, struct request *rq) · line 145

Quoted 5 lines

block/blk-core.c +2 -0

@@ -141,6 +141,8 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->end_io = NULL;
 	rq->end_io_data = NULL;
 	rq->next_rq = NULL;
+	rq->varlen_cdb_len = 0;
+	rq->varlen_cdb = NULL;

possibly s/varlen/large/

Quoted 11 lines

block/blk-core.c +0 -0

 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -0

index 6f79d40..f829571 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */

include/linux/blkdev.h struct request { · line 223

Quoted 10 lines

include/linux/blkdev.h +8 -2

-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short varlen_cdb_len;  /* length of varlen_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *varlen_cdb;/* an optional variable-length cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */

again s/varlen/large/

Quoted 13 lines

include/linux/blkdev.h +6 -0

+	};
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -484,6 +491,22 @@ enum {
 #define rq_is_sync(rq)		(rq_data_dir((rq)) == READ || (rq)->cmd_flags & REQ_RW_SYNC)
 #define rq_is_meta(rq)		((rq)->cmd_flags & REQ_RW_META)
 
+static inline void rq_set_varlen_cdb(struct request *rq, u8 *cdb, short cdb_len)
+{
+	rq->cmd_len = 0; /* Make sure legacy drivers don't get confused */
+	rq->varlen_cdb_len = cdb_len;
+	rq->varlen_cdb = cdb;

include/linux/blkdev.h enum { · line 508

Quoted 10 lines

include/linux/blkdev.h +10 -0

+}
+
+/* If ! a varlen_cdb than return NULL */
+static inline u8 *rq_get_varlen_cdb(struct request *rq)
+{
+	if (!rq->cmd_len && rq->varlen_cdb_len)
+		return rq->varlen_cdb;
+	else
+		return NULL;
+}

These accessors look wrong ... I think what we want is rq_get_cdb and rq_set_cdb that handles both <= BLK_MAX_CDB by simply a memcpy and > BLK_MAX_CDB by setting up the large_cdb pointer. That way we can accessorize the whole lot and the drivers don't have to care about what BLK_MAX_CDB is set to (it simply becomes an internal convenience of block ... and possibly allows us to eliminate the request carries command data entirely).

include/linux/blkdev.h enum { · line 511

Quoted 3 lines

include/linux/blkdev.h +0 -0

 static inline int blk_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)

James

From 1da591a5ef9697f37604721bfe437a2609bb8217 Mon Sep 17 00:00:00 2001

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Boaz

Quoted 55 lines · Boaz Harrosh

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Quoted 2 lines

Boaz

I forgot to say.
With the proposed way, we are saving the space of the pointer. And the final outcome of eventually eliminating the buffer, is the same.

Let me summarize.
- support extended, arbitrary large commands by introducing the notion that cdb space can be pointed to not carried.
- Eventually transition all block drivers and users to the new system, for all commands not just large ones.
- Do so without introducing any extra cost or instability, but also let the transition be done gradually, and not at once, (hence more stability).

I have thought about that long and hard, my first patch of the matter was 18 month ago. I still think this is the smoothest way to go, and not that ugly, really.

Quoted 37 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Quoted 45 lines · On Sun, 06 Apr 2008 12:35:04 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Quoted 8 lines

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

What are exactly block devices? ub and ide?

bsg are created only for scsi devices (and scsi objects like sas host) now. Are there other means to send commands except for ioctl?

Quoted 57 lines · FUJITA Tomonori

Quoted 53 lines · On Sun, 06 Apr 2008 12:35:04 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Quoted 4 lines

What are exactly block devices? ub and ide?

bsg are created only for scsi devices (and scsi objects like sas host) now. Are there other means to send commands except for ioctl?

I'm not 100% sure either way, so I would like to be safe. Any way, there is the size issue, this way we add *nothing* at all, so it looks preferable. The final outcome will be the same both ways.

I would like if you reconsider the ugliness issue. I admit that at first I personally disliked it, but now that I look at it, I think it is cleaner, coding style, this way. Because the union points out the exclusiveness of the two systems, the striate way give the notion of two separate systems.

Quoted 57 lines · On Sun, 13 Apr 2008 12:13:18 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 56 lines · FUJITA Tomonori

Quoted 53 lines · On Sun, 06 Apr 2008 12:35:04 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Quoted 7 lines

Quoted 4 lines

What are exactly block devices? ub and ide?

bsg are created only for scsi devices (and scsi objects like sas host) now. Are there other means to send commands except for ioctl?

I'm not 100% sure either way, so I would like to be safe. Any way, there is the size issue, this way we add *nothing* at all, so it looks preferable. The final outcome will be the same both ways.

I think that a clean design is an important issue than the sizeof of struct request.

Quoted 4 lines

I would like if you reconsider the ugliness issue. I admit that at first I personally disliked it, but now that I look at it, I think it is cleaner, coding style, this way. Because the union points out the exclusiveness of the two systems, the striate way give the notion of two separate systems.

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Quoted 9 lines · FUJITA Tomonori

On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <[email protected]> wrote:

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

Boaz
--

Quoted 32 lines · Boaz Harrosh

Quoted 9 lines · FUJITA Tomonori

On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <[email protected]> wrote:

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

Quoted 2 lines

Boaz
--

Maybe you mean something like below. I changed cdb => cmd and I use one cmd_len. I think that "cdb" was a good name. Note that we have BLK_MAX_CDB right there next to it. But if you don't like it then I don't mind to change the name, I think it was James idea.

But please note!!!
I think having one cmd_len is DANGEROUS. And it forces a full code audit to be sure, has with my approach we are much more safe. There, I said it.

Quoted 32 lines · On Mon, 14 Apr 2008 12:49:49 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 31 lines · Boaz Harrosh

Quoted 9 lines · FUJITA Tomonori

On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <[email protected]> wrote:

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

Quoted 6 lines

Quoted 2 lines

Boaz
--

Maybe you mean something like below. I changed cdb => cmd and I use one cmd_len. I think that "cdb" was a good name. Note that we have BLK_MAX_CDB right there next to it. But if you don't like it then I don't mind to change the name, I think it was James idea.

I don't. I've been talking mainly about how to represent the length of command.

About 'cdb' name, I thought that there is a better name than BLK_MAX_CDB. But it's up to Jens.

Quoted 3 lines

But please note!!!
I think having one cmd_len is DANGEROUS. And it forces a full code audit to be sure, has with my approach we are much more safe. There, I said it.

Could you be more specific? So far, you have not explained how it can be dangerous.

Quoted 49 lines · FUJITA Tomonori

Quoted 32 lines · On Mon, 14 Apr 2008 12:49:49 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 31 lines · Boaz Harrosh

Quoted 9 lines · FUJITA Tomonori

On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <[email protected]> wrote:

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

Quoted 6 lines

Quoted 2 lines

Boaz
--

Maybe you mean something like below. I changed cdb => cmd and I use one cmd_len. I think that "cdb" was a good name. Note that we have BLK_MAX_CDB right there next to it. But if you don't like it then I don't mind to change the name, I think it was James idea.

I don't. I've been talking mainly about how to represent the length of command.

About 'cdb' name, I thought that there is a better name than BLK_MAX_CDB. But it's up to Jens.

Quoted 6 lines

Quoted 3 lines

But please note!!!
I think having one cmd_len is DANGEROUS. And it forces a full code audit to be sure, has with my approach we are much more safe. There, I said it.

Could you be more specific? So far, you have not explained how it can be dangerous.

There is a pending patch by Pete, and our main goal, is to push OSD commands through bsg. All over the kernel there are cooked up char-devices, like in gdth, that do vendor specific management commands. All these, and future, special-command can go from user-mode through bsg in a common way. Via support of vendor specific and extended commands. All the new scsi defined commands that are bigger then 16 are not going to be sent through sg, but through bsg right? So we do want to use this facility from kernel and from user-land but we need some protection. In scsi LLDs we have .max_cmnd but block devices don't have that. So either you need to restrict what can be done, or play it safe like I did.

Quoted 54 lines · On Mon, 14 Apr 2008 14:28:45 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 48 lines · FUJITA Tomonori

Quoted 32 lines · On Mon, 14 Apr 2008 12:49:49 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 31 lines · Boaz Harrosh

Quoted 9 lines · FUJITA Tomonori

On Sun, 13 Apr 2008 12:13:18 +0300
Boaz Harrosh <[email protected]> wrote:

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

Quoted 6 lines

Quoted 2 lines

Boaz
--

Maybe you mean something like below. I changed cdb => cmd and I use one cmd_len. I think that "cdb" was a good name. Note that we have BLK_MAX_CDB right there next to it. But if you don't like it then I don't mind to change the name, I think it was James idea.

I don't. I've been talking mainly about how to represent the length of command.

About 'cdb' name, I thought that there is a better name than BLK_MAX_CDB. But it's up to Jens.

Quoted 5 lines

Quoted 3 lines

But please note!!!
I think having one cmd_len is DANGEROUS. And it forces a full code audit to be sure, has with my approach we are much more safe. There, I said it.

Could you be more specific? So far, you have not explained how it can be dangerous.

Quoted 10 lines

There is a pending patch by Pete, and our main goal, is to push OSD commands through bsg. All over the kernel there are cooked up char-devices, like in gdth, that do vendor specific management commands. All these, and future, special-command can go from user-mode through bsg in a common way. Via support of vendor specific and extended commands. All the new scsi defined commands that are bigger then 16 are not going to be sent through sg, but through bsg right? So we do want to use this facility from kernel and from user-land but we need some protection. In scsi LLDs we have .max_cmnd but block devices don't have that. So either you need to restrict what can be done, or play it safe like I did.

If we creates bsg devices for block devices, of course, we need to a mechanism to govern the length of commands.

But why do you want to create bsg devices for block devices? If block devices want large commands for some reasons, their bsg hook functions need to handle the length of commands anyway. There is no reason that the block layer has two values to represent one item, the length of command.

If we want to govern the command length in a common place, then I think that it would be better to add the limit to request queue.

And again, what are your block devices? What do they want bsg for? --

Quoted 83 lines · FUJITA Tomonori

Quoted 57 lines · On Sun, 13 Apr 2008 12:13:18 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 56 lines · FUJITA Tomonori

Quoted 53 lines · On Sun, 06 Apr 2008 12:35:04 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 36 lines · Jens Axboe

Quoted 10 lines · Boaz Harrosh

static void req_bio_endio(struct request *rq, struct bio *bio,

include/linux/blkdev.h +0 -1

index 6f79d40..2f87c9d 100644
@@ -213,8 +213,15 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;

include/linux/blkdev.h struct request { · line 224

Quoted 10 lines

include/linux/blkdev.h +9 -1

-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned short ext_cdb_len;  /* length of ext_cdb buffer */
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cdb;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};

Why not just something ala

        unsigned short cmd_len;
        unsigned char __cmd[BLK_MAX_CDB];
        unsigned char *cmd;

and then have rq_init() do

        rq->cmd = rq->__cmd;

and just have a function for setting up a larger ->cmd and adjusting ->cmd_len in the process?

Then rq_set_cdb() would be

Quoted 8 lines

static inline void rq_set_cdb(struct request *rq, u8 *cdb, short cdb_len)

{
        rq->cmd = cdb;
        rq->cmd_len = cdb_len;
}

and rq_get_cdb() plus rq_get_cdb_len() could just go away.

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it is what I've done for scsi, but for block devices we can not do that yet. With the way I did it here, Legacy drivers will see zero length command and will do the right thing, from what I've seen.

Quoted 7 lines

Quoted 4 lines

What are exactly block devices? ub and ide?

bsg are created only for scsi devices (and scsi objects like sas host) now. Are there other means to send commands except for ioctl?

I'm not 100% sure either way, so I would like to be safe. Any way, there is the size issue, this way we add *nothing* at all, so it looks preferable. The final outcome will be the same both ways.

I think that a clean design is an important issue than the sizeof of struct request.

Quoted 4 lines

I would like if you reconsider the ugliness issue. I admit that at first I personally disliked it, but now that I look at it, I think it is cleaner, coding style, this way. Because the union points out the exclusiveness of the two systems, the striate way give the notion of two separate systems.

That's a ugly hack for me.

Why do we have two separate systems to represent the command length? If the command length is smaller than 16 bytes, we use cmd_len. If the length is larger than 16 bytes, we use varlen_cdb_len?

Quoted 8 lines

For me, as Jens proposed, having only cmd_len is the right way.

And 'cdb' name is not appropriate for the block layer, I think.

I agreed that changing the block layer and the scsi midlayer gradually is a safe option. Shortly, I'll send patches to clean up the hack on the top of your patchset.
--

Sorry TOMO, I was sending the ver2 patchset and only saw your mail in the middle, so anyway you have the latest I have now.

If it's ok with you I will squash your patches onto mine and add your sign-off-by. There is no use putting code in the tree that will be changed immediately after.

Please note that I'm a bit afraid to put code that has both length as one if you are more confident then me, I will take your word for it.

Thanks for helping out, as you can see I did it very safe, but with your help maybe it can finally go in. Thanks++

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

Because this way it is dangerous if large commands are issued to legacy drivers. In scsi-land we have .cmd_len at host template that will govern if we are allow to issue larger commands to the driver. In block devices we do not have such a facility, and the danger is if such commands are issued through bsg or other means, even by malicious code. What you say is the ideal and it

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +1 -0

index 7153796..bac5ea1 100644
@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }
 
 EXPORT_SYMBOL(ide_init_drive_cmd);

include/linux/blkdev.h +3 -2

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

Quoted 59 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +1 -0

index 7153796..bac5ea1 100644
@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }
 
 EXPORT_SYMBOL(ide_init_drive_cmd);

include/linux/blkdev.h +3 -2

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 

include/linux/blkdev.h static inline void put_dev_sector(Sector p) · line 822

Quoted 7 lines

include/linux/blkdev.h +7 -0

+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}
+

This is 100% identical to what I suggested be done instead, so of course I agree with this.

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Quoted 6 lines

drivers/ide/ide-io.c +0 -0

 EXPORT_SYMBOL(ide_init_drive_cmd);

include/linux/blkdev.h +0 -0

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {

include/linux/blkdev.h struct request { · line 220

Quoted 8 lines

include/linux/blkdev.h +3 -2

 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;

Quoted 6 lines

include/linux/blkdev.h +0 -0

 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 

include/linux/blkdev.h static inline void put_dev_sector(Sector p) · line 822

Quoted 7 lines

include/linux/blkdev.h +7 -0

+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}
+

This is 100% identical to what I suggested be done instead, so of course I agree with this.

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

static void idetape_init_rq(struct request *rq, u8 cmd)

[ rq can be from privately allocated driver's "stack" so no rq_init() ]

{
	memset(rq, 0, sizeof(*rq));
	rq->cmd_type = REQ_TYPE_SPECIAL;
	rq->cmd[0] = cmd;
}

in ide-tape.c or REQ_TYPE_SENSE in ide-cd.c:

static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
struct request *failed_command)

{
        struct cdrom_info *info         = drive->driver_data;
        struct request *rq              = &info->request_sense_request;

        if (sense == NULL)
                sense = &info->sense_data;

        /* stuff the sense request in front of our current request */
        ide_cd_init_rq(drive, rq);

        rq->data = sense;
        rq->cmd[0] = GPCMD_REQUEST_SENSE;
        rq->cmd[4] = rq->data_len = 18;

        rq->cmd_type = REQ_TYPE_SENSE;

        /* NOTE! Save the failed command in "rq->buffer" */
        rq->buffer = (void *) failed_command;

        (void) ide_do_drive_cmd(drive, rq, ide_preempt);
}

Quoted 6 lines

Quoted 6 lines

drivers/ide/ide-io.c +0 -0

 EXPORT_SYMBOL(ide_init_drive_cmd);

include/linux/blkdev.h +0 -0

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {

Quoted 8 lines

include/linux/blkdev.h struct request { · line 220

Quoted 8 lines

include/linux/blkdev.h +3 -2

 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;

The source issue lies here:

rq->cmd _silently_ becomes something else and unconverted code will happily compile without even a _single_ warning (+ memory corruption / oops later).

This is _guaranteed_ to cause problems:

- overlooked code (like the IDE code above, with the current approach you have to _manually_ audit all code and still _can't_ be sure about the result)

- unmerged code from other trees (i.e., I _have_ WIP patches mapping REQ_TYPE_TASKFILE requests onto rq->cmd)

- out of tree code (in theory we don't care but in this specific case there is no reason to break things silently)

Please just add new field instead (cost should be negligable and if we're concerned about it I see no problem with using unnamed union like it was done by Boaz).

[ Probably it is also worth to add new length field instead of re-using
->cmd_len, just to stay on the safe side (+ for better consistency). ]

Quoted 6 lines

Quoted 6 lines

include/linux/blkdev.h +0 -0

 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

@@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 

Quoted 10 lines

include/linux/blkdev.h static inline void put_dev_sector(Sector p) · line 822

Quoted 7 lines

include/linux/blkdev.h +7 -0

+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}
+

This is 100% identical to what I suggested be done instead, so of course I agree with this.

Jens, I see that you've applied this patch to block tree
- please revert it for now, it needs to be re-designed.

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

Quoted 45 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

I try to convert ide to use blk_get_request properly if you want.

Quoted 11 lines

static void idetape_init_rq(struct request *rq, u8 cmd)

[ rq can be from privately allocated driver's "stack" so no rq_init() ]

{
	memset(rq, 0, sizeof(*rq));
	rq->cmd_type = REQ_TYPE_SPECIAL;
	rq->cmd[0] = cmd;
}

in ide-tape.c or REQ_TYPE_SENSE in ide-cd.c:

Thanks, I overlooked this. As I did for ide_init_drive_cmd, we need:

(quoted diff) +1 -0

+	rq->cmd = rq->__cmd;

Quoted 20 lines

static void cdrom_queue_request_sense(ide_drive_t *drive, void *sense,
struct request *failed_command)

{
        struct cdrom_info *info         = drive->driver_data;
        struct request *rq              = &info->request_sense_request;

        if (sense == NULL)
                sense = &info->sense_data;

        /* stuff the sense request in front of our current request */
        ide_cd_init_rq(drive, rq);

        rq->data = sense;
        rq->cmd[0] = GPCMD_REQUEST_SENSE;
        rq->cmd[4] = rq->data_len = 18;

        rq->cmd_type = REQ_TYPE_SENSE;

Quoted 5 lines

        /* NOTE! Save the failed command in "rq->buffer" */
        rq->buffer = (void *) failed_command;

        (void) ide_do_drive_cmd(drive, rq, ide_preempt);
}

This should work since I put the above hack to ide_init_drive_cmd (I tested the patchset with an ide cdrom drive).

Quoted 20 lines

Quoted 6 lines

Quoted 6 lines

drivers/ide/ide-io.c +0 -0

 EXPORT_SYMBOL(ide_init_drive_cmd);

include/linux/blkdev.h +0 -0

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {

Quoted 8 lines

include/linux/blkdev.h struct request { · line 220

Quoted 8 lines

include/linux/blkdev.h +3 -2

 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;

The source issue lies here:

rq->cmd _silently_ becomes something else and unconverted code will happily compile without even a _single_ warning (+ memory corruption / oops later).

Quoted 5 lines

This is _guaranteed_ to cause problems:

- overlooked code (like the IDE code above, with the current approach you have to _manually_ audit all code and still _can't_ be sure about the result)

As far as I know, only ide does that.

Quoted 5 lines

- unmerged code from other trees (i.e., I _have_ WIP patches mapping REQ_TYPE_TASKFILE requests onto rq->cmd)

- out of tree code (in theory we don't care but in this specific case there is no reason to break things silently)

Again, I think that we can say that we need to use the block layer properly, struct request always needs to be allocated via blk_get_request.

Quoted 6 lines

Please just add new field instead (cost should be negligable and if we're concerned about it I see no problem with using unnamed union like it was done by Boaz).

[ Probably it is also worth to add new length field instead of re-using
->cmd_len, just to stay on the safe side (+ for better consistency). ]

As we discussed, we don't like that hack:

http://marc.info/?t=120724777800003&r=1&w=2

Quoted 45 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

I try to convert ide to use blk_get_request properly if you want.

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +2 -20

index 6f0968f..3f4c563 100644
@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
 	rq->cmd = rq->__cmd;
-	memset(rq->cmd, 0, BLK_MAX_CDB);
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
+EXPORT_SYMBOL(rq_init);
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
 			  unsigned int nbytes, int error)

Signature · 1 line

1.5.3.3

Quoted 54 lines · Boaz Harrosh

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Quoted 60 lines

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +2 -20

index 6f0968f..3f4c563 100644
@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
 	rq->cmd = rq->__cmd;
-	memset(rq->cmd, 0, BLK_MAX_CDB);
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
+EXPORT_SYMBOL(rq_init);
 

Quoted 5 lines

block/blk-core.c +0 -0

 static void req_bio_endio(struct request *rq, struct bio *bio,
 			  unsigned int nbytes, int error)

Signature · 1 line

1.5.3.3

Totally agree, if we expand rq_init() to all users (including on stack), this must be included as well.

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +0 -0

index 6f0968f..3f4c563 100644

block/blk-core.c +1 -20

 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
 	rq->cmd = rq->__cmd;
-	memset(rq->cmd, 0, BLK_MAX_CDB);
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
+EXPORT_SYMBOL(rq_init);
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
 			  unsigned int nbytes, int error)

Signature · 6 lines

1.5.3.3

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

Quoted 86 lines · On Wed, 16 Apr 2008 12:08:25 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +0 -0

index 6f0968f..3f4c563 100644

block/blk-core.c EXPORT_SYMBOL(blk_get_backing_dev_info); · line 116

Quoted 4 lines

block/blk-core.c +1 -0

@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));

Hmm, rq_init comment says:

/*
* We can't just memset() the structure, since the allocation path
* already stored some information in the request.
*/

I think that we can't initialize rq->cmd_flags here.

Quoted 101 lines · FUJITA Tomonori

Quoted 86 lines · On Wed, 16 Apr 2008 12:08:25 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
      rq->errors = 0;
      rq->ref_count = 1;
      rq->cmd_len = 0;
+     rq->cmd = rq->__cmd;
      memset(rq->cmd, 0, BLK_MAX_CDB);
      rq->data_len = 0;
      rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
      memset(rq, 0, sizeof(*rq));
      rq->ref_count = 1;
+     rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +0 -0

index 6f0968f..3f4c563 100644

block/blk-core.c EXPORT_SYMBOL(blk_get_backing_dev_info); · line 116

Quoted 4 lines

block/blk-core.c +1 -0

@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+   memset(rq, 0, sizeof(*rq));

Hmm, rq_init comment says:

/*
* We can't just memset() the structure, since the allocation path
* already stored some information in the request.
*/

Quoted 1 line

I think that we can't initialize rq->cmd_flags here.

That is correct, the patch wont work as-is. The principle of clearing every member except block internal is sound and should be applied, though.

Quoted 105 lines · Jens Axboe

Quoted 102 lines · FUJITA Tomonori

Quoted 86 lines · On Wed, 16 Apr 2008 12:08:25 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
      rq->errors = 0;
      rq->ref_count = 1;
      rq->cmd_len = 0;
+     rq->cmd = rq->__cmd;
      memset(rq->cmd, 0, BLK_MAX_CDB);
      rq->data_len = 0;
      rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
      memset(rq, 0, sizeof(*rq));
      rq->ref_count = 1;
+     rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +0 -0

index 6f0968f..3f4c563 100644

block/blk-core.c EXPORT_SYMBOL(blk_get_backing_dev_info); · line 116

Quoted 4 lines

block/blk-core.c +1 -0

@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+   memset(rq, 0, sizeof(*rq));

Hmm, rq_init comment says:

/*
* We can't just memset() the structure, since the allocation path
* already stored some information in the request.
*/

I think that we can't initialize rq->cmd_flags here.

Quoted 3 lines

That is correct, the patch wont work as-is. The principle of clearing every member except block internal is sound and should be applied, though.

While we're at it we may fix it as well (preferably in another pre-patch):

- move setting rq->cmd_flags from blk_alloc_request() to the callers (this is only get_request() ATM)

- re-order setting rq->cmd_flags vs calling rq_init() in get_request(), queue_lfush() and start_ordered()

Quoted 101 lines · FUJITA Tomonori

Quoted 86 lines · On Wed, 16 Apr 2008 12:08:25 +0300 Boaz Harrosh <[email protected]> wrote:

Quoted 53 lines · Jens Axboe

Quoted 52 lines · FUJITA Tomonori

Quoted 43 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 7 lines

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

Just a patch that I had for ages since the bad old request bidi times, perhaps is also good today. (rebased to for-2.6.26 branch)
---
From: Boaz Harrosh <[email protected]>
Date: Wed, 16 Apr 2008 12:05:33 +0300
Subject: [PATCH] Initialize all members of struct request in rq_init

Before, every member added/removed from struct request would entitle a change to rq_init, for initialization. Now all members are default to zero and only the none zero members are specifically initialized.

Users that need requests on the stack or pre-allocated, must call rq_init() before use.

Signed-off-by Boaz Harrosh <[email protected]>

---
 block/blk-core.c |   22 ++--------------------
 1 files changed, 2 insertions(+), 20 deletions(-)

block/blk-core.c +0 -0

index 6f0968f..3f4c563 100644

block/blk-core.c EXPORT_SYMBOL(blk_get_backing_dev_info); · line 116

Quoted 4 lines

block/blk-core.c +1 -0

@@ -113,36 +113,18 @@ EXPORT_SYMBOL(blk_get_backing_dev_info);
  */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));

Hmm, rq_init comment says:

/*
* We can't just memset() the structure, since the allocation path
* already stored some information in the request.
*/

Quoted 1 line

I think that we can't initialize rq->cmd_flags here.

That is correct, the patch wont work as-is. The principle of clearing every member except block internal is sound and should be applied, though.

Quoted 55 lines · FUJITA Tomonori

Quoted 45 lines · On Wed, 16 Apr 2008 00:50:54 +0200 Bartlomiej Zolnierkiewicz…

Hi,

Quoted 30 lines · Jens Axboe

Quoted 29 lines · FUJITA Tomonori

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

---
 block/blk-core.c       |    1 +
 drivers/ide/ide-io.c   |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 3 files changed, 12 insertions(+), 2 deletions(-)

block/blk-core.c +1 -0

index 6669238..6f0968f 100644
@@ -132,6 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
+	rq->cmd = rq->__cmd;
 	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;

drivers/ide/ide-io.c +0 -0

index 7153796..bac5ea1 100644

Quoted 8 lines

drivers/ide/ide-io.c void ide_init_drive_cmd (struct request *rq) · line 1599

Quoted 8 lines

drivers/ide/ide-io.c +1 -0

@@ -1595,6 +1595,7 @@ void ide_init_drive_cmd (struct request *rq)
 {
 	memset(rq, 0, sizeof(*rq));
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }

Tomo, some more changes are needed:

Quoted 2 lines

Please think about all _static_/dynamic allocations of 'struct request' used together with REQ_TYPE_SPECIAL etc., i.e.

I think that using struct request allocated statically is wrong from the perspective of the block layer design, that is, you always need to use blk_get_request. I think that except ide, everyone does.

Quoted 1 line

I try to convert ide to use blk_get_request properly if you want.

That would be best, but the on-stack allocation has the benefit that it'll always work. So until we can completely get rid of that, lets just make it a hard rule that ANY rq allocation MUST call rq_init(). It's a lot saner than doing a memset() anyway.

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

include/linux/blkdev.h +3 -2

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

[email protected] wrote on Mon, 14 Apr 2008 19:50 +0900:

Quoted 9 lines

This patch changes rq->cmd from the static array to a pointer to support large commands.

We rarely handle large commands. So for optimization, a struct request still has a static array for a command. rq_init sets rq->cmd pointer to the static array.

Signed-off-by FUJITA Tomonori <[email protected]>

Cc Jens Axboe <[email protected]>

[..]

Quoted 17 lines

include/linux/blkdev.h +3 -2

index b3a58ad..5710ae4 100644
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */

include/linux/blkdev.h static inline void put_dev_sector(Sector p) · line 821

Quoted 9 lines

include/linux/blkdev.h +6 -0

@@ -812,6 +813,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 
+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}

Here's one way this will be used, in a patched bsg that understands large commands. Complication is the need to copy and hold onto the big command across the duration of the request.

Submit time is fairly clean:

	/* buf, len from user request */
	rq = blk_get_request(..);
	rq->cmd_len = len;
	if (len > BLK_MAX_CDB) {
		rq->cmd = kmalloc(len);
		if (rq->cmd == NULL)
			goto out;
	}
	copy_from_user(rq->cmd, buf, len);

Completion time needs to know when to free rq->cmd:

	if (rq->cmd_len > BLK_MAX_CDB)
		kfree(rq->cmd);
	blk_put_request(rq);

Could use (rq->cmd != rq->__cmd) instead, but nothing had better ever touch rq->cmd_len.

I don't think the helper rq_set_cmd() will be very useful, as the caller (bsg) must think about allocation of the command buffer if it is big.

One option would be to handle allocation/freeing of the big command in rq_set_... functions, but I don't think you want to constrain the interface like that.

Boaz's concern about big rq->cmd_len still worries me, although I think this approach is better and worth solving bugs in drivers as they arise. It only matters in the case that someone adds, say, a bsg interface to all block devices though. The queuecommand of ub shows a good example of how this will break.

In sum, this is a cleaner approach, and a bit easier for callers with long commands to deal with. And you could get rid of the trivial helper.