Re: [PATCH 0/7] Discard requests, v2

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

On Tue, Aug 12 2008, David Woodhouse wrote:
> On Tue, 2008-08-12 at 11:14 +0200, Jens Axboe wrote:
> > On Sat, Aug 09 2008, David Woodhouse wrote:
> > > This time on top of the for-2.6.28 branch of
> > > git://git.kernel.dk/linux-2.6-block.git
> > > 
> > > I've made it cope with merging and sorting discard requests, still as a
> > > separate patch at the end of the sequence. I don't think we have a
> > > problem with discards passing writes in the queue, any more than we
> > > _already_ had a problem with writes passing writes.
> > 
> > But we don't already have this problem, that is the point. For page
> > cache writes, the page cache nicely solves this issue for us - a write
> > that comes in later gets to wait on the page lock before proceeding. So
> > at least it's ordered. For O_DIRECT, the issuer is on his own.
> > 
> > I think this is a serious problem and that we must ensure that an
> > overlapping write doesn't pass a previously issued discard request. So
> > in that sense, discards must be considered soft barriers.
> 
> OK, that makes sense. Enabling merges is now the last commit in
> git.infradead.org/users/dwmw2/discard-2.6.git so we can easily drop it
> for now. Now that I've updated the FAT code to merge discard requests
> for contiguous chains, it's not a massive issue.

You can leave the merging, if we make it a barrier then that'll prevent
it from being merged anyway.

I'm fine with providing both a BIO_RW_DISCARD and
BIO_RW_DISCARD_NOBARRIER (or something like that) so that we at least
now the default is safe. If the file systems takes care of the ordering,
then it can use the less restricted BIO_RW_DISCARD_NOBARRIER instead.

> I'd still quite like to make it work though -- file systems that care
> can always use explicit barriers to ensure that the DISCARD requests are
> handled before any subsequent reallocation and write to the same
> sectors.

Agree, we should cater to those as well.

> If we want to allow that option for those who submit bios directly, is
> it sufficient to set BIO_RW_SYNC to the flags in blkdev_issue_discard,
> or do we really need REQ_SOFTBARRIER? There doesn't seem to be a way to
> indicate that in ->bi_rw.

BIO_RW_SYNC wont help you much, it'll just ensure that it wont go
through a plug cycle. We've never had bio softbarriers before, hence
there's just the one flag for barriers. The easy fix is would just be to
change init_request_from_bio() to do something ala:

if (bio_barrier(bio)) {
        if (bio_has_data(bio))
                req->cmd_flags |= REQ_HARDBARRIER;
        else
                req->cmd_flags |= REQ_SOFTBARRIER;
}

since there's no real data dependency when you don't have data attached.

-- 
Jens Axboe