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

[PATCH 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.

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     |   23 ++++-------------------
 fs/xfs/libxfs/xfs_sb.h     |    3 +++
 fs/xfs/xfs_mount.c         |   37 +++++++++++++++++++++++++++++++++++++
 4 files changed, 53 insertions(+), 20 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_mount.c before we start changing sb_inprogress.
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 |   23 ++++-------------------
 fs/xfs/libxfs/xfs_sb.h |    3 +++
 fs/xfs/xfs_mount.c     |   31 +++++++++++++++++++++++++++++++
 3 files changed, 38 insertions(+), 19 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 5aeafa59ed27..a2c43fe38f64 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -223,6 +223,7 @@ xfs_validate_sb_common(
 	struct xfs_dsb		*dsb = bp->b_addr;
 	uint32_t		agcount = 0;
 	uint32_t		rem;
+	int			error;
 
 	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
 		xfs_warn(mp, "bad magic number");
@@ -382,16 +383,9 @@ 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;
-	}
+	error = xfs_sb_validate_mount(mp, bp, sbp);
+	if (error)
+		return error;
 
 	/*
 	 * Currently only very few inode sizes are supported.
@@ -415,15 +409,6 @@ xfs_validate_sb_common(
 		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/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
index 92465a9a5162..ee0a5858dd47 100644
--- a/fs/xfs/libxfs/xfs_sb.h
+++ b/fs/xfs/libxfs/xfs_sb.h
@@ -42,4 +42,7 @@ extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
 				struct xfs_trans *tp, xfs_agnumber_t agno,
 				struct xfs_buf **bpp);
 
+int xfs_sb_validate_mount(struct xfs_mount *mp, struct xfs_buf *bp,
+		struct xfs_sb *sbp);
+
 #endif	/* __XFS_SB_H__ */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 7110507a2b6b..7bc7901d648d 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -259,6 +259,37 @@ xfs_initialize_perag(
 	return error;
 }
 
+/* Validate the superblock is compatible with this mount. */
+int
+xfs_sb_validate_mount(
+	struct xfs_mount	*mp,
+	struct xfs_buf		*bp,
+	struct xfs_sb		*sbp)
+{
+	/*
+	 * 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;
+	}
+
+	/*
+	 * 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;
+	}
+
+	return 0;
+}
+
 /*
  * xfs_readsb
  *

[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

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

[Brian Foster]

On Mon, Nov 30, 2020 at 07:37:25PM -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_mount.c before we start changing sb_inprogress.
> 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 |   23 ++++-------------------
>  fs/xfs/libxfs/xfs_sb.h |    3 +++
>  fs/xfs/xfs_mount.c     |   31 +++++++++++++++++++++++++++++++
>  3 files changed, 38 insertions(+), 19 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
> index 5aeafa59ed27..a2c43fe38f64 100644
> --- a/fs/xfs/libxfs/xfs_sb.c
> +++ b/fs/xfs/libxfs/xfs_sb.c
> @@ -223,6 +223,7 @@ xfs_validate_sb_common(
>  	struct xfs_dsb		*dsb = bp->b_addr;
>  	uint32_t		agcount = 0;
>  	uint32_t		rem;
> +	int			error;
>  
>  	if (!xfs_verify_magic(bp, dsb->sb_magicnum)) {
>  		xfs_warn(mp, "bad magic number");
> @@ -382,16 +383,9 @@ 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;
> -	}
> +	error = xfs_sb_validate_mount(mp, bp, sbp);
> +	if (error)
> +		return error;
>  
>  	/*
>  	 * Currently only very few inode sizes are supported.
> @@ -415,15 +409,6 @@ xfs_validate_sb_common(
>  		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/libxfs/xfs_sb.h b/fs/xfs/libxfs/xfs_sb.h
> index 92465a9a5162..ee0a5858dd47 100644
> --- a/fs/xfs/libxfs/xfs_sb.h
> +++ b/fs/xfs/libxfs/xfs_sb.h
> @@ -42,4 +42,7 @@ extern int	xfs_sb_get_secondary(struct xfs_mount *mp,
>  				struct xfs_trans *tp, xfs_agnumber_t agno,
>  				struct xfs_buf **bpp);
>  
> +int xfs_sb_validate_mount(struct xfs_mount *mp, struct xfs_buf *bp,
> +		struct xfs_sb *sbp);
> +
>  #endif	/* __XFS_SB_H__ */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 7110507a2b6b..7bc7901d648d 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -259,6 +259,37 @@ xfs_initialize_perag(
>  	return error;
>  }
>  
> +/* Validate the superblock is compatible with this mount. */
> +int
> +xfs_sb_validate_mount(
> +	struct xfs_mount	*mp,
> +	struct xfs_buf		*bp,
> +	struct xfs_sb		*sbp)
> +{
> +	/*
> +	 * 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;
> +	}
> +
> +	/*
> +	 * 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;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * xfs_readsb
>   *
> 

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
> 

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

[Brian Foster]

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

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

[PATCH 4/3] xfs_db: support the needsrepair feature flag in the version command

[Darrick J. Wong]

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

Teach the xfs_db version command about the 'needsrepair' flag, which can
be used to force the system administrator to repair the filesystem with
xfs_repair.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/check.c           |    5 ++
 db/sb.c              |  159 ++++++++++++++++++++++++++++++++++++++++++++++++--
 db/xfs_admin.sh      |    6 ++
 man/man8/xfs_admin.8 |   15 +++++
 man/man8/xfs_db.8    |    5 ++
 5 files changed, 182 insertions(+), 8 deletions(-)

diff --git a/db/check.c b/db/check.c
index 33736e33e833..485e855e8b78 100644
--- a/db/check.c
+++ b/db/check.c
@@ -3970,6 +3970,11 @@ scan_ag(
 			dbprintf(_("mkfs not completed successfully\n"));
 		error++;
 	}
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (!sflag)
+			dbprintf(_("filesystem needs xfs_repair\n"));
+		error++;
+	}
 	set_dbmap(agno, XFS_SB_BLOCK(mp), 1, DBM_SB, agno, XFS_SB_BLOCK(mp));
 	if (sb->sb_logstart && XFS_FSB_TO_AGNO(mp, sb->sb_logstart) == agno)
 		set_dbmap(agno, XFS_FSB_TO_AGBNO(mp, sb->sb_logstart),
diff --git a/db/sb.c b/db/sb.c
index d09f653dcedf..2c13d44d9954 100644
--- a/db/sb.c
+++ b/db/sb.c
@@ -379,6 +379,11 @@ uuid_f(
 				progname);
 			return 0;
 		}
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
 
 		if (!strcasecmp(argv[1], "generate")) {
 			platform_uuid_generate(&uu);
@@ -501,6 +506,7 @@ do_label(xfs_agnumber_t agno, char *label)
 		memcpy(&lbl[0], &tsb.sb_fname, sizeof(tsb.sb_fname));
 		return &lbl[0];
 	}
+
 	/* set label */
 	if ((len = strlen(label)) > sizeof(tsb.sb_fname)) {
 		if (agno == 0)
@@ -543,6 +549,12 @@ label_f(
 			return 0;
 		}
 
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
+
 		dbprintf(_("writing all SBs\n"));
 		for (ag = 0; ag < mp->m_sb.sb_agcount; ag++)
 			if ((p = do_label(ag, argv[1])) == NULL) {
@@ -584,6 +596,7 @@ version_help(void)
 " 'version attr1'    - enable v1 inline extended attributes\n"
 " 'version attr2'    - enable v2 inline extended attributes\n"
 " 'version log2'     - enable v2 log format\n"
+" 'version needsrepair' - flag filesystem as requiring repair\n"
 "\n"
 "The version function prints currently enabled features for a filesystem\n"
 "according to the version field of its primary superblock.\n"
@@ -620,6 +633,118 @@ do_version(xfs_agnumber_t agno, uint16_t version, uint32_t features)
 	return 1;
 }
 
+struct v5feat {
+	uint32_t		compat;
+	uint32_t		ro_compat;
+	uint32_t		incompat;
+	uint32_t		log_incompat;
+};
+
+static void
+get_v5_features(
+	struct xfs_mount	*mp,
+	struct v5feat		*feat)
+{
+	feat->compat = mp->m_sb.sb_features_compat;
+	feat->ro_compat = mp->m_sb.sb_features_ro_compat;
+	feat->incompat = mp->m_sb.sb_features_incompat;
+	feat->log_incompat = mp->m_sb.sb_features_log_incompat;
+}
+
+static bool
+set_v5_features(
+	struct xfs_mount	*mp,
+	const struct v5feat	*upgrade)
+{
+	struct xfs_sb		tsb;
+	struct v5feat		old;
+	xfs_agnumber_t		agno = 0;
+	xfs_agnumber_t		revert_agno = 0;
+
+	if (upgrade->compat == mp->m_sb.sb_features_compat &&
+	    upgrade->ro_compat == mp->m_sb.sb_features_ro_compat &&
+	    upgrade->incompat == mp->m_sb.sb_features_incompat &&
+	    upgrade->log_incompat == mp->m_sb.sb_features_log_incompat)
+		return true;
+
+	/* Upgrade primary superblock. */
+	if (!get_sb(agno, &tsb))
+		goto fail;
+
+	dbprintf(_("Upgrading V5 filesystem\n"));
+
+	/* Save old values */
+	old.compat = tsb.sb_features_compat;
+	old.ro_compat = tsb.sb_features_ro_compat;
+	old.incompat = tsb.sb_features_incompat;
+	old.log_incompat = tsb.sb_features_log_incompat;
+
+	/* Update feature flags and force user to run repair before mounting. */
+	tsb.sb_features_compat |= upgrade->compat;
+	tsb.sb_features_ro_compat |= upgrade->ro_compat;
+	tsb.sb_features_incompat |= upgrade->incompat;
+	tsb.sb_features_log_incompat |= upgrade->log_incompat;
+	tsb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+	libxfs_sb_to_disk(iocur_top->data, &tsb);
+
+	/* Write new primary superblock */
+	write_cur();
+	if (!iocur_top->bp || iocur_top->bp->b_error)
+		goto fail;
+
+	/* Update the secondary superblocks, or revert. */
+	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
+		if (!get_sb(agno, &tsb)) {
+			agno--;
+			goto revert;
+		}
+
+		/* Set features on secondary suepr. */
+		tsb.sb_features_compat |= upgrade->compat;
+		tsb.sb_features_ro_compat |= upgrade->ro_compat;
+		tsb.sb_features_incompat |= upgrade->incompat;
+		tsb.sb_features_log_incompat |= upgrade->log_incompat;
+		libxfs_sb_to_disk(iocur_top->data, &tsb);
+		write_cur();
+
+		/* Write or abort. */
+		if (!iocur_top->bp || iocur_top->bp->b_error)
+			goto revert;
+	}
+
+	/* All superblocks updated, update the incore values. */
+	mp->m_sb.sb_features_compat |= upgrade->compat;
+	mp->m_sb.sb_features_ro_compat |= upgrade->ro_compat;
+	mp->m_sb.sb_features_incompat |= upgrade->incompat;
+	mp->m_sb.sb_features_log_incompat |= upgrade->log_incompat;
+	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+
+	dbprintf(_("Upgraded V5 filesystem.  Please run xfs_repair.\n"));
+	return true;
+
+revert:
+	/*
+	 * Try to revert feature flag changes, and don't worry if we fail.
+	 * We're probably in a mess anyhow, and the admin will have to run
+	 * repair anyways.
+	 */
+	for (revert_agno = 0; revert_agno <= agno; revert_agno++) {
+		if (!get_sb(revert_agno, &tsb))
+			continue;
+
+		tsb.sb_features_compat = old.compat;
+		tsb.sb_features_ro_compat = old.ro_compat;
+		tsb.sb_features_incompat = old.incompat;
+		tsb.sb_features_log_incompat = old.log_incompat;
+		libxfs_sb_to_disk(iocur_top->data, &tsb);
+		write_cur();
+	}
+fail:
+	dbprintf(
+_("Failed to upgrade V5 filesystem at AG %d, please run xfs_repair.\n"), agno);
+	return false;
+}
+
 static char *
 version_string(
 	xfs_sb_t	*sbp)
@@ -691,15 +816,12 @@ version_string(
 		strcat(s, ",INOBTCNT");
 	if (xfs_sb_version_hasbigtime(sbp))
 		strcat(s, ",BIGTIME");
+	if (xfs_sb_version_needsrepair(sbp))
+		strcat(s, ",NEEDSREPAIR");
 	return s;
 }
 
-/*
- * XXX: this only supports reading and writing to version 4 superblock fields.
- * V5 superblocks always define certain V4 feature bits - they are blocked from
- * being changed if a V5 sb is detected, but otherwise v5 superblock features
- * are not handled here.
- */
+/* Upgrade a superblock to support a feature. */
 static int
 version_f(
 	int		argc,
@@ -710,6 +832,9 @@ version_f(
 	xfs_agnumber_t	ag;
 
 	if (argc == 2) {	/* WRITE VERSION */
+		struct v5feat	v5features;
+
+		get_v5_features(mp, &v5features);
 
 		if ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode) {
 			dbprintf(_("%s: not in expert mode, writing disabled\n"),
@@ -717,8 +842,23 @@ version_f(
 			return 0;
 		}
 
+		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
+			dbprintf(_("%s: filesystem needs xfs_repair\n"),
+				progname);
+			return 0;
+		}
+
 		/* Logic here derived from the IRIX xfs_chver(1M) script. */
-		if (!strcasecmp(argv[1], "extflg")) {
+		if (!strcasecmp(argv[1], "needsrepair")) {
+			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
+				dbprintf(
+		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
+				exitcode = 1;
+				return 1;
+			}
+
+			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		} else if (!strcasecmp(argv[1], "extflg")) {
 			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
 			case XFS_SB_VERSION_1:
 				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
@@ -809,6 +949,11 @@ version_f(
 			mp->m_sb.sb_versionnum = version;
 			mp->m_sb.sb_features2 = features;
 		}
+
+		if (!set_v5_features(mp, &v5features)) {
+			exitcode = 1;
+			return 1;
+		}
 	}
 
 	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index bd325da2f776..41a14d4521ba 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -9,7 +9,7 @@ DB_OPTS=""
 REPAIR_OPTS=""
 USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
 
-while getopts "efjlpuc:L:U:V" c
+while getopts "efjlpuc:L:O:U:V" c
 do
 	case $c in
 	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
@@ -19,6 +19,9 @@ do
 	l)	DB_OPTS=$DB_OPTS" -r -c label";;
 	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
 	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
+	O)	DB_OPTS=$DB_OPTS" -c 'version "$OPTARG"'";
+		# Force repair to run by adding a single space to REPAIR_OPTS
+		REPAIR_OPTS="$REPAIR_OPTS ";;
 	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
 	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
 	V)	xfs_db -p xfs_admin -V
@@ -48,6 +51,7 @@ case $# in
 		fi
 		if [ -n "$REPAIR_OPTS" ]
 		then
+			echo "Running xfs_repair to ensure filesystem consistency."
 			# Hide normal repair output which is sent to stderr
 			# assuming the filesystem is fine when a user is
 			# running xfs_admin.
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index 8afc873fb50a..b423981d8049 100644
--- a/man/man8/xfs_admin.8
+++ b/man/man8/xfs_admin.8
@@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
 [
 .B \-eflpu
 ] [
+.BI \-O " feature"
+] [
 .BR "\-c 0" | 1
 ] [
 .B \-L
@@ -103,6 +105,19 @@ The filesystem label can be cleared using the special "\c
 " value for
 .IR label .
 .TP
+.BI \-O " feature"
+Add a new feature to the filesystem.
+Only one feature can be specified at a time.
+Features are as follows:
+.RS 0.7i
+.TP
+.B needsrepair
+If this is a V5 filesystem, flag the filesystem as needing repairs.
+Until
+.BR xfs_repair (8)
+is run, the filesystem will not be mountable.
+.RE
+.TP
 .BI \-U " uuid"
 Set the UUID of the filesystem to
 .IR uuid .
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 587274957de3..7331cf196bb0 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -971,6 +971,11 @@ may toggle between
 and
 .B attr2
 at will (older kernels may not support the newer version).
+The filesystem can be flagged as requiring a run through
+.BR xfs_repair (8)
+if the
+.B needsrepair
+option is specified and the filesystem is formatted with the V5 format.
 .IP
 If no argument is given, the current version and feature bits are printed.
 With one argument, this command will write the updated version number

[PATCH 5/3] xfs_repair: clear the needsrepair flag

[Darrick J. Wong]

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

Clear the needsrepair flag, since it's used to prevent mounting of an
inconsistent filesystem.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/agheader.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/repair/agheader.c b/repair/agheader.c
index f28d8a7bb0de..53f541d4a53a 100644
--- a/repair/agheader.c
+++ b/repair/agheader.c
@@ -452,6 +452,17 @@ secondary_sb_whack(
 			rval |= XR_AG_SB_SEC;
 	}
 
+	if (xfs_sb_version_needsrepair(sb)) {
+		if (!no_modify)
+			sb->sb_features_incompat &=
+					~XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
+		if (!do_bzero) {
+			rval |= XR_AG_SB;
+			do_warn(_("needsrepair flag set in sb %d\n"), i);
+		} else
+			rval |= XR_AG_SB_SEC;
+	}
+
 	return(rval);
 }
 

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 4/3] xfs_db: support the needsrepair feature flag in the version command

[Eric Sandeen]

On 12/3/20 7:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Teach the xfs_db version command about the 'needsrepair' flag, which can
> be used to force the system administrator to repair the filesystem with
> xfs_repair.

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/check.c           |    5 ++
>  db/sb.c              |  159 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  db/xfs_admin.sh      |    6 ++
>  man/man8/xfs_admin.8 |   15 +++++
>  man/man8/xfs_db.8    |    5 ++
>  5 files changed, 182 insertions(+), 8 deletions(-)
> 
> diff --git a/db/check.c b/db/check.c
> index 33736e33e833..485e855e8b78 100644
> --- a/db/check.c
> +++ b/db/check.c
> @@ -3970,6 +3970,11 @@ scan_ag(
>  			dbprintf(_("mkfs not completed successfully\n"));
>  		error++;
>  	}
> +	if (xfs_sb_version_needsrepair(sb)) {
> +		if (!sflag)
> +			dbprintf(_("filesystem needs xfs_repair\n"));
> +		error++;
> +	}

ok this just marks it as an encountered error... *shrug* seems fine

>  	set_dbmap(agno, XFS_SB_BLOCK(mp), 1, DBM_SB, agno, XFS_SB_BLOCK(mp));
>  	if (sb->sb_logstart && XFS_FSB_TO_AGNO(mp, sb->sb_logstart) == agno)
>  		set_dbmap(agno, XFS_FSB_TO_AGBNO(mp, sb->sb_logstart),
> diff --git a/db/sb.c b/db/sb.c
> index d09f653dcedf..2c13d44d9954 100644
> --- a/db/sb.c
> +++ b/db/sb.c
> @@ -379,6 +379,11 @@ uuid_f(
>  				progname);
>  			return 0;
>  		}
> +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> +				progname);
> +			return 0;
> +		}
>  
>  		if (!strcasecmp(argv[1], "generate")) {
>  			platform_uuid_generate(&uu);
> @@ -501,6 +506,7 @@ do_label(xfs_agnumber_t agno, char *label)
>  		memcpy(&lbl[0], &tsb.sb_fname, sizeof(tsb.sb_fname));
>  		return &lbl[0];
>  	}
> +
>  	/* set label */
>  	if ((len = strlen(label)) > sizeof(tsb.sb_fname)) {
>  		if (agno == 0)
> @@ -543,6 +549,12 @@ label_f(
>  			return 0;
>  		}
>  
> +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> +				progname);
> +			return 0;
> +		}
> +

why are uuid_f and label_f uniquely disallowed from operating on a needsrepair
filesystem?  (is it because they are unique in that they rewrite all superblocks?)

How will we know when newly written routines should also exclude
these filesystems?  Should this just be caught earlier and refuse to operate
at all, maybe unless "-x" is specified?

Or if it's the "they write all supers" that's the problem, should we factor that
out into a helper that does the needsrepair check and bails if set?

>  		dbprintf(_("writing all SBs\n"));
>  		for (ag = 0; ag < mp->m_sb.sb_agcount; ag++)
>  			if ((p = do_label(ag, argv[1])) == NULL) {
> @@ -584,6 +596,7 @@ version_help(void)
>  " 'version attr1'    - enable v1 inline extended attributes\n"
>  " 'version attr2'    - enable v2 inline extended attributes\n"
>  " 'version log2'     - enable v2 log format\n"
> +" 'version needsrepair' - flag filesystem as requiring repair\n"
>  "\n"
>  "The version function prints currently enabled features for a filesystem\n"
>  "according to the version field of its primary superblock.\n"
> @@ -620,6 +633,118 @@ do_version(xfs_agnumber_t agno, uint16_t version, uint32_t features)
>  	return 1;
>  }
>  
> +struct v5feat {
> +	uint32_t		compat;
> +	uint32_t		ro_compat;
> +	uint32_t		incompat;
> +	uint32_t		log_incompat;
> +};
> +
> +static void
> +get_v5_features(
> +	struct xfs_mount	*mp,
> +	struct v5feat		*feat)
> +{
> +	feat->compat = mp->m_sb.sb_features_compat;
> +	feat->ro_compat = mp->m_sb.sb_features_ro_compat;
> +	feat->incompat = mp->m_sb.sb_features_incompat;
> +	feat->log_incompat = mp->m_sb.sb_features_log_incompat;
> +}
> +
> +static bool
> +set_v5_features(
> +	struct xfs_mount	*mp,
> +	const struct v5feat	*upgrade)
> +{
> +	struct xfs_sb		tsb;
> +	struct v5feat		old;
> +	xfs_agnumber_t		agno = 0;
> +	xfs_agnumber_t		revert_agno = 0;
> +
> +	if (upgrade->compat == mp->m_sb.sb_features_compat &&
> +	    upgrade->ro_compat == mp->m_sb.sb_features_ro_compat &&
> +	    upgrade->incompat == mp->m_sb.sb_features_incompat &&
> +	    upgrade->log_incompat == mp->m_sb.sb_features_log_incompat)
> +		return true;
> +
> +	/* Upgrade primary superblock. */
> +	if (!get_sb(agno, &tsb))
> +		goto fail;
> +
> +	dbprintf(_("Upgrading V5 filesystem\n"));
> +
> +	/* Save old values */
> +	old.compat = tsb.sb_features_compat;
> +	old.ro_compat = tsb.sb_features_ro_compat;
> +	old.incompat = tsb.sb_features_incompat;
> +	old.log_incompat = tsb.sb_features_log_incompat;
> +
> +	/* Update feature flags and force user to run repair before mounting. */
> +	tsb.sb_features_compat |= upgrade->compat;
> +	tsb.sb_features_ro_compat |= upgrade->ro_compat;
> +	tsb.sb_features_incompat |= upgrade->incompat;
> +	tsb.sb_features_log_incompat |= upgrade->log_incompat;
> +	tsb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;

Should this be optional?

I guess if we ever have a V5 feature upgrade that does not need repair, we can
make it optional at that point.  the feture you have added, needsrepair, does in
fact need repair.  :)

> +	libxfs_sb_to_disk(iocur_top->data, &tsb);
> +
> +	/* Write new primary superblock */
> +	write_cur();
> +	if (!iocur_top->bp || iocur_top->bp->b_error)
> +		goto fail;
> +
> +	/* Update the secondary superblocks, or revert. */
> +	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
> +		if (!get_sb(agno, &tsb)) {
> +			agno--;
> +			goto revert;
> +		}
> +
> +		/* Set features on secondary suepr. */
> +		tsb.sb_features_compat |= upgrade->compat;
> +		tsb.sb_features_ro_compat |= upgrade->ro_compat;
> +		tsb.sb_features_incompat |= upgrade->incompat;
> +		tsb.sb_features_log_incompat |= upgrade->log_incompat;
> +		libxfs_sb_to_disk(iocur_top->data, &tsb);
> +		write_cur();
> +
> +		/* Write or abort. */
> +		if (!iocur_top->bp || iocur_top->bp->b_error)
> +			goto revert;
> +	}
> +
> +	/* All superblocks updated, update the incore values. */
> +	mp->m_sb.sb_features_compat |= upgrade->compat;
> +	mp->m_sb.sb_features_ro_compat |= upgrade->ro_compat;
> +	mp->m_sb.sb_features_incompat |= upgrade->incompat;
> +	mp->m_sb.sb_features_log_incompat |= upgrade->log_incompat;
> +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +
> +	dbprintf(_("Upgraded V5 filesystem.  Please run xfs_repair.\n"));
> +	return true;
> +
> +revert:
> +	/*
> +	 * Try to revert feature flag changes, and don't worry if we fail.
> +	 * We're probably in a mess anyhow, and the admin will have to run
> +	 * repair anyways.
> +	 */
> +	for (revert_agno = 0; revert_agno <= agno; revert_agno++) {
> +		if (!get_sb(revert_agno, &tsb))
> +			continue;
> +
> +		tsb.sb_features_compat = old.compat;
> +		tsb.sb_features_ro_compat = old.ro_compat;
> +		tsb.sb_features_incompat = old.incompat;
> +		tsb.sb_features_log_incompat = old.log_incompat;
> +		libxfs_sb_to_disk(iocur_top->data, &tsb);
> +		write_cur();
> +	}
> +fail:
> +	dbprintf(
> +_("Failed to upgrade V5 filesystem at AG %d, please run xfs_repair.\n"), agno);
> +	return false;
> +}
> +
>  static char *
>  version_string(
>  	xfs_sb_t	*sbp)
> @@ -691,15 +816,12 @@ version_string(
>  		strcat(s, ",INOBTCNT");
>  	if (xfs_sb_version_hasbigtime(sbp))
>  		strcat(s, ",BIGTIME");
> +	if (xfs_sb_version_needsrepair(sbp))
> +		strcat(s, ",NEEDSREPAIR");
>  	return s;
>  }
>  
> -/*
> - * XXX: this only supports reading and writing to version 4 superblock fields.
> - * V5 superblocks always define certain V4 feature bits - they are blocked from
> - * being changed if a V5 sb is detected, but otherwise v5 superblock features
> - * are not handled here.
> - */
> +/* Upgrade a superblock to support a feature. */
>  static int
>  version_f(
>  	int		argc,
> @@ -710,6 +832,9 @@ version_f(
>  	xfs_agnumber_t	ag;
>  
>  	if (argc == 2) {	/* WRITE VERSION */
> +		struct v5feat	v5features;
> +
> +		get_v5_features(mp, &v5features);
>  
>  		if ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode) {
>  			dbprintf(_("%s: not in expert mode, writing disabled\n"),
> @@ -717,8 +842,23 @@ version_f(
>  			return 0;
>  		}
>  
> +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> +				progname);
> +			return 0;
> +		}

I guess this enforces a repair after each version change, which makes sense.

> +
>  		/* Logic here derived from the IRIX xfs_chver(1M) script. */
> -		if (!strcasecmp(argv[1], "extflg")) {
> +		if (!strcasecmp(argv[1], "needsrepair")) {
> +			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> +				dbprintf(
> +		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
> +				exitcode = 1;
> +				return 1;
> +			}
> +
> +			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> +		} else if (!strcasecmp(argv[1], "extflg")) {
>  			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
>  			case XFS_SB_VERSION_1:
>  				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
> @@ -809,6 +949,11 @@ version_f(
>  			mp->m_sb.sb_versionnum = version;
>  			mp->m_sb.sb_features2 = features;
>  		}
> +
> +		if (!set_v5_features(mp, &v5features)) {
> +			exitcode = 1;
> +			return 1;
> +		}
>  	}
>  
>  	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
> diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> index bd325da2f776..41a14d4521ba 100755
> --- a/db/xfs_admin.sh
> +++ b/db/xfs_admin.sh
> @@ -9,7 +9,7 @@ DB_OPTS=""
>  REPAIR_OPTS=""
>  USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
>  
> -while getopts "efjlpuc:L:U:V" c
> +while getopts "efjlpuc:L:O:U:V" c
>  do
>  	case $c in
>  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> @@ -19,6 +19,9 @@ do
>  	l)	DB_OPTS=$DB_OPTS" -r -c label";;
>  	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
>  	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
> +	O)	DB_OPTS=$DB_OPTS" -c 'version "$OPTARG"'";
> +		# Force repair to run by adding a single space to REPAIR_OPTS
> +		REPAIR_OPTS="$REPAIR_OPTS ";;
>  	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
>  	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
>  	V)	xfs_db -p xfs_admin -V
> @@ -48,6 +51,7 @@ case $# in
>  		fi
>  		if [ -n "$REPAIR_OPTS" ]
>  		then
> +			echo "Running xfs_repair to ensure filesystem consistency."
>  			# Hide normal repair output which is sent to stderr
>  			# assuming the filesystem is fine when a user is
>  			# running xfs_admin.
> diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> index 8afc873fb50a..b423981d8049 100644
> --- a/man/man8/xfs_admin.8
> +++ b/man/man8/xfs_admin.8
> @@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
>  [
>  .B \-eflpu
>  ] [
> +.BI \-O " feature"
> +] [
>  .BR "\-c 0" | 1
>  ] [
>  .B \-L
> @@ -103,6 +105,19 @@ The filesystem label can be cleared using the special "\c
>  " value for
>  .IR label .
>  .TP
> +.BI \-O " feature"
> +Add a new feature to the filesystem.
> +Only one feature can be specified at a time.
> +Features are as follows:
> +.RS 0.7i
> +.TP
> +.B needsrepair
> +If this is a V5 filesystem, flag the filesystem as needing repairs.
> +Until
> +.BR xfs_repair (8)
> +is run, the filesystem will not be mountable.
> +.RE
> +.TP

At some point I would like to add "projid32bit" and "extflg" and "log2" etc
to the "-O" documentation, and maybe deprecate the old feature-specific
options, since "-O projid32bit" et al come for free now.

>  .BI \-U " uuid"
>  Set the UUID of the filesystem to
>  .IR uuid .
> diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> index 587274957de3..7331cf196bb0 100644
> --- a/man/man8/xfs_db.8
> +++ b/man/man8/xfs_db.8
> @@ -971,6 +971,11 @@ may toggle between
>  and
>  .B attr2
>  at will (older kernels may not support the newer version).
> +The filesystem can be flagged as requiring a run through
> +.BR xfs_repair (8)
> +if the
> +.B needsrepair
> +option is specified and the filesystem is formatted with the V5 format.
>  .IP
>  If no argument is given, the current version and feature bits are printed.
>  With one argument, this command will write the updated version number
> 

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

[Eric Sandeen]

On 11/30/20 9:37 PM, 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.

Not sure it needs to be a separate patch from the prior one, but it
looks fine.

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

But maybe give me a chance to play with userspace in anger before you
merge these? ;)

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.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

[Eric Sandeen]

On 11/30/20 9:37 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_mount.c before we start changing sb_inprogress.
> This also reduces the diff between kernel and userspace libxfs.

My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
at all, and nobody reading the code or comments will know why we've chosen
to move just these two checks out of the common validator...

What does "compatible with this mount" mean?

Maybe just fess up in the comment, and say "these checks are different 
for kernel vs. userspace so we keep them over here" - and as for the
function name, *shrug* not sure I have anything better...

-Eric

Re: [PATCH 4/3] xfs_db: support the needsrepair feature flag in the version command

[Darrick J. Wong]

On Fri, Dec 04, 2020 at 02:32:32PM -0600, Eric Sandeen wrote:
> On 12/3/20 7:13 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Teach the xfs_db version command about the 'needsrepair' flag, which can
> > be used to force the system administrator to repair the filesystem with
> > xfs_repair.
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  db/check.c           |    5 ++
> >  db/sb.c              |  159 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  db/xfs_admin.sh      |    6 ++
> >  man/man8/xfs_admin.8 |   15 +++++
> >  man/man8/xfs_db.8    |    5 ++
> >  5 files changed, 182 insertions(+), 8 deletions(-)
> > 
> > diff --git a/db/check.c b/db/check.c
> > index 33736e33e833..485e855e8b78 100644
> > --- a/db/check.c
> > +++ b/db/check.c
> > @@ -3970,6 +3970,11 @@ scan_ag(
> >  			dbprintf(_("mkfs not completed successfully\n"));
> >  		error++;
> >  	}
> > +	if (xfs_sb_version_needsrepair(sb)) {
> > +		if (!sflag)
> > +			dbprintf(_("filesystem needs xfs_repair\n"));
> > +		error++;
> > +	}
> 
> ok this just marks it as an encountered error... *shrug* seems fine
> 
> >  	set_dbmap(agno, XFS_SB_BLOCK(mp), 1, DBM_SB, agno, XFS_SB_BLOCK(mp));
> >  	if (sb->sb_logstart && XFS_FSB_TO_AGNO(mp, sb->sb_logstart) == agno)
> >  		set_dbmap(agno, XFS_FSB_TO_AGBNO(mp, sb->sb_logstart),
> > diff --git a/db/sb.c b/db/sb.c
> > index d09f653dcedf..2c13d44d9954 100644
> > --- a/db/sb.c
> > +++ b/db/sb.c
> > @@ -379,6 +379,11 @@ uuid_f(
> >  				progname);
> >  			return 0;
> >  		}
> > +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> > +				progname);
> > +			return 0;
> > +		}
> >  
> >  		if (!strcasecmp(argv[1], "generate")) {
> >  			platform_uuid_generate(&uu);
> > @@ -501,6 +506,7 @@ do_label(xfs_agnumber_t agno, char *label)
> >  		memcpy(&lbl[0], &tsb.sb_fname, sizeof(tsb.sb_fname));
> >  		return &lbl[0];
> >  	}
> > +
> >  	/* set label */
> >  	if ((len = strlen(label)) > sizeof(tsb.sb_fname)) {
> >  		if (agno == 0)
> > @@ -543,6 +549,12 @@ label_f(
> >  			return 0;
> >  		}
> >  
> > +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> > +				progname);
> > +			return 0;
> > +		}
> > +
> 
> why are uuid_f and label_f uniquely disallowed from operating on a needsrepair
> filesystem?  (is it because they are unique in that they rewrite all superblocks?)

Because ... the fs needs repair, so the admin should repair the fs
before they try to modify the fs.  We don't allow mounting, so we
shouldn't allow label/uuid changes.

> How will we know when newly written routines should also exclude
> these filesystems?  Should this just be caught earlier and refuse to operate
> at all, maybe unless "-x" is specified?
> 
> Or if it's the "they write all supers" that's the problem, should we factor that
> out into a helper that does the needsrepair check and bails if set?
> 
> >  		dbprintf(_("writing all SBs\n"));
> >  		for (ag = 0; ag < mp->m_sb.sb_agcount; ag++)
> >  			if ((p = do_label(ag, argv[1])) == NULL) {
> > @@ -584,6 +596,7 @@ version_help(void)
> >  " 'version attr1'    - enable v1 inline extended attributes\n"
> >  " 'version attr2'    - enable v2 inline extended attributes\n"
> >  " 'version log2'     - enable v2 log format\n"
> > +" 'version needsrepair' - flag filesystem as requiring repair\n"
> >  "\n"
> >  "The version function prints currently enabled features for a filesystem\n"
> >  "according to the version field of its primary superblock.\n"
> > @@ -620,6 +633,118 @@ do_version(xfs_agnumber_t agno, uint16_t version, uint32_t features)
> >  	return 1;
> >  }
> >  
> > +struct v5feat {
> > +	uint32_t		compat;
> > +	uint32_t		ro_compat;
> > +	uint32_t		incompat;
> > +	uint32_t		log_incompat;
> > +};
> > +
> > +static void
> > +get_v5_features(
> > +	struct xfs_mount	*mp,
> > +	struct v5feat		*feat)
> > +{
> > +	feat->compat = mp->m_sb.sb_features_compat;
> > +	feat->ro_compat = mp->m_sb.sb_features_ro_compat;
> > +	feat->incompat = mp->m_sb.sb_features_incompat;
> > +	feat->log_incompat = mp->m_sb.sb_features_log_incompat;
> > +}
> > +
> > +static bool
> > +set_v5_features(
> > +	struct xfs_mount	*mp,
> > +	const struct v5feat	*upgrade)
> > +{
> > +	struct xfs_sb		tsb;
> > +	struct v5feat		old;
> > +	xfs_agnumber_t		agno = 0;
> > +	xfs_agnumber_t		revert_agno = 0;
> > +
> > +	if (upgrade->compat == mp->m_sb.sb_features_compat &&
> > +	    upgrade->ro_compat == mp->m_sb.sb_features_ro_compat &&
> > +	    upgrade->incompat == mp->m_sb.sb_features_incompat &&
> > +	    upgrade->log_incompat == mp->m_sb.sb_features_log_incompat)
> > +		return true;
> > +
> > +	/* Upgrade primary superblock. */
> > +	if (!get_sb(agno, &tsb))
> > +		goto fail;
> > +
> > +	dbprintf(_("Upgrading V5 filesystem\n"));
> > +
> > +	/* Save old values */
> > +	old.compat = tsb.sb_features_compat;
> > +	old.ro_compat = tsb.sb_features_ro_compat;
> > +	old.incompat = tsb.sb_features_incompat;
> > +	old.log_incompat = tsb.sb_features_log_incompat;
> > +
> > +	/* Update feature flags and force user to run repair before mounting. */
> > +	tsb.sb_features_compat |= upgrade->compat;
> > +	tsb.sb_features_ro_compat |= upgrade->ro_compat;
> > +	tsb.sb_features_incompat |= upgrade->incompat;
> > +	tsb.sb_features_log_incompat |= upgrade->log_incompat;
> > +	tsb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> 
> Should this be optional?
> 
> I guess if we ever have a V5 feature upgrade that does not need repair, we can
> make it optional at that point.  the feture you have added, needsrepair, does in
> fact need repair.  :)

The bigtime upgrade doesn't require a repair, but it seemed like a good
idea to make the admin run repair to ensure customer satisfaction. ;)

> > +	libxfs_sb_to_disk(iocur_top->data, &tsb);
> > +
> > +	/* Write new primary superblock */
> > +	write_cur();
> > +	if (!iocur_top->bp || iocur_top->bp->b_error)
> > +		goto fail;
> > +
> > +	/* Update the secondary superblocks, or revert. */
> > +	for (agno = 1; agno < mp->m_sb.sb_agcount; agno++) {
> > +		if (!get_sb(agno, &tsb)) {
> > +			agno--;
> > +			goto revert;
> > +		}
> > +
> > +		/* Set features on secondary suepr. */
> > +		tsb.sb_features_compat |= upgrade->compat;
> > +		tsb.sb_features_ro_compat |= upgrade->ro_compat;
> > +		tsb.sb_features_incompat |= upgrade->incompat;
> > +		tsb.sb_features_log_incompat |= upgrade->log_incompat;
> > +		libxfs_sb_to_disk(iocur_top->data, &tsb);
> > +		write_cur();
> > +
> > +		/* Write or abort. */
> > +		if (!iocur_top->bp || iocur_top->bp->b_error)
> > +			goto revert;
> > +	}
> > +
> > +	/* All superblocks updated, update the incore values. */
> > +	mp->m_sb.sb_features_compat |= upgrade->compat;
> > +	mp->m_sb.sb_features_ro_compat |= upgrade->ro_compat;
> > +	mp->m_sb.sb_features_incompat |= upgrade->incompat;
> > +	mp->m_sb.sb_features_log_incompat |= upgrade->log_incompat;
> > +	mp->m_sb.sb_features_incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +
> > +	dbprintf(_("Upgraded V5 filesystem.  Please run xfs_repair.\n"));
> > +	return true;
> > +
> > +revert:
> > +	/*
> > +	 * Try to revert feature flag changes, and don't worry if we fail.
> > +	 * We're probably in a mess anyhow, and the admin will have to run
> > +	 * repair anyways.
> > +	 */
> > +	for (revert_agno = 0; revert_agno <= agno; revert_agno++) {
> > +		if (!get_sb(revert_agno, &tsb))
> > +			continue;
> > +
> > +		tsb.sb_features_compat = old.compat;
> > +		tsb.sb_features_ro_compat = old.ro_compat;
> > +		tsb.sb_features_incompat = old.incompat;
> > +		tsb.sb_features_log_incompat = old.log_incompat;
> > +		libxfs_sb_to_disk(iocur_top->data, &tsb);
> > +		write_cur();
> > +	}
> > +fail:
> > +	dbprintf(
> > +_("Failed to upgrade V5 filesystem at AG %d, please run xfs_repair.\n"), agno);
> > +	return false;
> > +}
> > +
> >  static char *
> >  version_string(
> >  	xfs_sb_t	*sbp)
> > @@ -691,15 +816,12 @@ version_string(
> >  		strcat(s, ",INOBTCNT");
> >  	if (xfs_sb_version_hasbigtime(sbp))
> >  		strcat(s, ",BIGTIME");
> > +	if (xfs_sb_version_needsrepair(sbp))
> > +		strcat(s, ",NEEDSREPAIR");
> >  	return s;
> >  }
> >  
> > -/*
> > - * XXX: this only supports reading and writing to version 4 superblock fields.
> > - * V5 superblocks always define certain V4 feature bits - they are blocked from
> > - * being changed if a V5 sb is detected, but otherwise v5 superblock features
> > - * are not handled here.
> > - */
> > +/* Upgrade a superblock to support a feature. */
> >  static int
> >  version_f(
> >  	int		argc,
> > @@ -710,6 +832,9 @@ version_f(
> >  	xfs_agnumber_t	ag;
> >  
> >  	if (argc == 2) {	/* WRITE VERSION */
> > +		struct v5feat	v5features;
> > +
> > +		get_v5_features(mp, &v5features);
> >  
> >  		if ((x.isreadonly & LIBXFS_ISREADONLY) || !expert_mode) {
> >  			dbprintf(_("%s: not in expert mode, writing disabled\n"),
> > @@ -717,8 +842,23 @@ version_f(
> >  			return 0;
> >  		}
> >  
> > +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
> > +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
> > +				progname);
> > +			return 0;
> > +		}
> 
> I guess this enforces a repair after each version change, which makes sense.
> 
> > +
> >  		/* Logic here derived from the IRIX xfs_chver(1M) script. */
> > -		if (!strcasecmp(argv[1], "extflg")) {
> > +		if (!strcasecmp(argv[1], "needsrepair")) {
> > +			if (!xfs_sb_version_hascrc(&mp->m_sb)) {
> > +				dbprintf(
> > +		_("needsrepair flag cannot be enabled on pre-V5 filesystems\n"));
> > +				exitcode = 1;
> > +				return 1;
> > +			}
> > +
> > +			v5features.incompat |= XFS_SB_FEAT_INCOMPAT_NEEDSREPAIR;
> > +		} else if (!strcasecmp(argv[1], "extflg")) {
> >  			switch (XFS_SB_VERSION_NUM(&mp->m_sb)) {
> >  			case XFS_SB_VERSION_1:
> >  				version = 0x0004 | XFS_SB_VERSION_EXTFLGBIT;
> > @@ -809,6 +949,11 @@ version_f(
> >  			mp->m_sb.sb_versionnum = version;
> >  			mp->m_sb.sb_features2 = features;
> >  		}
> > +
> > +		if (!set_v5_features(mp, &v5features)) {
> > +			exitcode = 1;
> > +			return 1;
> > +		}
> >  	}
> >  
> >  	if (argc == 3) {	/* VERSIONNUM + FEATURES2 */
> > diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
> > index bd325da2f776..41a14d4521ba 100755
> > --- a/db/xfs_admin.sh
> > +++ b/db/xfs_admin.sh
> > @@ -9,7 +9,7 @@ DB_OPTS=""
> >  REPAIR_OPTS=""
> >  USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
> >  
> > -while getopts "efjlpuc:L:U:V" c
> > +while getopts "efjlpuc:L:O:U:V" c
> >  do
> >  	case $c in
> >  	c)	REPAIR_OPTS=$REPAIR_OPTS" -c lazycount="$OPTARG;;
> > @@ -19,6 +19,9 @@ do
> >  	l)	DB_OPTS=$DB_OPTS" -r -c label";;
> >  	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
> >  	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
> > +	O)	DB_OPTS=$DB_OPTS" -c 'version "$OPTARG"'";
> > +		# Force repair to run by adding a single space to REPAIR_OPTS
> > +		REPAIR_OPTS="$REPAIR_OPTS ";;
> >  	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
> >  	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
> >  	V)	xfs_db -p xfs_admin -V
> > @@ -48,6 +51,7 @@ case $# in
> >  		fi
> >  		if [ -n "$REPAIR_OPTS" ]
> >  		then
> > +			echo "Running xfs_repair to ensure filesystem consistency."
> >  			# Hide normal repair output which is sent to stderr
> >  			# assuming the filesystem is fine when a user is
> >  			# running xfs_admin.
> > diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
> > index 8afc873fb50a..b423981d8049 100644
> > --- a/man/man8/xfs_admin.8
> > +++ b/man/man8/xfs_admin.8
> > @@ -6,6 +6,8 @@ xfs_admin \- change parameters of an XFS filesystem
> >  [
> >  .B \-eflpu
> >  ] [
> > +.BI \-O " feature"
> > +] [
> >  .BR "\-c 0" | 1
> >  ] [
> >  .B \-L
> > @@ -103,6 +105,19 @@ The filesystem label can be cleared using the special "\c
> >  " value for
> >  .IR label .
> >  .TP
> > +.BI \-O " feature"
> > +Add a new feature to the filesystem.
> > +Only one feature can be specified at a time.
> > +Features are as follows:
> > +.RS 0.7i
> > +.TP
> > +.B needsrepair
> > +If this is a V5 filesystem, flag the filesystem as needing repairs.
> > +Until
> > +.BR xfs_repair (8)
> > +is run, the filesystem will not be mountable.
> > +.RE
> > +.TP
> 
> At some point I would like to add "projid32bit" and "extflg" and "log2" etc
> to the "-O" documentation, and maybe deprecate the old feature-specific
> options, since "-O projid32bit" et al come for free now.

<Nod>

--D

> >  .BI \-U " uuid"
> >  Set the UUID of the filesystem to
> >  .IR uuid .
> > diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
> > index 587274957de3..7331cf196bb0 100644
> > --- a/man/man8/xfs_db.8
> > +++ b/man/man8/xfs_db.8
> > @@ -971,6 +971,11 @@ may toggle between
> >  and
> >  .B attr2
> >  at will (older kernels may not support the newer version).
> > +The filesystem can be flagged as requiring a run through
> > +.BR xfs_repair (8)
> > +if the
> > +.B needsrepair
> > +option is specified and the filesystem is formatted with the V5 format.
> >  .IP
> >  If no argument is given, the current version and feature bits are printed.
> >  With one argument, this command will write the updated version number
> > 

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

[Darrick J. Wong]

On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote:
> On 11/30/20 9:37 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_mount.c before we start changing sb_inprogress.
> > This also reduces the diff between kernel and userspace libxfs.
> 
> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
> at all, and nobody reading the code or comments will know why we've chosen
> to move just these two checks out of the common validator...
> 
> What does "compatible with this mount" mean?

Compatible with this implementation?

> Maybe just fess up in the comment, and say "these checks are different 
> for kernel vs. userspace so we keep them over here" - and as for the
> function name, *shrug* not sure I have anything better...

_validate_implementation?  I don't have a better suggestion either.

--D

> -Eric
> 

Re: [PATCH 4/3] xfs_db: support the needsrepair feature flag in the version command

[Eric Sandeen]

On 12/4/20 3:09 PM, Darrick J. Wong wrote:
>>> @@ -543,6 +549,12 @@ label_f(
>>>  			return 0;
>>>  		}
>>>  
>>> +		if (xfs_sb_version_needsrepair(&mp->m_sb)) {
>>> +			dbprintf(_("%s: filesystem needs xfs_repair\n"),
>>> +				progname);
>>> +			return 0;
>>> +		}
>>> +
>> why are uuid_f and label_f uniquely disallowed from operating on a needsrepair
>> filesystem?  (is it because they are unique in that they rewrite all superblocks?)
> Because ... the fs needs repair, so the admin should repair the fs
> before they try to modify the fs.  We don't allow mounting, so we
> shouldn't allow label/uuid changes.

But xfs_db can change LOTS more than just these two things.

So this seems a bit ad-hoc and something that will drift as time
goes by.

So what is the "can't change" criteria here - only xfs_admin-invoked routines?

If so, could we put this in a central place, and reject if
!(strcmp(progname, "xfs_admin")) but let the xfs_db power user proceed?

Mostly I prefer to get this check centralized and not sprinkled around.

-Eric

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 1/3] xfs: move kernel-specific superblock validation out of libxfs

[Eric Sandeen]

On 12/4/20 3:12 PM, Darrick J. Wong wrote:
> On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote:
>> On 11/30/20 9:37 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_mount.c before we start changing sb_inprogress.

oh also, you're not changing sb_inprogress anymore, right? ;)

>>> This also reduces the diff between kernel and userspace libxfs.
>>
>> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
>> at all, and nobody reading the code or comments will know why we've chosen
>> to move just these two checks out of the common validator...
>>
>> What does "compatible with this mount" mean?
> 
> Compatible with this implementation?

Hm.

So most of xfs_validate_sb_common is doing internal consistency checking
that has nothing at all to do with the host's core capabilities or filesystem
"state" (other than version/features I guess).

You've moved out the PAGE_SIZE check, which depends on the host.

You've also moved the inprogress check, which depends on state.
(and that's not really "kernel-specific" is it?)

You'll later move the NEEDSREPAIR check, which I guess is state.

But you haven't moved the fsb_count-vs-host check, which depends on the host.

(and ... I think that one may actually be kernel-specific,
because it depends on pagecache limitations in the kernel, so maybe it
should be moved out as well?)

So maybe the distinction is internal consistency checks, vs
host-compatibility-and-filesystem-state checks.

How about ultimately:

/*
 * Do host compatibility and filesystem state checks here; these are unique
 * to the kernel, and may differ in userspace.
 */
xfs_validate_sb_host(
	struct xfs_mount	*mp,
	struct xfs_buf		*bp,
	struct xfs_sb		*sbp)
{
	/*
	 * 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;
	}

	/* 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;
	}

	/*
	 * 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;
	}

	/* Ensure this filesystem fits in the page cache limits */
        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;
        }

	return 0;
}

>> Maybe just fess up in the comment, and say "these checks are different 
>> for kernel vs. userspace so we keep them over here" - and as for the
>> function name, *shrug* not sure I have anything better...
> 
> _validate_implementation?  I don't have a better suggestion either.
> 
> --D
> 
>> -Eric
>>
> 

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

[Darrick J. Wong]

On Fri, Dec 04, 2020 at 03:46:19PM -0600, Eric Sandeen wrote:
> On 12/4/20 3:12 PM, Darrick J. Wong wrote:
> > On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote:
> >> On 11/30/20 9:37 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_mount.c before we start changing sb_inprogress.
> 
> oh also, you're not changing sb_inprogress anymore, right? ;)

Fixed.

> >>> This also reduces the diff between kernel and userspace libxfs.
> >>
> >> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
> >> at all, and nobody reading the code or comments will know why we've chosen
> >> to move just these two checks out of the common validator...
> >>
> >> What does "compatible with this mount" mean?
> > 
> > Compatible with this implementation?
> 
> Hm.
> 
> So most of xfs_validate_sb_common is doing internal consistency checking
> that has nothing at all to do with the host's core capabilities or filesystem
> "state" (other than version/features I guess).
> 
> You've moved out the PAGE_SIZE check, which depends on the host.
> 
> You've also moved the inprogress check, which depends on state.
> (and that's not really "kernel-specific" is it?)
> 
> You'll later move the NEEDSREPAIR check, which I guess is state.
> 
> But you haven't moved the fsb_count-vs-host check, which depends on the host.
> 
> (and ... I think that one may actually be kernel-specific,
> because it depends on pagecache limitations in the kernel, so maybe it
> should be moved out as well?)

Aha, yes, I missed that.

> So maybe the distinction is internal consistency checks, vs
> host-compatibility-and-filesystem-state checks.
> 
> How about ultimately:
> 
> /*
>  * Do host compatibility and filesystem state checks here; these are unique
>  * to the kernel, and may differ in userspace.
>  */
> xfs_validate_sb_host(
> 	struct xfs_mount	*mp,
> 	struct xfs_buf		*bp,
> 	struct xfs_sb		*sbp)
> {
> 	/*
> 	 * 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;
> 	}
> 
> 	/* 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;
> 	}
> 
> 	/*
> 	 * 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;
> 	}
> 
> 	/* Ensure this filesystem fits in the page cache limits */
>         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;

Sounds good to me.

--D

>         }
> 
> 	return 0;
> }
> 
> >> Maybe just fess up in the comment, and say "these checks are different 
> >> for kernel vs. userspace so we keep them over here" - and as for the
> >> function name, *shrug* not sure I have anything better...
> > 
> > _validate_implementation?  I don't have a better suggestion either.
> > 
> > --D
> > 
> >> -Eric
> >>
> > 

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

[Dave Chinner]

On Fri, Dec 04, 2020 at 03:46:19PM -0600, Eric Sandeen wrote:
> On 12/4/20 3:12 PM, Darrick J. Wong wrote:
> > On Fri, Dec 04, 2020 at 02:35:45PM -0600, Eric Sandeen wrote:
> >> On 11/30/20 9:37 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_mount.c before we start changing sb_inprogress.
> 
> oh also, you're not changing sb_inprogress anymore, right? ;)
> 
> >>> This also reduces the diff between kernel and userspace libxfs.
> >>
> >> My only complaint is that "xfs_sb_validate_mount" isn't really descriptive
> >> at all, and nobody reading the code or comments will know why we've chosen
> >> to move just these two checks out of the common validator...
> >>
> >> What does "compatible with this mount" mean?
> > 
> > Compatible with this implementation?
> 
> Hm.
> 
> So most of xfs_validate_sb_common is doing internal consistency checking
> that has nothing at all to do with the host's core capabilities or filesystem
> "state" (other than version/features I guess).
> 
> You've moved out the PAGE_SIZE check, which depends on the host.
> 
> You've also moved the inprogress check, which depends on state.
> (and that's not really "kernel-specific" is it?)
> 
> You'll later move the NEEDSREPAIR check, which I guess is state.
> 
> But you haven't moved the fsb_count-vs-host check, which depends on the host.
> 
> (and ... I think that one may actually be kernel-specific,
> because it depends on pagecache limitations in the kernel, so maybe it
> should be moved out as well?)
> 
> So maybe the distinction is internal consistency checks, vs
> host-compatibility-and-filesystem-state checks.
> 
> How about ultimately:
> 
> /*
>  * Do host compatibility and filesystem state checks here; these are unique
>  * to the kernel, and may differ in userspace.
>  */
> xfs_validate_sb_host(
> 	struct xfs_mount	*mp,
> 	struct xfs_buf		*bp,
> 	struct xfs_sb		*sbp)
> {

THis host stuff should be checked in xfs_fs_fill_super(), right?

i.e. it's not really part of the superblock validation, but checking
if the host can actually run this filesystem. That's what we do in
xfs_fc_fill_super(), such as the max file size support, whether
mount options are valid for the superblock config (e.g. reflink vs
rt) and so on. IOWs, these aren't corruption verifiers, just config
checks, so we should put them with all the other config checks we do
at mount time...

i.e. call it something like xfs_fs_validate_sb_config() and move all
the config and experimental function checks into it, call it only
from xfs_fs_fill_super() once we've already read in and validated
the superblock is within the bounds defined by the on-disk format.

Oh, and just another thing: can we rename all the "xfs_fc_*"
functions in xfs_super.c back to xfs_fs_*? I'm getting tired of not
being about to find the superblock init functions in xfs_super.c
(e.g. xfs_fs_fill_super()) with grep or cscope because they have a
whacky, one-off namespace now...

Cheers,

Dave.


-- 
Dave Chinner
david@fromorbit.com