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@xxxxxx>
> >
> > 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@xxxxxx>
> > Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx
>
