Re: [PATCH 1/7] block: Add block_flush_device()

[Chris Mason]

On Mon, 2009-03-30 at 15:16 -0400, Jeff Garzik wrote:
> Jens Axboe wrote:
> > On Mon, Mar 30 2009, Linus Torvalds wrote:
> >>
> >> On Mon, 30 Mar 2009, Jens Axboe wrote:
> >>> The problem is that we may not know upfront, so it sort-of has to be
> >>> this trial approach where the first barrier issued will notice and fail
> >>> with -EOPNOTSUPP.
> >> Well, absolutely. Except I don't think you shoul use ENOTSUPP, you should 
> >> just set a bit in the "struct request_queue", and then return 0.
> >>
> >> IOW, something like this
> >>
> >> 	--- a/block/blk-barrier.c
> >> 	+++ b/block/blk-barrier.c
> >> 	@@ -318,6 +318,9 @@ int blkdev_issue_flush(struct block_device *bdev, sector_t *error_sector)
> >> 	 	if (!q)
> >> 	 		return -ENXIO;
> >> 	 
> >> 	+	if (is_queue_noflush(q))
> >> 	+		return 0;
> >> 	+
> >> 	 	bio = bio_alloc(GFP_KERNEL, 0);
> >> 	 	if (!bio)
> >> 	 		return -ENOMEM;
> >> 	@@ -339,7 +342,7 @@ int blkdev_issue_flush(struct block_device *bdev, sector_t *error_sector)
> >> 	 
> >> 	 	ret = 0;
> >> 	 	if (bio_flagged(bio, BIO_EOPNOTSUPP))
> >> 	-		ret = -EOPNOTSUPP;
> >> 	+		set_queue_noflush(q);
> >> 	 	else if (!bio_flagged(bio, BIO_UPTODATE))
> >> 	 		ret = -EIO;
> >> 	 
> >>
> >> which just returns 0 if we don't support flushing on that queue.
> >>
> >> (Obviously incomplete patch, which is why I also intentionally 
> >> whitespace-broke it).
> >>
> >>> Sure, we could cache this value, but it's pretty
> >>> pointless since the filesystem will stop sending barriers in this case.
> >> Well no, it won't. Or rather, it will have to have such a stupid 
> >> per-filesystem flag, for no good reason.
> > 
> > Sorry, I just don't see much point to doing it this way instead. So now
> > the fs will have to check a queue bit after it has issued the flush, how
> > is that any better than having the 'error' returned directly?
> 
> AFAICS, the aim is simply to return zero rather than EOPNOTSUPP, for the 
> not-supported case, rather than burdening all callers with such checks.
> 
> Which is quite reasonable for Fernando's patch -- the direct call fsync 
> case.
> 
> But that leaves open the possibility that some people really do want the 
> EOPNOTSUPP return value, I guess?  Do existing callers need that?
> 

As far as I know, reiserfs is the only one actively using it to choose
different code.  It moves a single wait_on_buffer when barriers are on,
which I took out once to simplify the code.  Ric saw it in some
benchmark numbers and I put it back in.

Given that it was a long time ago, I don't have a problem with changing
it to work like all the other filesystems.

-chris


--
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/