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@xxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> ....
> > > 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@xxxxxxxxxx>
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
