On Tue, Dec 01, 2020 at 11:18:12AM -0500, Brian Foster wrote:
> On Mon, Nov 30, 2020 at 07:37:31PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> >
> > Define an incompat feature flag to indicate that the filesystem needs to
> > be repaired. While libxfs will recognize this feature, the kernel will
> > refuse to mount if the feature flag is set, and only xfs_repair will be
> > able to clear the flag. The goal here is to force the admin to run
> > xfs_repair to completion after upgrading the filesystem, or if we
> > otherwise detect anomalies.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> > ---
>
> IIUC, we're using an incompat bit to intentionally ensure the filesystem
> cannot mount, even on kernels that predate this particular "needs
> repair" feature. The only difference is that an older kernel would
> complain about an unknown feature and return a different error code.
> Right?
>
> That seems reasonable, but out of curiousity is there a need/reason for
> using an incompat bit over an ro_compat bit?
The general principle is to prevent /any/ mounting of the fs until the
admin runs repair, even if it's readonly mounting. The specific reason
is so that xfs_db can set some other feature flag as part of an upgrade
and then set the incompat bit to force a repair run (which xfs_admin
will immediately take care of).
Hm. Now that you got me thinking, maybe there should be an exception
for a norecovery mount?
--D
> Brian
>
> > fs/xfs/libxfs/xfs_format.h | 7 +++++++
> > fs/xfs/xfs_mount.c | 6 ++++++
> > 2 files changed, 13 insertions(+)
> >
> >
> > diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> > index dd764da08f6f..5d8ba609ac0b 100644
> > --- a/fs/xfs/libxfs/xfs_format.h
> > +++ b/fs/xfs/libxfs/xfs_format.h
> > @@ -468,6 +468,7 @@ xfs_sb_has_ro_compat_feature(
> > #define XFS_SB_FEAT_INCOMPAT_SPINODES (1 << 1) /* sparse inode chunks */
> > #define XFS_SB_FEAT_INCOMPAT_META_UUID (1 << 2) /* metadata UUID */
> > #define XFS_SB_FEAT_INCOMPAT_BIGTIME (1 << 3) /* large timestamps */
> > +#define XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR (1 << 4) /* needs xfs_repair */
> > #define XFS_SB_FEAT_INCOMPAT_ALL \
> > (XFS_SB_FEAT_INCOMPAT_FTYPE| \
> > XFS_SB_FEAT_INCOMPAT_SPINODES| \
> > @@ -584,6 +585,12 @@ static inline bool xfs_sb_version_hasinobtcounts(struct xfs_sb *sbp)
> > (sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_INOBTCNT);
> > }
> >
> > +static inline bool xfs_sb_version_needsrepair(struct xfs_sb *sbp)
> > +{
> > + return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
> > + (sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR);
> > +}
> > +
> > /*
> > * end of superblock version macros
> > */
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index 7bc7901d648d..2853ad49b27d 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -266,6 +266,12 @@ xfs_sb_validate_mount(
> > struct xfs_buf *bp,
> > struct xfs_sb *sbp)
> > {
> > + /* Filesystem claims it needs repair, so refuse the mount. */
> > + if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > + xfs_warn(mp, "Filesystem needs repair. Please run xfs_repair.");
> > + return -EFSCORRUPTED;
> > + }
> > +
> > /*
> > * Don't touch the filesystem if a user tool thinks it owns the primary
> > * superblock. mkfs doesn't clear the flag from secondary supers, so
> >
>
