On Tue, Oct 15, 2024 at 03:27:08PM +1100, Dave Chinner wrote:
> 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....
No. TOT kernel/xfs_repair/xfs_scrub enforce sb_bad_features2 ==
sb_features2 for V5 filesystems. If a new xfs_db omits a
sb_bad_features2 rewrite on an pre-metadir V5 fs because
xfs_sb_has_mismatched_features2 returns false, then an old repair will
complain "superblock has a features2 mismatch, correcting".
> 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....
<shrug> I don't see the point in undefining sb_bad_features2 for metadir
and not reusing the space, but since you and hch both favor extending
the sb instead, I'll just do that and drop this patch.
--D
> -Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
