Re: [PATCH v1] xfs: Fix rgcount/rgsize value reported in XFS_IOC_FSGEOMETRY ioctl

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

On Tue, Dec 16, 2025 at 01:20:19PM +0530, Nirjhar Roy (IBM) wrote:
> 
> On 12/9/25 21:29, Darrick J. Wong wrote:
> > On Tue, Dec 09, 2025 at 03:53:34PM +0530, Nirjhar Roy (IBM) wrote:
> > > On 12/9/25 12:20, Darrick J. Wong wrote:
> > > > On Tue, Dec 09, 2025 at 11:05:21AM +0530, Nirjhar Roy (IBM) wrote:
> > > > > On 12/8/25 23:10, Darrick J. Wong wrote:
> > > > > > On Mon, Dec 08, 2025 at 08:46:11PM +0530, Nirjhar Roy (IBM) wrote:
> > > > > > > With mkfs.xfs -m dir=0 i.e, with XFS_SB_FEAT_INCOMPAT_METADIR
> > > > > > > disabled, number of realtime groups should be reported as 1 and
> > > > > > > the size of it should be equal to total number of realtime
> > > > > > > extents since this the entire realtime filesystem has only 1
> > > > > > > realtime group.
> > > > > > No.  This (pre-metadir realtime having one group encompassing the entire
> > > > > > rt volume) is an implementation detail, not a property of the filesystem
> > > > > > geometry.
> > > > > > 
> > > > > > Or put another way: a metadir rt filesystem with one rtgroup that covers
> > > > > > the entire rt device is different from a pre-metadir rt filesystem.
> > > > > > xfs_info should present that distinction to userspace, particularly
> > > > > > since xfs_scrub cares about that difference.
> > > > > Okay, got it. A quick question:
> > > > > 
> > > > > A metadir rt filesystem will have 1 bitmap/summary file per rt AG, isn't it?
> > > > Per rtgroup, but yes.
> > > > 
> > > > > If yes, then shouldn't functions like xfs_rtx_to_rbmblock(mp,
> > > > > xfs_rtxnum_t        rtx) return offset of the corresponding bitmap file of
> > > > > the rt AG where rtx belongs?
> > > > xfs_rtx_to_rbmblock is an unfortunate function.  Given an extent number
> > > > within an rtgroup, it tells you the corresponding block number within
> > > > that rtgroup's bitmap file.  Yes, that's confusing because xfs_rtxnum_t
> > > > historically denotes an extent number anywhere on the rt volume.
> > > > 
> > > > IOWs, it *should* be an xfs_rgxnum_t (group extent number), which could
> > > So the current XFS code with metadir enabled, calls xfs_rtx_to_rbmblock() in
> > > such a way that the extent number passed to the function is relative to the
> > > AG and not an absolute extent number, am I right?
> > Correct.
> 
> Okay. A couple of questions regarding realtime fs growth?
> 
> Q1. Looking at the loop in "xfs_growfs_rt()"
> 
>     for (rgno = old_rgcount; rgno < new_rgcount; rgno++) {
>         xfs_rtbxlen_t    rextents = div_u64(in->newblocks, in->extsize);
> 
>         error = xfs_rtgroup_alloc(mp, rgno, new_rgcount, rextents);
>         if (error)
>             goto out_unlock;
> 
>         error = xfs_growfs_rtg(mp, rgno, in->newblocks, delta_rtb,
>                        in->extsize);
>         if (error) {
>             struct xfs_rtgroup    *rtg;
> 
>             rtg = xfs_rtgroup_grab(mp, rgno);
>             if (!WARN_ON_ONCE(!rtg)) {
>                 xfs_rtunmount_rtg(rtg);
>                 xfs_rtgroup_rele(rtg);
>                 xfs_rtgroup_free(mp, rgno);
>             }
>             break;
>         }
> 
>     }
> 
> So are we supporting partial growth success i.e, if we are trying to
> increase the size from 4 rtgs to 8 rtgs, and we fail (let's say) after 6th
> rtg, so the new realtime fs size will be 6 rtgs, right? The reason why I am
> asking is that I don't see the rollbacks of the previously allocated rtgs in
> the 2nd error path i.e, after error = xfs_growfs_rtg(mp, rgno,
> in->newblocks, delta_rtb, in->extsize);

Yes, I /think/ we're supposed to end up with a partially grown
filesystem if there's an error midway through... assuming the fs doesn't
go down as a result cancelling a dirty transaction, etc.

> Q2. In the function xfs_rtcopy_summary(), shouldn't the last line be "return
> error" instead of "return 0"? If yes, then I will send a patch fixing this.

That's definitely broken.

--D

> --NR
> 
> 
> > 
> > --D
> > 
> > > > be a u32 quantity EXCEPT there's a stupid corner case: pre-metadir rt
> > > > volumes being treated as if they have one huge group.
> > > > 
> > > > It's theoretically possible for the "single" rtgroup of a pre-metadir rt
> > > > volume to have more than 2^32 blocks.  You're unlikely to find one in
> > > > practice because (a) old kernels screw up some of the computations and
> > > > explode, and (b) lack of sharding means the performance is terrible.
> > > > 
> > > > However, we don't want to create copy-pasted twins of the functions so
> > > > we took a dumb shortcut and made xfs_rtx_to_rbmblock take xfs_rtxnum_t.
> > > > Were someone to make a Rust XFS, they really ought to define separate
> > > > types for each distinct geometry usage, and define From traits to go
> > > > from one to the other.  Then our typesafety nightmare will be over. ;)
> > > Okay, got it.
> > > > > Right now, looking at the definition of
> > > > > xfs_rtx_to_rbmblock() it looks like it calculates the offset as if there is
> > > > > only 1 global bitmap file?
> > > > Right.
> > > Okay, thank you so much for the explanation.
> > > 
> > > --NR
> > > 
> > > > --D
> > > > 
> > > > > --NR
> > > > > 
> > > > > > --D
> > > > > > 
> > > > > > > Signed-off-by: Nirjhar Roy (IBM) <nirjhar.roy.lists@gmail.com>
> > > > > > > ---
> > > > > > >     fs/xfs/libxfs/xfs_sb.c | 8 +++-----
> > > > > > >     1 file changed, 3 insertions(+), 5 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> > > > > > > index cdd16dd805d7..989553e7ec02 100644
> > > > > > > --- a/fs/xfs/libxfs/xfs_sb.c
> > > > > > > +++ b/fs/xfs/libxfs/xfs_sb.c
> > > > > > > @@ -875,7 +875,7 @@ __xfs_sb_from_disk(
> > > > > > >     	} else {
> > > > > > >     		to->sb_metadirino = NULLFSINO;
> > > > > > >     		to->sb_rgcount = 1;
> > > > > > > -		to->sb_rgextents = 0;
> > > > > > > +		to->sb_rgextents = to->sb_rextents;
> > > > > > >     	}
> > > > > > >     	if (to->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_ZONED) {
> > > > > > > @@ -1586,10 +1586,8 @@ xfs_fs_geometry(
> > > > > > >     	geo->version = XFS_FSOP_GEOM_VERSION_V5;
> > > > > > > -	if (xfs_has_rtgroups(mp)) {
> > > > > > > -		geo->rgcount = sbp->sb_rgcount;
> > > > > > > -		geo->rgextents = sbp->sb_rgextents;
> > > > > > > -	}
> > > > > > > +	geo->rgcount = sbp->sb_rgcount;
> > > > > > > +	geo->rgextents = sbp->sb_rgextents;
> > > > > > >     	if (xfs_has_zoned(mp)) {
> > > > > > >     		geo->rtstart = sbp->sb_rtstart;
> > > > > > >     		geo->rtreserved = sbp->sb_rtreserved;
> > > > > > > -- 
> > > > > > > 2.43.5
> > > > > > > 
> > > > > > > 
> > > > > -- 
> > > > > Nirjhar Roy
> > > > > Linux Kernel Developer
> > > > > IBM, Bangalore
> > > > > 
> > > -- 
> > > Nirjhar Roy
> > > Linux Kernel Developer
> > > IBM, Bangalore
> > > 
> > > 
> -- 
> Nirjhar Roy
> Linux Kernel Developer
> IBM, Bangalore
>