Re: [PATCH 21/26] xfs: make the RT allocator rtgroup aware

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

On Mon, Aug 26, 2024 at 02:56:37PM +1000, Dave Chinner wrote:
> On Thu, Aug 22, 2024 at 05:26:38PM -0700, Darrick J. Wong wrote:
> > From: Christoph Hellwig <hch@lst.de>
> > 
> > Make the allocator rtgroup aware by either picking a specific group if
> > there is a hint, or loop over all groups otherwise.  A simple rotor is
> > provided to pick the placement for initial allocations.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_bmap.c     |   13 +++++-
> >  fs/xfs/libxfs/xfs_rtbitmap.c |    6 ++-
> >  fs/xfs/xfs_mount.h           |    1 
> >  fs/xfs/xfs_rtalloc.c         |   98 ++++++++++++++++++++++++++++++++++++++----
> >  4 files changed, 105 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 126a0d253654a..88c62e1158ac7 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -3151,8 +3151,17 @@ xfs_bmap_adjacent_valid(
> >  	struct xfs_mount	*mp = ap->ip->i_mount;
> >  
> >  	if (XFS_IS_REALTIME_INODE(ap->ip) &&
> > -	    (ap->datatype & XFS_ALLOC_USERDATA))
> > -		return x < mp->m_sb.sb_rblocks;
> > +	    (ap->datatype & XFS_ALLOC_USERDATA)) {
> > +		if (x >= mp->m_sb.sb_rblocks)
> > +			return false;
> > +		if (!xfs_has_rtgroups(mp))
> > +			return true;
> > +
> > +		return xfs_rtb_to_rgno(mp, x) == xfs_rtb_to_rgno(mp, y) &&
> > +			xfs_rtb_to_rgno(mp, x) < mp->m_sb.sb_rgcount &&
> > +			xfs_rtb_to_rtx(mp, x) < mp->m_sb.sb_rgextents;
> 
> WHy do we need the xfs_has_rtgroups() check here? The new rtg logic will
> return true for an old school rt device here, right?

The incore sb_rgextents is zero on !rtg filesystems, so we need the
xfs_has_rtgroups.

> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 3fedc552b51b0..2b57ff2687bf6 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -1661,8 +1661,9 @@ xfs_rtalloc_align_minmax(
> >  }
> >  
> >  static int
> > -xfs_rtallocate(
> > +xfs_rtallocate_rtg(
> >  	struct xfs_trans	*tp,
> > +	xfs_rgnumber_t		rgno,
> >  	xfs_rtblock_t		bno_hint,
> >  	xfs_rtxlen_t		minlen,
> >  	xfs_rtxlen_t		maxlen,
> > @@ -1682,16 +1683,33 @@ xfs_rtallocate(
> >  	xfs_rtxlen_t		len = 0;
> >  	int			error = 0;
> >  
> > -	args.rtg = xfs_rtgroup_grab(args.mp, 0);
> > +	args.rtg = xfs_rtgroup_grab(args.mp, rgno);
> >  	if (!args.rtg)
> >  		return -ENOSPC;
> >  
> >  	/*
> > -	 * Lock out modifications to both the RT bitmap and summary inodes.
> > +	 * We need to lock out modifications to both the RT bitmap and summary
> > +	 * inodes for finding free space in xfs_rtallocate_extent_{near,size}
> > +	 * and join the bitmap and summary inodes for the actual allocation
> > +	 * down in xfs_rtallocate_range.
> > +	 *
> > +	 * For RTG-enabled file system we don't want to join the inodes to the
> > +	 * transaction until we are committed to allocate to allocate from this
> > +	 * RTG so that only one inode of each type is locked at a time.
> > +	 *
> > +	 * But for pre-RTG file systems we need to already to join the bitmap
> > +	 * inode to the transaction for xfs_rtpick_extent, which bumps the
> > +	 * sequence number in it, so we'll have to join the inode to the
> > +	 * transaction early here.
> > +	 *
> > +	 * This is all a bit messy, but at least the mess is contained in
> > +	 * this function.
> >  	 */
> >  	if (!*rtlocked) {
> >  		xfs_rtgroup_lock(args.rtg, XFS_RTGLOCK_BITMAP);
> > -		xfs_rtgroup_trans_join(tp, args.rtg, XFS_RTGLOCK_BITMAP);
> > +		if (!xfs_has_rtgroups(args.mp))
> > +			xfs_rtgroup_trans_join(tp, args.rtg,
> > +					XFS_RTGLOCK_BITMAP);
> >  		*rtlocked = true;
> >  	}
> >  
> > @@ -1701,7 +1719,7 @@ xfs_rtallocate(
> >  	 */
> >  	if (bno_hint)
> >  		start = xfs_rtb_to_rtx(args.mp, bno_hint);
> > -	else if (initial_user_data)
> > +	else if (!xfs_has_rtgroups(args.mp) && initial_user_data)
> >  		start = xfs_rtpick_extent(args.rtg, tp, maxlen);
> 
> Check initial_user_data first - we don't care if there are rtgroups
> enabled if initial_user_data is not true, and we only ever allocate
> initial data on an inode once...

<nod>

> > @@ -1741,6 +1767,53 @@ xfs_rtallocate(
> >  	return error;
> >  }
> >  
> > +static int
> > +xfs_rtallocate_rtgs(
> > +	struct xfs_trans	*tp,
> > +	xfs_fsblock_t		bno_hint,
> > +	xfs_rtxlen_t		minlen,
> > +	xfs_rtxlen_t		maxlen,
> > +	xfs_rtxlen_t		prod,
> > +	bool			wasdel,
> > +	bool			initial_user_data,
> > +	xfs_rtblock_t		*bno,
> > +	xfs_extlen_t		*blen)
> > +{
> > +	struct xfs_mount	*mp = tp->t_mountp;
> > +	xfs_rgnumber_t		start_rgno, rgno;
> > +	int			error;
> > +
> > +	/*
> > +	 * For now this just blindly iterates over the RTGs for an initial
> > +	 * allocation.  We could try to keep an in-memory rtg_longest member
> > +	 * to avoid the locking when just looking for big enough free space,
> > +	 * but for now this keep things simple.
> > +	 */
> > +	if (bno_hint != NULLFSBLOCK)
> > +		start_rgno = xfs_rtb_to_rgno(mp, bno_hint);
> > +	else
> > +		start_rgno = (atomic_inc_return(&mp->m_rtgrotor) - 1) %
> > +				mp->m_sb.sb_rgcount;
> > +
> > +	rgno = start_rgno;
> > +	do {
> > +		bool		rtlocked = false;
> > +
> > +		error = xfs_rtallocate_rtg(tp, rgno, bno_hint, minlen, maxlen,
> > +				prod, wasdel, initial_user_data, &rtlocked,
> > +				bno, blen);
> > +		if (error != -ENOSPC)
> > +			return error;
> > +		ASSERT(!rtlocked);
> > +
> > +		if (++rgno == mp->m_sb.sb_rgcount)
> > +			rgno = 0;
> > +		bno_hint = NULLFSBLOCK;
> > +	} while (rgno != start_rgno);
> > +
> > +	return -ENOSPC;
> > +}
> > +
> >  static int
> >  xfs_rtallocate_align(
> >  	struct xfs_bmalloca	*ap,
> > @@ -1835,9 +1908,16 @@ xfs_bmap_rtalloc(
> >  	if (xfs_bmap_adjacent(ap))
> >  		bno_hint = ap->blkno;
> >  
> > -	error = xfs_rtallocate(ap->tp, bno_hint, raminlen, ralen, prod,
> > -			ap->wasdel, initial_user_data, &rtlocked,
> > -			&ap->blkno, &ap->length);
> > +	if (xfs_has_rtgroups(ap->ip->i_mount)) {
> > +		error = xfs_rtallocate_rtgs(ap->tp, bno_hint, raminlen, ralen,
> > +				prod, ap->wasdel, initial_user_data,
> > +				&ap->blkno, &ap->length);
> > +	} else {
> > +		error = xfs_rtallocate_rtg(ap->tp, 0, bno_hint, raminlen, ralen,
> > +				prod, ap->wasdel, initial_user_data,
> > +				&rtlocked, &ap->blkno, &ap->length);
> > +	}
> 
> The xfs_has_rtgroups() check is unnecessary.  The iterator in
> xfs_rtallocate_rtgs() will do the right thing for the
> !xfs_has_rtgroups() case - it'll set start_rgno = 0 and break out
> after a single call to xfs_rtallocate_rtg() with rgno = 0.
> 
> Another thing that probably should be done here is push all the
> constant value calculations a couple of functions down the stack to
> where they are used. Then we only need to pass two parameters down
> through the rg iterator here, not 11...

..and pass the ap itself too, to remove three of the parameters?

--D

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