Re: [PATCH v2 1/6] xfs: cache last bitmap block in realtime allocator

["Darrick J. Wong" <djwong@kernel.org>]

On Fri, Sep 15, 2023 at 07:48:24AM +1000, Dave Chinner wrote:
> On Fri, Sep 08, 2023 at 08:28:27AM -0700, Darrick J. Wong wrote:
> > On Fri, Sep 08, 2023 at 01:43:15PM +1000, Dave Chinner wrote:
> > > On Tue, Sep 05, 2023 at 02:51:52PM -0700, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > > 
> > > > Profiling a workload on a highly fragmented realtime device showed a ton
> > > > of CPU cycles being spent in xfs_trans_read_buf() called by
> > > > xfs_rtbuf_get(). Further tracing showed that much of that was repeated
> > > > calls to xfs_rtbuf_get() for the same block of the realtime bitmap.
> > > > These come from xfs_rtallocate_extent_block(): as it walks through
> > > > ranges of free bits in the bitmap, each call to xfs_rtcheck_range() and
> > > > xfs_rtfind_{forw,back}() gets the same bitmap block. If the bitmap block
> > > > is very fragmented, then this is _a lot_ of buffer lookups.
> > > > 
> > > > The realtime allocator already passes around a cache of the last used
> > > > realtime summary block to avoid repeated reads (the parameters rbpp and
> > > > rsb). We can do the same for the realtime bitmap.
> > > > 
> > > > This replaces rbpp and rsb with a struct xfs_rtbuf_cache, which caches
> > > > the most recently used block for both the realtime bitmap and summary.
> > > > xfs_rtbuf_get() now handles the caching instead of the callers, which
> > > > requires plumbing xfs_rtbuf_cache to more functions but also makes sure
> > > > we don't miss anything.
> > > > 
> > > > Signed-off-by: Omar Sandoval <osandov@fb.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_rtbitmap.c | 179 ++++++++++++++++++-----------------
> > > >  fs/xfs/xfs_rtalloc.c         | 119 +++++++++++------------
> > > >  fs/xfs/xfs_rtalloc.h         |  30 ++++--
> > > >  3 files changed, 170 insertions(+), 158 deletions(-)
> > > 
> > > ....
> > > 
> > > > diff --git a/fs/xfs/xfs_rtalloc.h b/fs/xfs/xfs_rtalloc.h
> > > > index 62c7ad79cbb6..72f4261bb101 100644
> > > > --- a/fs/xfs/xfs_rtalloc.h
> > > > +++ b/fs/xfs/xfs_rtalloc.h
> > > > @@ -101,29 +101,40 @@ xfs_growfs_rt(
> > > >  /*
> > > >   * From xfs_rtbitmap.c
> > > >   */
> > > > +struct xfs_rtbuf_cache {
> > > > +	struct xfs_buf *bbuf;	/* bitmap block buffer */
> > > > +	xfs_fileoff_t bblock;	/* bitmap block number */
> > > > +	struct xfs_buf *sbuf;	/* summary block buffer */
> > > > +	xfs_fileoff_t sblock;	/* summary block number */
> > > 
> > > I don't think the block numbers are file offsets? Most of the code
> > > passes the bitmap and summary block numbers around as a
> > > xfs_fsblock_t...
> > 
> > They're fed into xfs_bmapi_read as the xfs_fileoff_t parameter.
> > 
> > I have a whole series cleaning up all of the units abuse in the rt code
> > part of the realtime modernization patchdeluge:
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=clean-up-realtime-units
> > 
> > Here's the specific patch that cleans up this one part:
> > https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=realtime-groups&id=b344bd4bd655576f8bda193b0e33f471a6295b05
> ....
> 
> > Could someone (Omar?) take a look at my other rt cleanups too?  I'd very
> > much like to get them merged out of my dev tree.
> 
> Can you post the series to the list? That makes it much easier to
> comment on them....

https://lore.kernel.org/linux-xfs/167243865605.709511.15650588946095003543.stgit@magnolia/

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com