Re: [PATCH 05/16] xfs: replace xfs_sb_version checks with feature flag checks

[Dave Chinner <david@fromorbit.com>]

On Wed, Aug 11, 2021 at 03:13:29PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 10, 2021 at 03:24:40PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Convert the xfs_sb_version_hasfoo() to checks against
> > mp->m_features. Checks of the superblock itself during disk
> > operations (e.g. in the read/write verifiers and the to/from disk
> > formatters) are not converted - they operate purely on the
> > superblock state. Everything else should use the mount features.
> > 
> > Large parts of this conversion were done with sed with commands like
> > this:
> > 
> > for f in `git grep -l xfs_sb_version_has fs/xfs/*.c`; do
> > 	sed -i -e 's/xfs_sb_version_has\(.*\)(&\(.*\)->m_sb)/xfs_has_\1(\2)/' $f
> > done
> > 
> > With manual cleanups for things like "xfs_has_extflgbit" and other
> > little inconsistencies in naming.
> > 
> > The result is ia lot less typing to check features and an XFS binary
> > size reduced by a bit over 3kB:
> > 
> > $ size -t fs/xfs/built-in.a
> > 	text	   data	    bss	    dec	    hex	filenam
> > before	1130866  311352     484 1442702  16038e (TOTALS)
> > after	1127727  311352     484 1439563  15f74b (TOTALS)
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> 
> <snip>
> 
> > diff --git a/fs/xfs/xfs_rtalloc.c b/fs/xfs/xfs_rtalloc.c
> > index 699066fb9052..7361830163d7 100644
> > --- a/fs/xfs/xfs_rtalloc.c
> > +++ b/fs/xfs/xfs_rtalloc.c
> > @@ -951,8 +951,7 @@ xfs_growfs_rt(
> >  		return -EINVAL;
> >  
> >  	/* Unsupported realtime features. */
> > -	if (xfs_sb_version_hasrmapbt(&mp->m_sb) ||
> > -	    xfs_sb_version_hasreflink(&mp->m_sb))
> > +	if (xfs_has_rmapbt(mp) || xfs_has_reflink(mp))
> 
> A regression test that I developed to test the act of adding a realtime
> volume to an existing filesystem exposed the following failure:
> 
> --- /tmp/fstests/tests/xfs/779.out      2021-08-08 08:47:08.535033170 -0700
> +++ /var/tmp/fstests/xfs/779.out.bad    2021-08-11 14:54:18.389346401 -0700
> @@ -1,2 +1,4 @@
>  QA output created by 779
> +/opt/a is not a realtime file?
> +expected file extszhint 0, got 12288
>  Silence is golden
> 
> Since this test is not yet upstream, I will describe the sequence of
> events:
> 
> 1. Suppress SCRATCH_RTDEV when invoking _scratch_mkfs.  This creates a
>    filesystem with no realtime volume attached.
> 2. Mount the fs with SCRATCH_RTDEV in the mount options.  The rt volume
>    still has not been attached.
> 3. Set RTINHERIT (and EXTSZINHERIT) on the root directory.  Make sure
>    the extent size hint is not a multiple of the rt extent size.
> 4. Call xfs_growfs -r to add the rt volume into the filesystem.
> 5. Create a file.  Due to RTINHERIT, the new file should be a realtime
>    file.
> 6. Check that the file is actually a realtime file and does not have an
>    extent size hint.
> 
> The regression of course happens in step 6, because xfs_growfs_rt can
> add a realtime volume to an existing filesystem.  Prior to this patch,
> the "has realtime?" predicate always looked at the mp->m_sb.  Now that
> the feature state has been turned into a separate xfs_mount state, we
> must set the REALTIME m_feature flag explicitly.
> 
> The following patch solves the regression:

I've added a variant of this to the patch - it just sets
XFS_FEAT_REALTIME directly without the "xfs_add_realtime()" wrapper.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com