Re: [PATCH linux-2.6-block:post-2.6.15 08/10] blk: update IDE to use new blk_ordered

[Tejun Heo <htejun@gmail.com>]

On Tue, Nov 22, 2005 at 09:36:09AM +0100, Bartlomiej Zolnierkiewicz wrote:
[--snip--]
> >
> > Ordered requests are processed in the following order.
> >
> > 1. barrier bio reaches blk queue
> >
> > 2. barrier req queued in order
> >
> > 3. when barrier req reaches the head of the request queue, it gets
> >    interpreted into preflush-barrier-postflush requests sequence
> >    and queued.  ->prepare_flush_fn is called in this step.
> >
> > 4. When all three requests complete, the ordered sequence ends.
> >
> > Adding !drive->wcache test to idedisk_prepare_flush, which in turn
> > requires adding ->prepare_flush_fn error handling to blk ordered
> > handling, prevents flushes for barrier requests between step#1 and
> 
> Why for !drive->wcache flush can't be consider as successful
> like it was before these changes...
> 
> > step#3.  We can still have flush reqeuests between #3 and #4 after
> > wcache is turned off.
> 
> ditto
> 

I think we have two alternatives here - both have some problems.

1. make ->prepare_flush_fn return some code to tell blk layer skip
   the flush as the original code did.

This is what you're proposing, I guess.  The reason why I'm reluctant
to take this approach is that there still remains window of error
between #3 and #4.  The flush requests could already be prepared and
in the queue when ->wcache is turned off.  AFAICS, the original code
had the same problem, although the window was narrower.

2. complete flush commands successfully in execute_drive_cmd() if wcache
   is not enabled.

This approach fixes all cases but the implementation would be a bit
hackish.  execute_drive_cmd() will have to look at the command and
ide_disk->wcache to determine if a special command should be completed
successfully without actually executing it.  Maybe we can add a
per-HL-driver callback for that.

Bartlomiej, I'm not really sure about both of above approaches.  My
humble opinion is that benefits brought by both of above aren't worth
the added complexity.  The worst can happen is a few IDE command
failures caused by executing flush command on a wcache disabled drive.
And that would happen only when the user turns off wcache while
barrier sequence is *in progress*.

Hmmm... What do you think?  It's your call.  I'll do what you choose.

[--snip--]

> > I agree that it should go into ->release, but I am still a bit scared
> > about issuing commands in ->release (it might access some data
> > structure which might be gone by then).  Also, the correct order seems
> > to be 'turning off ordered' and then 'perform the last cache flush'.
> > So, how about adding blk_queue_ordered right above the last
> > ide_cacheflush_p() now and move both to ->release in a separate patch
> > for both IDE and SCSI?
> 
> Not needed, when ide-disk is fixed to call del_gendisk() after
> ide_cacheflush_p(), we can add blk_queue_orderer() before
> the latter and then everything should be OK.

Can you get the patch into the post-2.6.15 branch of linux-2.6-block
tree?

Thanks.  :-)

-- 
tejun