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

[Dave Chinner]

On Thu, Feb 20, 2014 at 01:06:02PM -0600, Eric Sandeen wrote:
> On 2/19/14, 11:55 PM, Dave Chinner wrote:
> > From: Dave Chinner <dchinner at 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?

Because we have application code that is building buffer maps from
extent maps, and so having a API for both
contigous and discontiguous buffers makes sense. That's the way we
do it in the kernel - everything now uses the _map paths - but the
userspace code is a much larger surface area to change over and so
it's only being done in the places that matter first...

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

Right. And like I said, it's also how the kernel does stuff, which
is why the libxfs_readbuf_map code functions like it does. ;)

Cheers,

Dave.
-- 
Dave Chinner
david at fromorbit.com