Re: [PATCH] avoid too many boundaries in DIO

[Andrew Morton <akpm@osdl.org>]

On Thu, 9 Nov 2006 20:48:54 -0500
Chris Mason <chris.mason@oracle.com> wrote:

> Dave Chinner found a 10% performance regression with ext3 when using DIO
> to fill holes instead of buffered IO.  On large IOs, the ext3 get_block
> routine will send more than a page worth of blocks back to DIO via a
> single buffer_head with a large b_size value.
> 
> The DIO code iterates through this massive block and tests for a
> boundary buffer over and over again.  For every block size unit spanned
> by the big map_bh, the boundary bit is tested and a bio may be forced
> down to the block layer.
> 
> There are two potential fixes, one is to ignore the boundary bit on
> large regions returned by the FS.  DIO can't tell which part of the big
> region was a boundary, and so it may not be a good idea to trust the
> hint.
> 
> This patch just clears the boundary bit after using it once.  It is 10%
> faster for a streaming DIO write w/blocksize of 512k on my sata drive.
> 

Thanks.

But that's two large performance regressions (so far) from the multi-block
get_block() feature.  And that was allegedly a performance optimisation! 
Who's testing this stuff?

> 
> diff -r 38d08cbe880b fs/direct-io.c
> --- a/fs/direct-io.c	Thu Nov 09 20:02:08 2006 -0500
> +++ b/fs/direct-io.c	Thu Nov 09 20:31:12 2006 -0500
> @@ -959,6 +959,17 @@ do_holes:
>  			BUG_ON(this_chunk_bytes == 0);
>  
>  			dio->boundary = buffer_boundary(map_bh);
> +
> +			/*
> +			 * get_block may return more than one page worth
> +			 * of blocks.  Only make the first one a boundary.
> +			 * This is still sub-optimal, it probably only
> +			 * makes sense to play with boundaries when
> +			 * get_block returns a single FS block sized
> +			 * unit.
> +			 */
> +			clear_buffer_boundary(map_bh);
> +
>  			ret = submit_page_section(dio, page, offset_in_page,
>  				this_chunk_bytes, dio->next_block_for_io);
>  			if (ret) {


Is that actually correct?  If ->get_block() returned a buffer_boundary()
buffer then what we want to do is to push down all the thus-far-queued BIOs
once we've submitted _all_ of the BIOs represented by map_bh.  I think that
if we require more than one BIO to cover map_bh.b_size then we'll do the
submission after the first BIO has been sent instead of after the final one
has been sent?