Re: [PATCH] avoid too many boundaries in DIO

From: Andrew Morton
Date: Fri Nov 10 2006 - 01:56:42 EST


On Thu, 9 Nov 2006 20:48:54 -0500
Chris Mason <chris.mason@xxxxxxxxxx> 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?

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