Re: [PATCH 1/5] [BLOCK] Add 'discard' request handling

[Jens Axboe <jens.axboe@oracle.com>]

On Fri, Aug 08 2008, David Woodhouse wrote:
> On Thu, 2008-08-07 at 20:41 +0200, Jens Axboe wrote:
> > I've queued this up for some testing, I'll add the merge support as
> > well.
> 
> Thanks. Did you pull from my tree? If so I'll provide an incremental
> patch. Otherwise I'll go back and recommit it with your suggested
> changes.

I did not, the final version will be different from the 1st one, so no
point in carrying that history. What I did this morning was add the
bio_has_data() support bits, here:

http://git.kernel.dk/?p=linux-2.6-block.git;a=shortlog;h=refs/heads/for-2.6.28

> > > diff --git a/block/blk-barrier.c b/block/blk-barrier.c
> > > index a09ead1..29e60ff 100644
> > > --- a/block/blk-barrier.c
> > > +++ b/block/blk-barrier.c
> > 
> > Not sure why you are placing it here, it should probably just go into
> > blk-core.c
> 
> Yeah, I vacillated about that for a while -- and even got as far as
> moving it there, and back again. It's not really _core_ code either.
> Since it started off almost identical to blkdev_issue_flush() I figured
> it might as well sit next to it. But I'm happy to move it too.

It doesn't matter a whole lot, so lets just keep it there. None of the
files are a perfect fit, and it would be silly to start a new one for
this.

> > > diff --git a/block/elevator.c b/block/elevator.c
> > > index ed6f8f3..bb26424 100644
> > > --- a/block/elevator.c
> > > +++ b/block/elevator.c
> > > @@ -675,7 +675,7 @@ void __elv_add_request(struct request_queue *q, struct request *rq, int where,
> > >  	if (q->ordcolor)
> > >  		rq->cmd_flags |= REQ_ORDERED_COLOR;
> > >  
> > > -	if (rq->cmd_flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER)) {
> > > +	if (rq->cmd_flags & (REQ_SOFTBARRIER | REQ_HARDBARRIER | REQ_DISCARD)) {
> > >  		/*
> > >  		 * toggle ordered color
> > >  		 */
> > 
> > Just make REQ_DISCARD set REQ_SOFTBARRIER? Do you care if this acts as a
> > barrier or not?
> 
> Er, didn't you object to me setting REQ_SOFTBARRIER for discard
> requests, a few lines up? Admittedly, I shouldn't need to do _both_, but
> I think I need one or the other.

Not at all, please re-read that part. I objected to adding a strategy
hook just for setting cmd type and flags for discard. Basically I think
this boils down to the fact that we only really pass fs requests through
submit_bio(). Other types of requests are typically inserted directly
into the queue, if they aren't "normal" file system requests. Since we
want to do merging on discard requests, that isn't a good idea here.

The question here is whether you want discard to be a barrier or not.
Either you mark it as such when the rq is inited or you don't, don't
check for it explicitly in __elv_add_request().

> In the long term no, I don't care if it acts as a barrier. I just did
> that until we implement the merge support for discard requests.

OK

-- 
Jens Axboe