[PATCH v2 0/3] xfs: add the ability to flag a fs for repair

[PATCH v2 0/3] xfs: add the ability to flag a fs for repair

[Darrick J. Wong]

Hi all,

This "new feature" adds a new incompat feature flag so that we can force
a sysadmin to run xfs_repair on a filesystem before mounting.  The
intent for this code is to make it so that one can use xfs_db to upgrade
a filesystem to support new V5 features (e.g. y2038 or inode btree
counters).  Because some upgrades may require xfs_repair to fix or add
things before the filesystem goes back into use, this is the means for
xfs_db to force that to happen.

Note: xfs_admin will automatically run repair when required, so
sysadmins won't have to issue the repair command directly.

v2: move all the kernel-specific checks to xfs_fs_fill_super since that's
where we put the rest of them already

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=needsrepair-5.11
---
 fs/xfs/libxfs/xfs_format.h |   10 +++++++++-
 fs/xfs/libxfs/xfs_sb.c     |   27 ---------------------------
 fs/xfs/xfs_super.c         |   39 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 28 deletions(-)

[PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

A couple of the superblock validation checks apply only to the kernel,
so move them to xfs_fc_fill_super before we add the needsrepair "feature",
which will prevent the kernel (but not xfsprogs) from mounting the
filesystem.  This also reduces the diff between kernel and userspace
libxfs.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_sb.c |   27 ---------------------------
 fs/xfs/xfs_super.c     |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 27 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 5aeafa59ed27..05359690aaed 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -382,17 +382,6 @@ xfs_validate_sb_common(
 		return -EFSCORRUPTED;
 	}
 
-	/*
-	 * Until this is fixed only page-sized or smaller data blocks work.
-	 */
-	if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) {
-		xfs_warn(mp,
-		"File system with blocksize %d bytes. "
-		"Only pagesize (%ld) or less will currently work.",
-				sbp->sb_blocksize, PAGE_SIZE);
-		return -ENOSYS;
-	}
-
 	/*
 	 * Currently only very few inode sizes are supported.
 	 */
@@ -408,22 +397,6 @@ xfs_validate_sb_common(
 		return -ENOSYS;
 	}
 
-	if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) ||
-	    xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) {
-		xfs_warn(mp,
-		"file system too large to be mounted on this system.");
-		return -EFBIG;
-	}
-
-	/*
-	 * 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
-	 * we don't check them at all.
-	 */
-	if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
-		xfs_warn(mp, "Offline file system operation in progress!");
-		return -EFSCORRUPTED;
-	}
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index e3e229e52512..599566c1a3b4 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1467,6 +1467,38 @@ xfs_fc_fill_super(
 #endif
 	}
 
+	/*
+	 * 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
+	 * we don't check them at all.
+	 */
+	if (mp->m_sb.sb_inprogress) {
+		xfs_warn(mp, "Offline file system operation in progress!");
+		error = -EFSCORRUPTED;
+		goto out_free_sb;
+	}
+
+	/*
+	 * Until this is fixed only page-sized or smaller data blocks work.
+	 */
+	if (mp->m_sb.sb_blocksize > PAGE_SIZE) {
+		xfs_warn(mp,
+		"File system with blocksize %d bytes. "
+		"Only pagesize (%ld) or less will currently work.",
+				mp->m_sb.sb_blocksize, PAGE_SIZE);
+		error = -ENOSYS;
+		goto out_free_sb;
+	}
+
+	/* Ensure this filesystem fits in the page cache limits */
+	if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) ||
+	    xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) {
+		xfs_warn(mp,
+		"file system too large to be mounted on this system.");
+		error = -EFBIG;
+		goto out_free_sb;
+	}
+
 	/*
 	 * XFS block mappings use 54 bits to store the logical block offset.
 	 * This should suffice to handle the maximum file size that the VFS

[PATCH 2/3] xfs: define a new "needrepair" feature

[Darrick J. Wong]

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(+)


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

[PATCH 3/3] xfs: enable the needsrepair feature

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Make it so that libxfs recognizes the needsrepair feature.  Note that
the kernel will still refuse to mount these.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
Reviewed-by: Eric Sandeen <sandeen@redhat.com>
---
 fs/xfs/libxfs/xfs_format.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 5d8ba609ac0b..f64eed3ccfed 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -473,7 +473,8 @@ xfs_sb_has_ro_compat_feature(
 		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
 		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
 		 XFS_SB_FEAT_INCOMPAT_META_UUID| \
-		 XFS_SB_FEAT_INCOMPAT_BIGTIME)
+		 XFS_SB_FEAT_INCOMPAT_BIGTIME| \
+		 XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR)
 
 #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
 static inline bool

Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Dave Chinner]

On Sun, Dec 06, 2020 at 03:09:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A couple of the superblock validation checks apply only to the kernel,
> so move them to xfs_fc_fill_super before we add the needsrepair "feature",
> which will prevent the kernel (but not xfsprogs) from mounting the
> filesystem.  This also reduces the diff between kernel and userspace
> libxfs.

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Dave Chinner]

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>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: enable the needsrepair feature

[Dave Chinner]

On Sun, Dec 06, 2020 at 03:09:39PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Make it so that libxfs recognizes the needsrepair feature.  Note that
> the kernel will still refuse to mount these.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> Reviewed-by: Eric Sandeen <sandeen@redhat.com>
> ---
>  fs/xfs/libxfs/xfs_format.h |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
> index 5d8ba609ac0b..f64eed3ccfed 100644
> --- a/fs/xfs/libxfs/xfs_format.h
> +++ b/fs/xfs/libxfs/xfs_format.h
> @@ -473,7 +473,8 @@ xfs_sb_has_ro_compat_feature(
>  		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
>  		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
>  		 XFS_SB_FEAT_INCOMPAT_META_UUID| \
> -		 XFS_SB_FEAT_INCOMPAT_BIGTIME)
> +		 XFS_SB_FEAT_INCOMPAT_BIGTIME| \
> +		 XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR)
>  
>  #define XFS_SB_FEAT_INCOMPAT_UNKNOWN	~XFS_SB_FEAT_INCOMPAT_ALL
>  static inline bool

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Brian Foster]

On Sun, Dec 06, 2020 at 03:09:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A couple of the superblock validation checks apply only to the kernel,
> so move them to xfs_fc_fill_super before we add the needsrepair "feature",
> which will prevent the kernel (but not xfsprogs) from mounting the
> filesystem.  This also reduces the diff between kernel and userspace
> libxfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/libxfs/xfs_sb.c |   27 ---------------------------
>  fs/xfs/xfs_super.c     |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 27 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 5aeafa59ed27..05359690aaed 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -382,17 +382,6 @@ xfs_validate_sb_common(
>  		return -EFSCORRUPTED;
>  	}
>  
> -	/*
> -	 * Until this is fixed only page-sized or smaller data blocks work.
> -	 */
> -	if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) {
> -		xfs_warn(mp,
> -		"File system with blocksize %d bytes. "
> -		"Only pagesize (%ld) or less will currently work.",
> -				sbp->sb_blocksize, PAGE_SIZE);
> -		return -ENOSYS;
> -	}
> -
>  	/*
>  	 * Currently only very few inode sizes are supported.
>  	 */
> @@ -408,22 +397,6 @@ xfs_validate_sb_common(
>  		return -ENOSYS;
>  	}
>  
> -	if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) ||
> -	    xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) {
> -		xfs_warn(mp,
> -		"file system too large to be mounted on this system.");
> -		return -EFBIG;
> -	}
> -
> -	/*
> -	 * 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
> -	 * we don't check them at all.
> -	 */
> -	if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
> -		xfs_warn(mp, "Offline file system operation in progress!");
> -		return -EFSCORRUPTED;
> -	}
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..599566c1a3b4 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1467,6 +1467,38 @@ xfs_fc_fill_super(
>  #endif
>  	}
>  
> +	/*
> +	 * 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
> +	 * we don't check them at all.
> +	 */
> +	if (mp->m_sb.sb_inprogress) {
> +		xfs_warn(mp, "Offline file system operation in progress!");
> +		error = -EFSCORRUPTED;
> +		goto out_free_sb;
> +	}
> +
> +	/*
> +	 * Until this is fixed only page-sized or smaller data blocks work.
> +	 */
> +	if (mp->m_sb.sb_blocksize > PAGE_SIZE) {
> +		xfs_warn(mp,
> +		"File system with blocksize %d bytes. "
> +		"Only pagesize (%ld) or less will currently work.",
> +				mp->m_sb.sb_blocksize, PAGE_SIZE);
> +		error = -ENOSYS;
> +		goto out_free_sb;
> +	}
> +
> +	/* Ensure this filesystem fits in the page cache limits */
> +	if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) ||
> +	    xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) {
> +		xfs_warn(mp,
> +		"file system too large to be mounted on this system.");
> +		error = -EFBIG;
> +		goto out_free_sb;
> +	}
> +
>  	/*
>  	 * XFS block mappings use 54 bits to store the logical block offset.
>  	 * This should suffice to handle the maximum file size that the VFS
> 

Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Eric Sandeen]

On 12/6/20 5:09 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A couple of the superblock validation checks apply only to the kernel,
> so move them to xfs_fc_fill_super before we add the needsrepair "feature",
> which will prevent the kernel (but not xfsprogs) from mounting the
> filesystem.  This also reduces the diff between kernel and userspace
> libxfs.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Nice, I like this version :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>  fs/xfs/libxfs/xfs_sb.c |   27 ---------------------------
>  fs/xfs/xfs_super.c     |   32 ++++++++++++++++++++++++++++++++
>  2 files changed, 32 insertions(+), 27 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 5aeafa59ed27..05359690aaed 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -382,17 +382,6 @@ xfs_validate_sb_common(
>  		return -EFSCORRUPTED;
>  	}
>  
> -	/*
> -	 * Until this is fixed only page-sized or smaller data blocks work.
> -	 */
> -	if (unlikely(sbp->sb_blocksize > PAGE_SIZE)) {
> -		xfs_warn(mp,
> -		"File system with blocksize %d bytes. "
> -		"Only pagesize (%ld) or less will currently work.",
> -				sbp->sb_blocksize, PAGE_SIZE);
> -		return -ENOSYS;
> -	}
> -
>  	/*
>  	 * Currently only very few inode sizes are supported.
>  	 */
> @@ -408,22 +397,6 @@ xfs_validate_sb_common(
>  		return -ENOSYS;
>  	}
>  
> -	if (xfs_sb_validate_fsb_count(sbp, sbp->sb_dblocks) ||
> -	    xfs_sb_validate_fsb_count(sbp, sbp->sb_rblocks)) {
> -		xfs_warn(mp,
> -		"file system too large to be mounted on this system.");
> -		return -EFBIG;
> -	}
> -
> -	/*
> -	 * 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
> -	 * we don't check them at all.
> -	 */
> -	if (XFS_BUF_ADDR(bp) == XFS_SB_DADDR && sbp->sb_inprogress) {
> -		xfs_warn(mp, "Offline file system operation in progress!");
> -		return -EFSCORRUPTED;
> -	}
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index e3e229e52512..599566c1a3b4 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1467,6 +1467,38 @@ xfs_fc_fill_super(
>  #endif
>  	}
>  
> +	/*
> +	 * 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
> +	 * we don't check them at all.
> +	 */
> +	if (mp->m_sb.sb_inprogress) {
> +		xfs_warn(mp, "Offline file system operation in progress!");
> +		error = -EFSCORRUPTED;
> +		goto out_free_sb;
> +	}
> +
> +	/*
> +	 * Until this is fixed only page-sized or smaller data blocks work.
> +	 */
> +	if (mp->m_sb.sb_blocksize > PAGE_SIZE) {
> +		xfs_warn(mp,
> +		"File system with blocksize %d bytes. "
> +		"Only pagesize (%ld) or less will currently work.",
> +				mp->m_sb.sb_blocksize, PAGE_SIZE);
> +		error = -ENOSYS;
> +		goto out_free_sb;
> +	}
> +
> +	/* Ensure this filesystem fits in the page cache limits */
> +	if (xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_dblocks) ||
> +	    xfs_sb_validate_fsb_count(&mp->m_sb, mp->m_sb.sb_rblocks)) {
> +		xfs_warn(mp,
> +		"file system too large to be mounted on this system.");
> +		error = -EFBIG;
> +		goto out_free_sb;
> +	}
> +
>  	/*
>  	 * XFS block mappings use 54 bits to store the logical block offset.
>  	 * This should suffice to handle the maximum file size that the VFS
> 

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Eric Sandeen]

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
> 

Re: [PATCH 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Christoph Hellwig]

On Sun, Dec 06, 2020 at 03:09:26PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> A couple of the superblock validation checks apply only to the kernel,
> so move them to xfs_fc_fill_super before we add the needsrepair "feature",
> which will prevent the kernel (but not xfsprogs) from mounting the
> filesystem.  This also reduces the diff between kernel and userspace
> libxfs.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Christoph Hellwig]

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.

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Darrick J. Wong]

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

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Christoph Hellwig]

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?

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Darrick J. Wong]

On Fri, Dec 04, 2020 at 02:07:49PM -0600, Eric Sandeen wrote:
> On 12/1/20 10:18 AM, Brian Foster wrote:
> > On Mon, Nov 30, 2020 at 07:37:31PM -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>
> >> ---
> > 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?
> 
> I'm a fan of a straight-up incompat, because we don't really know what
> format changes in the future might require this flag to be set; nothing
> guarantees that future changes will be ro-compat-safe, right?

Correct.  In the case of the inobtcount upgrade, we know that the
inobt/finobt blockcounts in the AGI are zero (and thus wrong) right
after the upgrade.  If we made it a rocompat bit then we'd allow ro
mounts but we'd also have to be careful to prohibit a ro->rw remount,
at which point the admin gets a Big Surprise.

Why not just make the admin repair the system right then and there?
I mean, xfs_admin is already going to run repair anyway, so in practice
there shouldn't be that many people trying to push an "upgraded but
needs repair" fs at the kernel anyway.

--D

> -Eric

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Eric Sandeen]

On 12/1/20 10:18 AM, Brian Foster wrote:
> On Mon, Nov 30, 2020 at 07:37:31PM -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>
>> ---
> 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?

I'm a fan of a straight-up incompat, because we don't really know what
format changes in the future might require this flag to be set; nothing
guarantees that future changes will be ro-compat-safe, right?

-Eric

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Brian Foster]

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@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>
> > > ---
> > 
> > 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@redhat.com>

> --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
> > > 
> > 
> 

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Darrick J. Wong]

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@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>
> > ---
> 
> 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
> > 
> 

Re: [PATCH 2/3] xfs: define a new "needrepair" feature

[Brian Foster]

On Mon, Nov 30, 2020 at 07:37:31PM -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>
> ---

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?

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
> 

[PATCH 2/3] xfs: define a new "needrepair" feature

[Darrick J. Wong]

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>
---
 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