| Message ID | 20210810052451.41578-4-david@fromorbit.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: rework feature flags | expand |
On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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. > > Hence you can mount with noattr2, crash shortly afterwards, and > mount again without attr2 or noattr2 and still have attr2 enabled > because the second mount sees attr2 still enabled in the superblock > before recovery runs and removes the feature bit. It's just a mess. > > Further, this is all legacy code as the v5 format requires attr2 to > be enabled at all times and it cannot be disabled. i.e. the noattr2 > mount option returns an error when used on v5 format filesystems. > > To straighten this all out, this patch reverts the attr2/noattr2 > mount option behaviour back to the original behaviour. There is no > reason for disabling attr2 these days, so we will only do this when > the noattr2 mount option is set. This will not remove the superblock > feature bit. The superblock bit will provide the default behaviour > and only track whether attr2 is present on disk or not. The attr2 > mount option will enable the creation of attr2 format inode forks, > and if the superblock feature bit is not set it will be added when > the first attr2 inode fork is created. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > Reviewed-by: Darrick J. Wong <djwong@kernel.org> I see the following regression on xfs/187 with this patch applied: --- /tmp/fstests/tests/xfs/187.out 2021-05-13 11:47:55.849859833 -0700 +++ /var/tmp/fstests/xfs/187.out.bad 2021-08-11 15:59:15.692618610 -0700 @@ -9,6 +9,8 @@ noattr2 fs +MOREBITS +ATTR2 *** 2. test attr2 mkfs and then noattr2 mount with 1 EA *** @@ -23,6 +25,8 @@ user.test ATTR +MOREBITS +ATTR2 *** 3. test noattr2 mount and lazy sb *** @@ -36,4 +40,5 @@ noattr2 fs MOREBITS +ATTR2 LAZYSBCOUNT I am pretty sure this is a direct result of "This will not remove the superblock feature bit", correct? Do you have an adjustment to xfs/187 to avoid regressing QA? --D > --- > fs/xfs/libxfs/xfs_format.h | 7 ------- > fs/xfs/xfs_mount.c | 27 ++++++++++----------------- > fs/xfs/xfs_super.c | 16 +++++++--------- > 3 files changed, 17 insertions(+), 33 deletions(-) > > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h > index 5d8a129150d5..ac739e6a921e 100644 > --- a/fs/xfs/libxfs/xfs_format.h > +++ b/fs/xfs/libxfs/xfs_format.h > @@ -398,13 +398,6 @@ static inline void xfs_sb_version_addattr2(struct xfs_sb *sbp) > sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT; > } > > -static inline void xfs_sb_version_removeattr2(struct xfs_sb *sbp) > -{ > - sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT; > - if (!sbp->sb_features2) > - sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT; > -} > - > static inline bool xfs_sb_version_hasprojid32bit(struct xfs_sb *sbp) > { > return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) || > 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 > @@ -612,25 +612,8 @@ xfs_mountfs( > xfs_warn(mp, "correcting sb_features alignment problem"); > sbp->sb_features2 |= sbp->sb_bad_features2; > mp->m_update_sb = true; > - > - /* > - * Re-check for ATTR2 in case it was found in bad_features2 > - * slot. > - */ > - if (xfs_sb_version_hasattr2(&mp->m_sb) && > - !(mp->m_flags & XFS_MOUNT_NOATTR2)) > - mp->m_flags |= XFS_MOUNT_ATTR2; > } > > - if (xfs_sb_version_hasattr2(&mp->m_sb) && > - (mp->m_flags & XFS_MOUNT_NOATTR2)) { > - xfs_sb_version_removeattr2(&mp->m_sb); > - mp->m_update_sb = true; > - > - /* update sb_versionnum for the clearing of the morebits */ > - if (!sbp->sb_features2) > - mp->m_update_sb = true; > - } > > /* always use v2 inodes by default now */ > if (!(mp->m_sb.sb_versionnum & XFS_SB_VERSION_NLINKBIT)) { > @@ -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; > + > /* > * Log's mount-time initialization. The first part of recovery can place > * some items on the AIL, to be handled when recovery is finished or > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 53ce25008948..6ab985ee6ba2 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -968,14 +968,6 @@ xfs_finish_flags( > return -EINVAL; > } > > - /* > - * mkfs'ed attr2 will turn on attr2 mount unless explicitly > - * told by noattr2 to turn it off > - */ > - if (xfs_sb_version_hasattr2(&mp->m_sb) && > - !(mp->m_flags & XFS_MOUNT_NOATTR2)) > - mp->m_flags |= XFS_MOUNT_ATTR2; > - > /* > * prohibit r/w mounts of read-only filesystems > */ > @@ -1338,7 +1330,6 @@ xfs_fs_parse_param( > return 0; > case Opt_noattr2: > xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_NOATTR2, true); > - parsing_mp->m_flags &= ~XFS_MOUNT_ATTR2; > parsing_mp->m_flags |= XFS_MOUNT_NOATTR2; > return 0; > default: > @@ -1362,6 +1353,13 @@ xfs_fs_validate_params( > return -EINVAL; > } > > + if ((mp->m_flags & (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) == > + (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) { > + xfs_warn(mp, "attr2 and noattr2 cannot both be specified."); > + return -EINVAL; > + } > + > + > if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > (mp->m_dalign || mp->m_swidth)) { > xfs_warn(mp, > -- > 2.31.1 >
On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > 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? --D > + > /* > * Log's mount-time initialization. The first part of recovery can place > * some items on the AIL, to be handled when recovery is finished or > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 53ce25008948..6ab985ee6ba2 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -968,14 +968,6 @@ xfs_finish_flags( > return -EINVAL; > } > > - /* > - * mkfs'ed attr2 will turn on attr2 mount unless explicitly > - * told by noattr2 to turn it off > - */ > - if (xfs_sb_version_hasattr2(&mp->m_sb) && > - !(mp->m_flags & XFS_MOUNT_NOATTR2)) > - mp->m_flags |= XFS_MOUNT_ATTR2; > - > /* > * prohibit r/w mounts of read-only filesystems > */ > @@ -1338,7 +1330,6 @@ xfs_fs_parse_param( > return 0; > case Opt_noattr2: > xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_NOATTR2, true); > - parsing_mp->m_flags &= ~XFS_MOUNT_ATTR2; > parsing_mp->m_flags |= XFS_MOUNT_NOATTR2; > return 0; > default: > @@ -1362,6 +1353,13 @@ xfs_fs_validate_params( > return -EINVAL; > } > > + if ((mp->m_flags & (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) == > + (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) { > + xfs_warn(mp, "attr2 and noattr2 cannot both be specified."); > + return -EINVAL; > + } > + > + > if ((mp->m_flags & XFS_MOUNT_NOALIGN) && > (mp->m_dalign || mp->m_swidth)) { > xfs_warn(mp, > -- > 2.31.1 >
On Wed, Aug 11, 2021 at 04:04:05PM -0700, Darrick J. Wong wrote: > On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote: > > From: Dave Chinner <dchinner@redhat.com> > > > > 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. ...... > > I see the following regression on xfs/187 with this patch applied: > > --- /tmp/fstests/tests/xfs/187.out 2021-05-13 11:47:55.849859833 -0700 > +++ /var/tmp/fstests/xfs/187.out.bad 2021-08-11 15:59:15.692618610 -0700 > @@ -9,6 +9,8 @@ > > noattr2 fs > > +MOREBITS > +ATTR2 > > *** 2. test attr2 mkfs and then noattr2 mount with 1 EA *** > > @@ -23,6 +25,8 @@ > user.test > > ATTR > +MOREBITS > +ATTR2 > > *** 3. test noattr2 mount and lazy sb *** > > @@ -36,4 +40,5 @@ > noattr2 fs > > MOREBITS > +ATTR2 > LAZYSBCOUNT > > I am pretty sure this is a direct result of "This will not remove the > superblock feature bit", correct? Do you have an adjustment to xfs/187 > to avoid regressing QA? I've been looking at 187, and the tests that are failing are testing the specific behaviour of "mkfs w/attr2 enabled; mount noattr2 and ensure the attr2 version bits are not set". Basically, the premise of the noattr2 verification tests are not valid anymore, but adding filters to mask out the addition of MOREBITS and ATTR2 basically renders the test useless for validating the old behaviour is still correct. IOWs, there's no way to detect if a kernel is going to behave like the old code and remove the bits, or whether it is going to just leave the attr2 fields set on disk. Hence I've got no idea how we'd change xfs/187 to validate both the old and new behaviour because we have no way of knowing which behaviour to expect. So I'd vote for just throwing away xfs/187 because a) it looks unfixable to me, and b) it's trying to validate what should be considered broken behaviour (i.e. on-disk feature bit gets removed even though the feature is still present on disk)... Your thoughts? Cheers, Dave.
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@redhat.com> > > > > 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.
diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h index 5d8a129150d5..ac739e6a921e 100644 --- a/fs/xfs/libxfs/xfs_format.h +++ b/fs/xfs/libxfs/xfs_format.h @@ -398,13 +398,6 @@ static inline void xfs_sb_version_addattr2(struct xfs_sb *sbp) sbp->sb_features2 |= XFS_SB_VERSION2_ATTR2BIT; } -static inline void xfs_sb_version_removeattr2(struct xfs_sb *sbp) -{ - sbp->sb_features2 &= ~XFS_SB_VERSION2_ATTR2BIT; - if (!sbp->sb_features2) - sbp->sb_versionnum &= ~XFS_SB_VERSION_MOREBITSBIT; -} - static inline bool xfs_sb_version_hasprojid32bit(struct xfs_sb *sbp) { return (XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5) || 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 @@ -612,25 +612,8 @@ xfs_mountfs( xfs_warn(mp, "correcting sb_features alignment problem"); sbp->sb_features2 |= sbp->sb_bad_features2; mp->m_update_sb = true; - - /* - * Re-check for ATTR2 in case it was found in bad_features2 - * slot. - */ - if (xfs_sb_version_hasattr2(&mp->m_sb) && - !(mp->m_flags & XFS_MOUNT_NOATTR2)) - mp->m_flags |= XFS_MOUNT_ATTR2; } - if (xfs_sb_version_hasattr2(&mp->m_sb) && - (mp->m_flags & XFS_MOUNT_NOATTR2)) { - xfs_sb_version_removeattr2(&mp->m_sb); - mp->m_update_sb = true; - - /* update sb_versionnum for the clearing of the morebits */ - if (!sbp->sb_features2) - mp->m_update_sb = true; - } /* always use v2 inodes by default now */ if (!(mp->m_sb.sb_versionnum & XFS_SB_VERSION_NLINKBIT)) { @@ -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; + /* * Log's mount-time initialization. The first part of recovery can place * some items on the AIL, to be handled when recovery is finished or diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 53ce25008948..6ab985ee6ba2 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -968,14 +968,6 @@ xfs_finish_flags( return -EINVAL; } - /* - * mkfs'ed attr2 will turn on attr2 mount unless explicitly - * told by noattr2 to turn it off - */ - if (xfs_sb_version_hasattr2(&mp->m_sb) && - !(mp->m_flags & XFS_MOUNT_NOATTR2)) - mp->m_flags |= XFS_MOUNT_ATTR2; - /* * prohibit r/w mounts of read-only filesystems */ @@ -1338,7 +1330,6 @@ xfs_fs_parse_param( return 0; case Opt_noattr2: xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_NOATTR2, true); - parsing_mp->m_flags &= ~XFS_MOUNT_ATTR2; parsing_mp->m_flags |= XFS_MOUNT_NOATTR2; return 0; default: @@ -1362,6 +1353,13 @@ xfs_fs_validate_params( return -EINVAL; } + if ((mp->m_flags & (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) == + (XFS_MOUNT_ATTR2|XFS_MOUNT_NOATTR2)) { + xfs_warn(mp, "attr2 and noattr2 cannot both be specified."); + return -EINVAL; + } + + if ((mp->m_flags & XFS_MOUNT_NOALIGN) && (mp->m_dalign || mp->m_swidth)) { xfs_warn(mp,