On Thu, Oct 10, 2024 at 05:49:25PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@xxxxxxxxxx>
>
> The metadir feature is a good opportunity to break from the past where
> we had to do this strange dance with sb_bad_features2 due to a past bug
> where the superblock was not kept aligned to a multiple of 8 bytes.
>
> Therefore, stop this pretense when metadir is enabled. We'll just set
> the incore and ondisk fields to zero, thereby freeing up 4 bytes of
> space in the superblock. We'll reuse those 4 bytes later.
>
> Signed-off-by: Darrick J. Wong <djwong@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_format.h | 73 ++++++++++++++++++++++++++++----------------
> fs/xfs/libxfs/xfs_sb.c | 27 +++++++++++++---
> fs/xfs/scrub/agheader.c | 9 ++++-
> 3 files changed, 75 insertions(+), 34 deletions(-)
This is all pretty nasty. We are not short on space in the
superblock, so I'm not sure why we want to add all this complexity
just to save 4 bytes of space in the sueprblock.
In reality, the V5 superblock version has never had the
bad-features2 problem at all - it was something that happened and
was fixed long before V5 came along. Hence, IMO .....
> @@ -437,6 +446,18 @@ static inline bool xfs_sb_version_hasmetadir(const struct xfs_sb *sbp)
> (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_METADIR);
> }
>
> +/*
> + * Detect a mismatched features2 field. Older kernels read/wrote
> + * this into the wrong slot, so to be safe we keep them in sync.
> + * Newer metadir filesystems have never had this bug, so the field is always
> + * zero.
> + */
> +static inline bool xfs_sb_has_mismatched_features2(const struct xfs_sb *sbp)
> +{
> + return !xfs_sb_version_hasmetadir(sbp) &&
> + sbp->sb_bad_features2 != sbp->sb_features2;
> +}
This could be:
static inline bool xfs_sb_has_mismatched_features2(const struct xfs_sb *sbp)
{
return !xfs_sb_is_v5(sbp) &&
sbp->sb_bad_features2 != sbp->sb_features2;
}
and nobody should notice anything changes. If we then check that
sbp->sb_bad_features2 == sbp->sb_features2 in the v5 section of
xfs_validate_sb_common(), all we'll catch is syzbot trying to do
stupid things to the feature fields on v5 filesystems....
Then we can add whatever new fields metadir needs at the end of
the superblock. That seems much cleaer than adding this
conditional handling of the field in when reading, writing and
verifying the superblock and when mounting the filesystem....
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
