Re: Regarding posted scsi midlyaer patchsets

Re: Regarding posted scsi midlyaer patchsets

[Tejun Heo]

 Hello, James.
 Hello, Jens.

James Bottomley wrote:
> On Mon, 2005-04-18 at 07:41 +0900, Tejun Heo wrote:
> 
>> As it's been almost a week since I posted scsi midlayer patchsets and
>>haven't heard anything yet, I've been wondering what's going on.  Are
>>they under review or all dropped?  If they are dropped, can you please
>>tell me why they are dropped?
> 
> 
> I have about four of them in the scsi-misc-2.6 tree, if you look.
> 
> Your request path rewrite I already gave you feedback that I didn't want
> REQ_SOFTBARRIER in scsi ... it needs to be in the block submit API for
> special requests.  Also, you have a patch for block in this code so I
> can't apply it without an ack from Jens.  And all the rest of your
> patches depend on this one.

 This thread started as an private inquiry to James regarding the status
of four patchsets I posted about a week ago.  I'm replying publicly as I
think we can use some discussion.  The four patchsets are... (in the
following order)

 * timer updates
 * REQ_SPECIAL/REQ_SOFTBARRIER usage change
 * scsi_request_fn reimpl
 * requeue path consolidation.

 Accepted patches are

 * scsi_cmnd->internal_timeout kill
 * scsi_cmnd->serial_number_at_timeout
 * remove volatile
 * scsi_send_eh_cmnd() clean up

 All four accepted patches are not included in any of above patchsets
and the timer update patchset doesn't depend on
REQ_SPECIAL/REQ_SOFTBARRIER usage change patchset, so please review the
timer update patchset.

 And, James, regarding REQ_SOFTBARRIER, if the REQ_SOFTBARRIER thing can
be removed from SCSI midlayer, do you agree to change REQ_SPECIAL to
mean special requests?  If so, I have three proposals.

 * move REQ_SOFTBARRIER setting to right after the allocation of
scsi_cmnd in scsi_prep_fn().  This will be the only place where
REQ_SOFTBARRIER is used in SCSI midlayer, making it less pervasive.
 * Or, make another API which sets REQ_SOFTBARRIER on requeue.  maybe
blk_requeue_ordered_request()?
 * Or, make blk_insert_request() not set REQ_SPECIAL on requeue.  IMHO,
this is a bit too subtle.

 I like #1 or #2.  Jens, what do you think?  Do you agree to remove
requeue feature from blk_insert_request()?

 Thanks a lot. :-)

-- 
tejun

Re: Regarding posted scsi midlyaer patchsets

[Jens Axboe]

On Mon, Apr 18 2005, Tejun Heo wrote:
>  And, James, regarding REQ_SOFTBARRIER, if the REQ_SOFTBARRIER thing can
> be removed from SCSI midlayer, do you agree to change REQ_SPECIAL to
> mean special requests?  If so, I have three proposals.
> 
>  * move REQ_SOFTBARRIER setting to right after the allocation of
> scsi_cmnd in scsi_prep_fn().  This will be the only place where
> REQ_SOFTBARRIER is used in SCSI midlayer, making it less pervasive.
>  * Or, make another API which sets REQ_SOFTBARRIER on requeue.  maybe
> blk_requeue_ordered_request()?
>  * Or, make blk_insert_request() not set REQ_SPECIAL on requeue.  IMHO,
> this is a bit too subtle.
> 
>  I like #1 or #2.  Jens, what do you think?  Do you agree to remove
> requeue feature from blk_insert_request()?

#2 is the best, imho. We really want to maintain ordering on requeue
always, marking it softbarrier automatically in the block layer means
the io schedulers don't have to do anything specific to handle it.

I have no problem with removing the requeue stuff from
blk_insert_request(). That function is horribly weird as it is, it is
supposed to look generic but is really just a scsi special case.

-- 
Jens Axboe

Re: Regarding posted scsi midlyaer patchsets

[James Bottomley]

On Tue, 2005-04-19 at 14:34 +0200, Jens Axboe wrote:
> On Mon, Apr 18 2005, Tejun Heo wrote:
> >  And, James, regarding REQ_SOFTBARRIER, if the REQ_SOFTBARRIER thing can
> > be removed from SCSI midlayer, do you agree to change REQ_SPECIAL to
> > mean special requests?  If so, I have three proposals.
> > 
> >  * move REQ_SOFTBARRIER setting to right after the allocation of
> > scsi_cmnd in scsi_prep_fn().  This will be the only place where
> > REQ_SOFTBARRIER is used in SCSI midlayer, making it less pervasive.
> >  * Or, make another API which sets REQ_SOFTBARRIER on requeue.  maybe
> > blk_requeue_ordered_request()?
> >  * Or, make blk_insert_request() not set REQ_SPECIAL on requeue.  IMHO,
> > this is a bit too subtle.
> > 
> >  I like #1 or #2.  Jens, what do you think?  Do you agree to remove
> > requeue feature from blk_insert_request()?
> 
> #2 is the best, imho. We really want to maintain ordering on requeue
> always, marking it softbarrier automatically in the block layer means
> the io schedulers don't have to do anything specific to handle it.

This is my preference too.  In general, block is the only one that
should care what the REQ_SOFTBARRIER flag actually means.  SCSI only
cares that it submits a non mergeable request.

I'm happy to separate the meaning of REQ_SPECIAL from req->special.

> I have no problem with removing the requeue stuff from
> blk_insert_request(). That function is horribly weird as it is, it is
> supposed to look generic but is really just a scsi special case.

heh .. would this be because no other driver uses the block layer for
requeuing ... ?

James

Re: Regarding posted scsi midlyaer patchsets

[Jens Axboe]

On Tue, Apr 19 2005, James Bottomley wrote:
> On Tue, 2005-04-19 at 14:34 +0200, Jens Axboe wrote:
> > On Mon, Apr 18 2005, Tejun Heo wrote:
> > >  And, James, regarding REQ_SOFTBARRIER, if the REQ_SOFTBARRIER thing can
> > > be removed from SCSI midlayer, do you agree to change REQ_SPECIAL to
> > > mean special requests?  If so, I have three proposals.
> > > 
> > >  * move REQ_SOFTBARRIER setting to right after the allocation of
> > > scsi_cmnd in scsi_prep_fn().  This will be the only place where
> > > REQ_SOFTBARRIER is used in SCSI midlayer, making it less pervasive.
> > >  * Or, make another API which sets REQ_SOFTBARRIER on requeue.  maybe
> > > blk_requeue_ordered_request()?
> > >  * Or, make blk_insert_request() not set REQ_SPECIAL on requeue.  IMHO,
> > > this is a bit too subtle.
> > > 
> > >  I like #1 or #2.  Jens, what do you think?  Do you agree to remove
> > > requeue feature from blk_insert_request()?
> > 
> > #2 is the best, imho. We really want to maintain ordering on requeue
> > always, marking it softbarrier automatically in the block layer means
> > the io schedulers don't have to do anything specific to handle it.
> 
> This is my preference too.  In general, block is the only one that
> should care what the REQ_SOFTBARRIER flag actually means.  SCSI only
> cares that it submits a non mergeable request.
> 
> I'm happy to separate the meaning of REQ_SPECIAL from req->special.

Isn't it just duplicate information anyways? I mean, just clear
->special if it isn't valid anymore. Having a seperate flag to indicate
this seems a little suboptimal. It made more sense when ->cmd was a
integer being READ, WRITE, etc. But as a seperate state now it doesn't.

> > I have no problem with removing the requeue stuff from
> > blk_insert_request(). That function is horribly weird as it is, it is
> > supposed to look generic but is really just a scsi special case.
> 
> heh .. would this be because no other driver uses the block layer for
> requeuing ... ?

Not so much that, more that it isn't very clean. It sets rq flags,
assigns ->special, insert the request and then runs the queue. It does
way too much. Either the caller wants a requeue, so he calls
blk_requeue_request(). Or call blk_insert_request(), which should just
do what the name indicates and not a whole bunch of other stuff. Then
add a nicely named function for actually running the queue:

void blk_kick_queue_handling(request_queue_t *q)
{
        if (blk_queue_plugged(q))
                __generic_unplug_device(q);
        else
                q->request_fn(q);
}

(with a better name, I cannot come up with one just now :-)

Yep, this requires you do do the ->special assignment and the queue run
in the caller, but I rather like that compared to a function that you
have to look up in source everytime because you don't know exactly how
much it does.

-- 
Jens Axboe

Re: Regarding posted scsi midlyaer patchsets

[Jens Axboe]

On Tue, Apr 19 2005, Jens Axboe wrote:
> On Tue, Apr 19 2005, James Bottomley wrote:
> > On Tue, 2005-04-19 at 14:34 +0200, Jens Axboe wrote:
> > > On Mon, Apr 18 2005, Tejun Heo wrote:
> > > >  And, James, regarding REQ_SOFTBARRIER, if the REQ_SOFTBARRIER thing can
> > > > be removed from SCSI midlayer, do you agree to change REQ_SPECIAL to
> > > > mean special requests?  If so, I have three proposals.
> > > > 
> > > >  * move REQ_SOFTBARRIER setting to right after the allocation of
> > > > scsi_cmnd in scsi_prep_fn().  This will be the only place where
> > > > REQ_SOFTBARRIER is used in SCSI midlayer, making it less pervasive.
> > > >  * Or, make another API which sets REQ_SOFTBARRIER on requeue.  maybe
> > > > blk_requeue_ordered_request()?
> > > >  * Or, make blk_insert_request() not set REQ_SPECIAL on requeue.  IMHO,
> > > > this is a bit too subtle.
> > > > 
> > > >  I like #1 or #2.  Jens, what do you think?  Do you agree to remove
> > > > requeue feature from blk_insert_request()?
> > > 
> > > #2 is the best, imho. We really want to maintain ordering on requeue
> > > always, marking it softbarrier automatically in the block layer means
> > > the io schedulers don't have to do anything specific to handle it.
> > 
> > This is my preference too.  In general, block is the only one that
> > should care what the REQ_SOFTBARRIER flag actually means.  SCSI only
> > cares that it submits a non mergeable request.
> > 
> > I'm happy to separate the meaning of REQ_SPECIAL from req->special.
> 
> Isn't it just duplicate information anyways? I mean, just clear
> ->special if it isn't valid anymore. Having a seperate flag to indicate
> this seems a little suboptimal. It made more sense when ->cmd was a
> integer being READ, WRITE, etc. But as a seperate state now it doesn't.

Oh, and this is only true of SCSI, btw. REQ_SPECIAL should not be seen
outside of driver code, its meaning is defined solely by the driver.
SCSI ties it to ->special, but that is not necessarily true for any
other driver.

-- 
Jens Axboe