Re: [PATCH 03/16] xfs: rework attr2 feature and mount options
From: "Darrick J. Wong" <djwong@kernel.org>
Date: 2021-08-12 00:27:28
On Tue, Aug 10, 2021 at 03:24:38PM +1000, Dave Chinner wrote:
From: Dave Chinner <redacted>
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...
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
+ /* * Log's mount-time initialization. The first part of recovery can place * some items on the AIL, to be handled when recovery is finished ordiff --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