Re: [PATCH 04/16] xfs: reflect sb features in xfs_mount

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

On Thu, Jul 15, 2021 at 09:07:31AM +1000, Dave Chinner wrote:
> On Wed, Jul 14, 2021 at 03:56:03PM -0700, Darrick J. Wong wrote:
> > On Wed, Jul 14, 2021 at 02:19:00PM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > Currently on-disk feature checks require decoding the superblock
> > > fileds and so can be non-trivial. We have almost 400 hundred
> > > individual feature checks in the XFS code, so this is a significant
> > > amount of code. To reduce runtime check overhead, pre-process all
> > > the version flags into a features field in the xfs_mount at mount
> > > time so we can convert all the feature checks to a simple flag
> > > check.
> > > 
> > > There is also a need to convert the dynamic feature flags to update
> > > the m_features field. This is required for attr, attr2 and quota
> > > features. New xfs_mount based wrappers are added for this.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ....
> > > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > > index 5db3fb184fbe..488f472cedba 100644
> > > --- a/fs/xfs/xfs_log_recover.c
> > > +++ b/fs/xfs/xfs_log_recover.c
> > > @@ -3315,6 +3315,7 @@ xlog_do_recover(
> > >  	xfs_buf_relse(bp);
> > >  
> > >  	/* re-initialise in-core superblock and geometry structures */
> > > +	mp->m_features |= xfs_sb_version_to_features(sbp);
> > 
> > '|=' instead of '=' ?
> 
> Yes, that is intended.
> 
> > I would have expected assignment, but I guess the assumption here is
> > that log recovery can process a sb update that adds a feature?  And that
> > log recovery won't be turning off features?
> 
> Right, we can add but we should never remove on-disk feature bits at
> runtime. Getting rid of the noattr2 shenanigans dropped the only
> case where we remove feature bits at runtime. Also, see below....

<nod>

> > >  	xfs_reinit_percpu_counters(mp);
> > >  	error = xfs_initialize_perag(mp, sbp->sb_agcount, &mp->m_maxagi);
> > >  	if (error) {
> > > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > > index 6be2a1c5b0f4..0ec463d91cce 100644
> > > --- a/fs/xfs/xfs_mount.c
> > > +++ b/fs/xfs/xfs_mount.c
> > > @@ -225,6 +225,7 @@ xfs_readsb(
> > >  		goto reread;
> > >  	}
> > >  
> > > +	mp->m_features |= xfs_sb_version_to_features(sbp);
> > 
> > Also, can't this be a plain assignment?
> 
> Nope, because a couple of patches further down the series,
> mp->m_features will already contain all the mount features that have
> been set and we do not want to overwrite them.

Ah, yup, now I saw that.  In that case,

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

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