Re: [PATCH 2/3 ver2] block layer extended-cdb support

[Boaz Harrosh <bharrosh@panasas.com>]

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:
> 
>> On Sat, Apr 12 2008 at 8:52 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>> On Sun, 06 Apr 2008 12:35:04 +0300
>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>
>>>> On Fri, Apr 04 2008 at 14:46 +0300, Jens Axboe <jens.axboe@oracle.com> wrote:
>>>>> On Thu, Apr 03 2008, Boaz Harrosh wrote:
>>>>>>  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..2f87c9d 100644
>>>>>> --- a/include/linux/blkdev.h
>>>>>> +++ b/include/linux/blkdev.h
>>>>>> @@ -213,8 +213,15 @@ 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 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.
>>>>>
>>>> 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?
>> 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.
> 
> 
>> 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.
> --

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