| Message ID | 160729617344.1606994.3329458995178500981.stgit@magnolia (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | xfs: add the ability to flag a fs for repair | expand |
On Sun, Dec 06, 2020 at 03:09:33PM -0800, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> lgtm Reviewed-by: Dave Chinner <dchinner@redhat.com>
On 12/6/20 5:09 PM, Darrick J. Wong wrote: > From: Darrick J. Wong <darrick.wong@oracle.com> > > 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@oracle.com> > Reviewed-by: Brian Foster <bfoster@redhat.com> > --- > fs/xfs/libxfs/xfs_format.h | 7 +++++++ > fs/xfs/xfs_super.c | 7 +++++++ > 2 files changed, 14 insertions(+) I'm curious, will this ever be used to make a filesystem unmountable if a verifier or scrub detects a problem? That might be some serious scope-creep; if not, I wonder if intent here should be in the commit log or in a comment. "if we otherwise detect anomalies" seems to leave that open. It doesn't change what we're doing here, I just wonder if we should indicate the current intent a bit more clearly to the code-and-commit reader. That said, for the change itself, Reviewed-by: Eric Sandeen <sandeen@redhat.com> > 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_super.c b/fs/xfs/xfs_super.c > index 599566c1a3b4..36002f460d7c 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1467,6 +1467,13 @@ xfs_fc_fill_super( > #endif > } > > + /* 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."); > + error = -EFSCORRUPTED; > + goto out_free_sb; > + } > + > /* > * 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 >
Who is going to set this flag? If the kernel ever sets it that is a good way to totally brick systems if it happens on the root file system.
On Wed, Dec 09, 2020 at 06:04:00PM +0000, Christoph Hellwig wrote: > Who is going to set this flag? If the kernel ever sets it that is > a good way to totally brick systems if it happens on the root file > system. So far the only user is xfs_db, when xfs_admin tells it to upgrade a v5 filesystem and we want/need to force the fs through xfs_repair: https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mad4ee9c757051692f33a993a348f4e4e61ac098b https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mb6ef416f9626d87610625e069f516945783a5c13 I don't think there's ever going to be a use case for the kernel setting the feature flag on a mounted fs -- if some error is fixable we should just have online repair fix it; or if it's truly catastrophic we don't want to write the disk at all. --D
On Wed, Dec 09, 2020 at 10:10:56AM -0800, Darrick J. Wong wrote: > On Wed, Dec 09, 2020 at 06:04:00PM +0000, Christoph Hellwig wrote: > > Who is going to set this flag? If the kernel ever sets it that is > > a good way to totally brick systems if it happens on the root file > > system. > > So far the only user is xfs_db, when xfs_admin tells it to upgrade a v5 > filesystem and we want/need to force the fs through xfs_repair: > > https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mad4ee9c757051692f33a993a348f4e4e61ac098b > https://lore.kernel.org/linux-xfs/160679383892.447856.12907477074923729733.stgit@magnolia/T/#mb6ef416f9626d87610625e069f516945783a5c13 > > I don't think there's ever going to be a use case for the kernel setting > the feature flag on a mounted fs -- if some error is fixable we should > just have online repair fix it; or if it's truly catastrophic we don't > want to write the disk at all. Maybe the definition wants a comment to explain this?
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_super.c b/fs/xfs/xfs_super.c index 599566c1a3b4..36002f460d7c 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1467,6 +1467,13 @@ xfs_fc_fill_super( #endif } + /* 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."); + error = -EFSCORRUPTED; + goto out_free_sb; + } + /* * 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