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

From: Dave Chinner <david@fromorbit.com>
To: "Darrick J. Wong" <djwong@kernel.org>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 04/16] xfs: reflect sb features in xfs_mount
Date: Thu, 15 Jul 2021 09:07:31 +1000	[thread overview]
Message-ID: <20210714230731.GB664593@dread.disaster.area> (raw)
In-Reply-To: <20210714225603.GW22402@magnolia>

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....

> >  	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.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com