On Tue, Dec 01, 2020 at 08:25:39AM -0800, Darrick J. Wong wrote:
> 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?
>
Yeah, I was more thinking about for recovery purposes if something
happens to go wrong, so that should imply norecovery as well. Eh, I
suppose one could always clear the bit too in that case so it's not that
big of a deal. Either way:
Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> --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
> > >
> >
>
