All of lore.kernel.org
 help / color / mirror / Atom feed
From: Boaz Harrosh <bharrosh@panasas.com>
To: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Cc: jens.axboe@oracle.com, James.Bottomley@HansenPartnership.com,
	hch@infradead.org, linux-scsi@vger.kernel.org,
	akpm@linux-foundation.org
Subject: [PATCH 2/3 ver3] block layer extended-cdb support
Date: Mon, 14 Apr 2008 12:49:49 +0300	[thread overview]
Message-ID: <480328BD.7050206@panasas.com> (raw)
In-Reply-To: <480239D2.3040206@panasas.com>

On Sun, Apr 13 2008 at 19:50 +0300, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On Sun, Apr 13 2008 at 19:17 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>> On Sun, 13 Apr 2008 12:13:18 +0300
>> Boaz Harrosh <bharrosh@panasas.com> 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?
>>
>> 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
> --
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.

---
From: Boaz Harrosh <bharrosh@panasas.com>
Date: Wed, 31 Oct 2007 14:56:09 +0200
Subject: [PATCH] block layer extended-command support

  - add ext_cmd to hold a large external cdb if needed. 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 an external extended command but never both, we hold the two
    types in a union to save space. cmd_len > BLK_MAX_CDB signals
    an ext_cmd mode.
  - Use new rq_set_cmd() to set everything properly.
  - Use rq_get_cmd() to retrieve the command in a generic way.
  - Once all drivers and users convert to the new accessors the cmd[]
    16 bytes buffer can be discarded.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-core.c       |    2 ++
 include/linux/blkdev.h |   25 ++++++++++++++++++++++++-
 2 files changed, 26 insertions(+), 1 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..a2bf07f 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -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->cmd_len = 0;
+	rq->ext_cmd = NULL;
 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 6f79d40..0c4f5f5 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -214,7 +214,13 @@ struct request {
 	 * when request is used as a packet command carrier
 	 */
 	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	union {
+		unsigned char cmd[BLK_MAX_CDB];
+		unsigned char *ext_cmd;/* an optional extended cdb.
+	                                   * points to a user buffer that must
+	                                   * be valid until end of request
+	                                   */
+	};
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */
@@ -484,6 +490,23 @@ 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_cmd(struct request *rq, u8 *cmd, short cmd_len)
+{
+	rq->cmd_len = cmd_len;
+	if (cmd_len <= BLK_MAX_CDB)
+		memcpy(rq->cmd, cmd, cmd_len);
+	else
+		rq->ext_cmd = cmd;
+}
+
+static inline u8 *rq_get_cmd(struct request *rq)
+{
+	if (rq->cmd_len <= BLK_MAX_CDB)
+		return rq->cmd;
+	else
+		return rq->ext_cmd;
+}
+
 static inline int blk_queue_full(struct request_queue *q, int rw)
 {
 	if (rw == READ)
-- 
1.5.3.3

 




  reply	other threads:[~2008-04-14  9:50 UTC|newest]

Thread overview: 76+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2008-03-25 16:21 [PATCHSET 0/3] Is it time for varlen extended and vendor-specific cdbs Boaz Harrosh
2008-03-25 16:22 ` [PATCH] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
2008-03-25 16:32 ` [PATCH 2/3] block layer varlen-cdb Boaz Harrosh
2008-04-03 16:43   ` James Bottomley
2008-04-03 17:26     ` Benny Halevy
2008-04-03 18:32     ` [PATCH 2/3 ver2] block layer extended-cdb support Boaz Harrosh
2008-04-04 11:46       ` Jens Axboe
2008-04-06  9:35         ` Boaz Harrosh
2008-04-06 11:05           ` Boaz Harrosh
2008-04-07  8:31             ` Jens Axboe
2008-04-12  5:52           ` FUJITA Tomonori
2008-04-13  9:13             ` Boaz Harrosh
2008-04-13 16:17               ` FUJITA Tomonori
2008-04-13 16:50                 ` Boaz Harrosh
2008-04-14  9:49                   ` Boaz Harrosh [this message]
2008-04-14 11:04                     ` [PATCH 2/3 ver3] " FUJITA Tomonori
2008-04-14 11:28                       ` Boaz Harrosh
2008-04-14 12:08                         ` FUJITA Tomonori
2008-04-14 12:22                           ` Boaz Harrosh
2008-04-14 11:04                   ` [PATCH 2/3 ver2] " FUJITA Tomonori
2008-04-14 10:50                 ` [PATCH 0/4] add large command support to the block layer FUJITA Tomonori
2008-04-14 10:50                   ` [PATCH 1/4] block: no need to initialize rq->cmd in prepare_flush_fn hook FUJITA Tomonori
2008-04-14 10:50                     ` [PATCH 2/4] block: no need to initialize rq->cmd with blk_get_request FUJITA Tomonori
2008-04-14 10:50                       ` [PATCH 3/4] block: replace sizeof(rq->cmd) with BLK_MAX_CDB FUJITA Tomonori
2008-04-14 10:50                         ` [PATCH 4/4] block: add large command support FUJITA Tomonori
2008-04-14 11:29                           ` Jens Axboe
2008-04-14 12:08                             ` FUJITA Tomonori
2008-04-15 22:50                             ` Bartlomiej Zolnierkiewicz
2008-04-15 22:57                               ` FUJITA Tomonori
2008-04-16  0:22                                 ` Bartlomiej Zolnierkiewicz
2008-04-16  8:33                                 ` Jens Axboe
2008-04-16  9:08                                   ` Boaz Harrosh
2008-04-16  9:42                                     ` Jens Axboe
2008-04-16 22:28                                       ` Bartlomiej Zolnierkiewicz
2008-04-17  3:59                                     ` FUJITA Tomonori
2008-04-17  7:07                                       ` Jens Axboe
2008-04-17 11:55                                         ` Bartlomiej Zolnierkiewicz
2008-04-17 11:58                                           ` Bartlomiej Zolnierkiewicz
2008-04-17 12:07                                         ` FUJITA Tomonori
2008-04-17  4:02                                   ` FUJITA Tomonori
2008-04-14 14:41                           ` Pete Wyckoff
2008-04-14 22:33                             ` FUJITA Tomonori
2008-04-15 13:44                               ` Pete Wyckoff
2008-04-15  7:45                             ` Boaz Harrosh
2008-04-15 10:05                               ` FUJITA Tomonori
2008-04-15  7:29                           ` Jens Axboe
2008-04-14 11:21                   ` [PATCH 0/4] add large command support to the block layer FUJITA Tomonori
2008-04-14 11:38                   ` Boaz Harrosh
2008-04-14 12:36                     ` Boaz Harrosh
2008-04-14 13:06                       ` FUJITA Tomonori
2008-04-15 12:24                   ` [PATCH 0/3] scsi: variable-length CDBs support Boaz Harrosh
2008-04-15 12:30                     ` [PATCH 1/3] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
2008-04-15 12:34                     ` [PATCH 2/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh
2008-04-16  2:09                       ` FUJITA Tomonori
2008-04-16  6:40                         ` Boaz Harrosh
2008-04-16  6:49                           ` [PATCH 2/3 ver2] " Boaz Harrosh
2008-04-17  4:01                           ` [PATCH 2/3] " FUJITA Tomonori
2008-04-17 12:25                             ` [PATCH 2/3 ver3] " Boaz Harrosh
2008-04-17 12:49                               ` Boaz Harrosh
2008-04-17 13:04                               ` FUJITA Tomonori
2008-04-17 13:29                                 ` Boaz Harrosh
2008-04-15 12:37                     ` [PATCH 3/3] iscsi_tcp: Enable large commands Boaz Harrosh
2008-04-15 13:08                       ` James Smart
2008-04-15 13:38                         ` Boaz Harrosh
2008-04-15 13:57                           ` Benny Halevy
2008-04-15 13:46                         ` FUJITA Tomonori
2008-04-13 14:07             ` [PATCH 2/3 ver2] block layer extended-cdb support James Bottomley
2008-04-13 16:17               ` FUJITA Tomonori
2008-03-25 16:36 ` [PATCH 3/3] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh
2008-04-03 16:07 ` [PATCHSET 0/3] Is it time for " Boaz Harrosh
2008-04-13 16:30 ` [PATCHSET 0/4 ver2] " Boaz Harrosh
2008-04-13 16:37   ` [PATCH 1/4] Let scsi_cmnd->cmnd use request->cmd buffer Boaz Harrosh
2008-04-13 16:39   ` [PATCH 2/4] block layer extended-cdb support Boaz Harrosh
2008-04-13 16:39   ` [PATCH 3/4] scsi: varlen extended and vendor-specific cdbs Boaz Harrosh
2008-04-13 16:41   ` [PATCH 4/4] iscsi_tcp: Enable large command Boaz Harrosh
2008-04-18 17:11     ` Mike Christie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=480328BD.7050206@panasas.com \
    --to=bharrosh@panasas.com \
    --cc=James.Bottomley@HansenPartnership.com \
    --cc=akpm@linux-foundation.org \
    --cc=fujita.tomonori@lab.ntt.co.jp \
    --cc=hch@infradead.org \
    --cc=jens.axboe@oracle.com \
    --cc=linux-scsi@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.