Re: [PATCH 1/2] libxfs: contiguous buffers are not discontigous

Eric Sandeen
2014-02-20T19:06:07Z

On 2/19/14, 11:55 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> When discontiguous directory buffer support was fixed in xfs_repair,
> (dd9093d xfs_repair: fix discontiguous directory block support)
> it changed to using libxfs_getbuf_map() to support mapping
> discontiguous blocks, and the prefetch code special cased such
> discontiguous buffers.
>
> The issue is that libxfs_getbuf_map() marks all buffers, even
> contiguous ones - as LIBXFS_B_DISCONTIG, and so the prefetch code
> was treating every buffer as discontiguous. This causes the prefetch
> code to completely bypass the large IO optimisations for dense areas
> of metadata. Because there was no obvious change in performance or
> IO patterns, this wasn't noticed during performance testing.
>
> However, this change mysteriously fixed a regression in xfs/033 in
> the v3.2.0-alpha release, and this change in behaviour was
> discovered as part of triaging why it "fixed" the regression.
> Anyway, restoring the large IO prefetch optimisation results
> a reapiron a 10 million inode filesystem dropping from 197s to 173s,
> and the peak IOPS rate in phase 3 dropping from 25,000 to roughly
> 2,000 by trading off a bandwidth increase of roughly 100% (i.e.
> 200MB/s to 400MB/s). Phase 4 saw similar changes in IO profile and
> speed increases.
>
> This, however, re-introduces the regression in xfs/033, which will
> now be fixed in a separate patch.

Thanks for finding this. I was getting close. ;)

It seems fine, although a little unexpected; why do we ever
create a map of 1? It feels a little odd to call getbuf_map
with only 1 item, and then short-circuit it. Should this
be something more obvious in the callers?

Wel, I guess it's pretty much consistent w/ the same behavior
in libxfs_readbuf_map()... *shrug*

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Reported-by: Eric Sandeen <esandeen@redhat.com>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
> libxfs/rdwr.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index ac7739f..78a9b37 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -590,6 +590,10 @@ libxfs_getbuf_map(struct xfs_buftarg *btp, struct xfs_buf_map *map,
> struct xfs_bufkey key = {0};
> int i;
>
> + if (nmaps == 1)
> + return libxfs_getbuf_flags(btp, map[0].bm_bn, map[0].bm_len,
> + flags);
> +
> key.buftarg = btp;
> key.blkno = map[0].bm_bn;
> for (i = 0; i < nmaps; i++) {
>