On Wed, Aug 11, 2021 at 05:27:27PM -0700, Darrick J. Wong wrote:
> On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > The attr2 feature is somewhat unique in that it has both a superblock
> > feature bit to enable it and mount options to enable and disable it.
> >
> > Back when it was first introduced in 2005, attr2 was disabled unless
> > either the attr2 superblock feature bit was set, or the attr2 mount
> > option was set. If the superblock feature bit was not set but the
> > mount option was set, then when the first attr2 format inode fork
> > was created, it would set the superblock feature bit. This is as it
> > should be - the superblock feature bit indicated the presence of the
> > attr2 on disk format.
> >
> > The noattr2 mount option, however, did not affect the superblock
> > feature bit. If noattr2 was specified, the on-disk superblock
> > feature bit was ignored and the code always just created attr1
> > format inode forks. If neither of the attr2 or noattr2 mounts
> > option were specified, then the behaviour was determined by the
> > superblock feature bit.
> >
> > This was all pretty sane.
> >
> > Fast foward 3 years, and we are dealing with fallout from the
> > botched sb_features2 addition and having to deal with feature
> > mismatches between the sb_features2 and sb_bad_features2 fields. The
> > attr2 feature bit was one of these flags. The reconciliation was
> > done well after mount option parsing and, unfortunately, the feature
> > reconciliation had a bug where it ignored the noattr2 mount option.
> >
> > For reasons lost to the mists of time, it was decided that resolving
> > this issue in commit 7c12f296500e ("[XFS] Fix up noattr2 so that it
> > will properly update the versionnum and features2 fields.") required
> > noattr2 to clear the superblock attr2 feature bit. This greatly
> > complicated the attr2 behaviour and broke rules about feature bits
> > needing to be set when those specific features are present in the
> > filesystem.
> >
> > By complicated, I mean that it introduced problems due to feature
> > bit interactions with log recovery. All of the superblock feature
> > bit checks are done prior to log recovery, but if we crash after
> > removing a feature bit, then on the next mount we see the feature
> > bit in the unrecovered superblock, only to have it go away after the
> > log has been replayed. This means our mount time feature processing
> > could be all wrong.
>
> Speaking of log recovery...
>
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 74349eab5b58..f2b3a7932f3b 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -773,6 +756,16 @@ xfs_mountfs(
> > if (error)
> > goto out_fail_wait;
> >
> > + /*
> > + * Now that we've recovered any pending superblock feature bit
> > + * additions, we can finish setting up the attr2 behaviour for the
> > + * mount. If no attr2 mount options were specified, the we use the
> > + * behaviour specified by the superblock feature bit.
> > + */
> > + if (!(mp->m_flags & (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) &&
> > + xfs_sb_version_hasattr2(&mp->m_sb))
> > + mp->m_flags |= XFS_MOUNT_ATTR2;
>
> ...shouldn't this come /after/ the call to xfs_log_mount?
Hmmm - that smeels like a mis-merge on top of the inodegc
modifications.
/me goes and looks at v1
Yup, rebase dumped it before log recovery when the original v1 patch
had it correctly placed after the xfs_log_mount() call.
Good catch, fixed.
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
