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

[FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>]

On Sun, 13 Apr 2008 19:50:26 +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:
> > 
> >> 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.

There is no mergeable stuff between yours and my patchset. I've just
sent four patches. The first three patches are not related your
patchset (they are cleanups for changing rq->cmd from the static array
to a pointer). The fourth patch is a substitute for your second
patch. Nothing are common between them.


> 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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html