Re: [PATCH scsi-misc-2.6 01/13] scsi: don't useblk_insert_request() for requeueing

[James Bottomley]

On Fri, 2005-04-01 at 14:01 +0900, Tejun Heo wrote:
> > Well, REQ_SPECIAL is the signal to the mid-layer that we've allocated
> > the resources necessary to process the command, so in practice it will
> > be turned on for every requeue request (because we set it when the
> > command is prepared),
> 
>  Sorry, but where?  AFAICT, only blk_insert_request() and
> scsi_init_io() on sgtable allocation failure set REQ_SPECIAL during
> scsi midlayer processing.  This patch replaces blk_insert_request()
> with blk_requeue_request() and the next patch removes REQ_SPECIAL
> setting in scsi_init_io().
> 
>  REQ_SPECIAL is currently overloaded to mean two different things.
> 
>  * The request is a special request.
>  * The request has been requeued using scsi_queue_insert().
>    i.e. It has been prepped.

But its true meaning is defined by the block layer (since it's a block
flag).  It's supposed to mean that the ->special field of the request is
in use to carry data meaningful to the underlying driver.  SCSI sets it
on that basis.

So, if I understand correctly, based on the fact that the current block
code in fact never bothers with REQ_SPECIAL, but only checks req-
>special, you're proposing that we need never actually set REQ_SPECIAL
when making use of the ->special field?  Thus you want to use
REQ_SPECIAL to distinguish between internally generated commands and
external commands?  That sounds fine as long as the block API gets
updated to reflect this (comments in linux/blkdev.h shoudl be fine).

> > The other reason I don't like this is that we've been trying hard to
> > sweep excess block knowledge out of the mid-layer.  I don't think
> > REQ_SOFTBARRIER is anything we really have to know about.
> 
>  We currently requeue using two different block functions.
> 
>  * blk_insert_request(): this function does two things
> 	1. enqueue special requests
> 	2. turn on REQ_SPECIAL|REQ_SOFTBARRIER and call
> 	   blk_requeue_request().  This is used only by scsi midlayer.
>  * blk_requeue_request()
> 
>  REQ_SOFTBARRIER tells the request queue that other requests shouldn't
> pass this one.  We need this to be set for prepped requests;
> otherwise, it may, theoretically, deadlock (unprepped request waiting
> for the prepped request back in the queue).  So, the current code
> 
>  * depends on blk_insert_request() sets REQ_SOFTBARRIER when
>    requeueing.  It works but nothing in the interface or semantics
>    is clear about it.  Why expect a request reinserted using
>    blk_insert_request() gets REQ_SOFTBARRIER turned on while a request
>    reinserted with blk_requeue_request() doesn't?  or why are there
>    two different paths doing mostly the same thing with slightly
>    different semantics?
>  * requeueing using blk_requeue_request() in scsi_request_fn() doesn't
>    turn on either REQ_SPECIAL or REQ_SOFTBARRIER.  Missing REQ_SPECIAL
>    is okay, as we have the extra path in prep_fn (if it ever gets requeued
>    due to medium failure, so gets re-prepped), but missing
>    REQ_SOFTBARRIER can *theoretically* cause problem.
> 
>  So, it's more likely becoming less dependent on unobvious behavior of
> block layer.  As we need the request to stay on top, we tell the block
> layer to do so, instead of depending on blk_insert_request()
> unobviously doing it for us.

But that's the point.  This is non obvious behaviour in the block layer,
so SCSI doesn't want to know about it (and the block maintainer won't
want to trawl through the SCSI and other block drivers altering it if it
changes).  If the bug is that the block layer isn't setting it on all
our requeues where it should, then either we're using the wrong API or
the block layer API is buggy.

James


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/