On Tue, Aug 27, 2024 at 11:56:31AM +1000, Dave Chinner wrote:
> On Mon, Aug 26, 2024 at 12:40:28PM -0700, Darrick J. Wong wrote:
> > 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.
>
> Hmmm. Could we initialise it in memory only for !rtg filesystems,
> and make sure we never write it back via a check in the
> xfs_sb_to_disk() formatter function?
Only if the incore sb_rgextents becomes u64, which will then cause the
incore and ondisk superblock structures not to match anymore. There's
probably not much reason to keep them the same anymore. That said, up
until recently the metadir patchset actually broke the two apart, but
then hch and I put things back to reduce our own confusion.
> That would remove one of the problematic in-memory differences
> between old skool rtdev setups and the new rtg-based setups...
>
> > > > @@ -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?
>
> Yeah, I was thinking that the iterator only needs the bno_hint
> to determine which group to start iterating. Everything else is
> derived from information in the ap structure and so doesn't need to
> be calculated above the iterator.
>
> Though we could just lift the xfs_rtalloc_args() up to this level
> and stuff all the parameters into that structure and pass it down
> instead (like we do with xfs_alloc_args for the btree allocator).
> Then we only need to pass args through xfs_rtallocate(),
> xfs_rtallocate_extent_near/size() and all the other helper
> functions, too.
>
> That's a much bigger sort of cleanup, though, but I think it would
> be worth doing a some point because it would bring the rtalloc code
> closer to how the btalloc code is structured. And, perhaps, allow us
> to potentially share group selection and iteration code between
> the bt and rt allocators in future...
Well we're already tearing the rt allocator to pieces and rebuilding it,
so why not...
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
