[PATCHSET v2 0/2] xfs: fix ro mounting with unknown rocompat features

[PATCHSET v2 0/2] xfs: fix ro mounting with unknown rocompat features

[Darrick J. Wong]

Hi all,

Dave pointed out some failures in xfs/270 when he upgraded Debian
unstable and util-linux started using the new mount apis.  Upon further
inquiry I noticed that XFS is quite a hot mess when it encounters a
filesystem with unrecognized rocompat bits set in the superblock.

Whereas we used to allow readonly mounts under these conditions, a
change to the sb write verifier several years ago resulted in the
filesystem going down immediately because the post-mount log cleaning
writes the superblock, which trips the sb write verifier on the
unrecognized rocompat bit.  I made the observation that the ROCOMPAT
features RMAPBT and REFLINK both protect new log intent item types,
which means that we actually cannot support recovering the log if we
don't recognize all the rocompat bits.

Therefore -- fix inode inactivation to work when we're recovering the
log, disallow recovery when there's unrecognized rocompat bits, and
don't clean the log if doing so would trip the rocompat checks.

v2: change direction of series to allow log recovery on ro mounts

If you're going to start using this code, I strongly recommend pulling
from my git trees, which are linked below.

This has been lightly tested with fstests.  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=fix-ro-mounts-6.6
---
 fs/xfs/libxfs/xfs_sb.c |    3 ++-
 fs/xfs/xfs_inode.c     |   14 ++++++++++----
 fs/xfs/xfs_log.c       |   17 -----------------
 3 files changed, 12 insertions(+), 22 deletions(-)

[PATCH 1/2] xfs: allow inode inactivation during a ro mount log recovery

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

In the next patch, we're going to prohibit log recovery if the primary
superblock contains an unrecognized rocompat feature bit even on
readonly mounts.  This requires removing all the code in the log
mounting process that temporarily disables the readonly state.

Unfortunately, inode inactivation disables itself on readonly mounts.
Clearing the iunlinked lists after log recovery needs inactivation to
run to free the unreferenced inodes, which (AFAICT) is the only reason
why log mounting plays games with the readonly state in the first place.

Therefore, change the inactivation predicates to allow inactivation
during log recovery of a readonly mount.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_inode.c |   14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 9e62cc500140..6ee266be45d4 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -1643,8 +1643,11 @@ xfs_inode_needs_inactive(
 	if (VFS_I(ip)->i_mode == 0)
 		return false;
 
-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (xfs_is_readonly(mp))
+	/*
+	 * If this is a read-only mount, don't do this (would generate I/O)
+	 * unless we're in log recovery and cleaning the iunlinked list.
+	 */
+	if (xfs_is_readonly(mp) && !xlog_recovery_needed(mp->m_log))
 		return false;
 
 	/* If the log isn't running, push inodes straight to reclaim. */
@@ -1704,8 +1707,11 @@ xfs_inactive(
 	mp = ip->i_mount;
 	ASSERT(!xfs_iflags_test(ip, XFS_IRECOVERY));
 
-	/* If this is a read-only mount, don't do this (would generate I/O) */
-	if (xfs_is_readonly(mp))
+	/*
+	 * If this is a read-only mount, don't do this (would generate I/O)
+	 * unless we're in log recovery and cleaning the iunlinked list.
+	 */
+	if (xfs_is_readonly(mp) && !xlog_recovery_needed(mp->m_log))
 		goto out;
 
 	/* Metadata inodes require explicit resource cleanup. */

[PATCH 2/2] xfs: fix log recovery when unknown rocompat bits are set

[Darrick J. Wong]

From: Darrick J. Wong <djwong@kernel.org>

Fix log recovery to proceed on a readonly mount if the primary sb
advertises unknown rocompat bits.  We used to allow this, but due to a
misunderstanding between Eric and Darrick back in 2018, we accidentally
changed the superblock write verifier to shutdown the fs over that exact
scenario.  As a result, the log cleaning that occurs at the end of the
mounting process fails.

We now allow writing of the superblock if there are unknown rocompat
bits set, but only if the filesystem is read only.  Hence we still have
to remove all ro state toggling that occurs around the log mount calls.

Fixes: 9e037cb7972f ("xfs: check for unknown v5 feature bits in superblock write verifier"
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_sb.c |    3 ++-
 fs/xfs/xfs_log.c       |   17 -----------------
 2 files changed, 2 insertions(+), 18 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index 5e174685a77c..6264daaab37b 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -266,7 +266,8 @@ xfs_validate_sb_write(
 		return -EFSCORRUPTED;
 	}
 
-	if (xfs_sb_has_ro_compat_feature(sbp, XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
+	if (!xfs_is_readonly(mp) &&
+	    xfs_sb_has_ro_compat_feature(sbp, XFS_SB_FEAT_RO_COMPAT_UNKNOWN)) {
 		xfs_alert(mp,
 "Corruption detected in superblock read-only compatible features (0x%x)!",
 			(sbp->sb_features_ro_compat &
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 79004d193e54..51c100c86177 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -715,15 +715,7 @@ xfs_log_mount(
 	 * just worked.
 	 */
 	if (!xfs_has_norecovery(mp)) {
-		/*
-		 * log recovery ignores readonly state and so we need to clear
-		 * mount-based read only state so it can write to disk.
-		 */
-		bool	readonly = test_and_clear_bit(XFS_OPSTATE_READONLY,
-						&mp->m_opstate);
 		error = xlog_recover(log);
-		if (readonly)
-			set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
 		if (error) {
 			xfs_warn(mp, "log mount/recovery failed: error %d",
 				error);
@@ -772,7 +764,6 @@ xfs_log_mount_finish(
 	struct xfs_mount	*mp)
 {
 	struct xlog		*log = mp->m_log;
-	bool			readonly;
 	int			error = 0;
 
 	if (xfs_has_norecovery(mp)) {
@@ -780,12 +771,6 @@ xfs_log_mount_finish(
 		return 0;
 	}
 
-	/*
-	 * log recovery ignores readonly state and so we need to clear
-	 * mount-based read only state so it can write to disk.
-	 */
-	readonly = test_and_clear_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
-
 	/*
 	 * During the second phase of log recovery, we need iget and
 	 * iput to behave like they do for an active filesystem.
@@ -835,8 +820,6 @@ xfs_log_mount_finish(
 	xfs_buftarg_drain(mp->m_ddev_targp);
 
 	clear_bit(XLOG_RECOVERY_NEEDED, &log->l_opstate);
-	if (readonly)
-		set_bit(XFS_OPSTATE_READONLY, &mp->m_opstate);
 
 	/* Make sure the log is dead if we're returning failure. */
 	ASSERT(!error || xlog_is_shutdown(log));

Re: [PATCH 2/2] xfs: fix log recovery when unknown rocompat bits are set

[Dave Chinner]

On Tue, Aug 29, 2023 at 04:09:35PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Fix log recovery to proceed on a readonly mount if the primary sb
> advertises unknown rocompat bits.  We used to allow this, but due to a
> misunderstanding between Eric and Darrick back in 2018, we accidentally
> changed the superblock write verifier to shutdown the fs over that exact
> scenario.  As a result, the log cleaning that occurs at the end of the
> mounting process fails.
> 
> We now allow writing of the superblock if there are unknown rocompat
> bits set, but only if the filesystem is read only.  Hence we still have
> to remove all ro state toggling that occurs around the log mount calls.

APart from the wording of the commit message, this all looks good.
It took me a little while to parse what it meant paragraph meant,
so can I suggest something like:

	Log recovery has always run on read only mounts, even where the
	primary superblock advertises unknown rocompat bits. Due to a
	misunderstanding between Eric and Darrick back in 2018, we accidentally
	changed the superblock write verifier to shutdown the fs over that exact
	scenario.  As a result, the log cleaning that occurs at the end of the
	mounting process fails if there are unknown rocompat bits set.

	As we now allow writing of the superblock if there are unknown
	rocompat bits set on a RO mount, we no longer want to turn off RO
	state to allow log recovery to succeed on a RO mount.  Hence we also
	remove all the (now unnecessary) RO state toggling from the log
	recovery path.

Other than that,

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/2] xfs: allow inode inactivation during a ro mount log recovery

[Dave Chinner]

On Tue, Aug 29, 2023 at 04:09:29PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> In the next patch, we're going to prohibit log recovery if the primary
> superblock contains an unrecognized rocompat feature bit even on
> readonly mounts.  This requires removing all the code in the log
> mounting process that temporarily disables the readonly state.
> 
> Unfortunately, inode inactivation disables itself on readonly mounts.
> Clearing the iunlinked lists after log recovery needs inactivation to
> run to free the unreferenced inodes, which (AFAICT) is the only reason
> why log mounting plays games with the readonly state in the first place.
> 
> Therefore, change the inactivation predicates to allow inactivation
> during log recovery of a readonly mount.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_inode.c |   14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)

Looks good.

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/2] xfs: fix log recovery when unknown rocompat bits are set

[Darrick J. Wong]

On Wed, Aug 30, 2023 at 10:31:59AM +1000, Dave Chinner wrote:
> On Tue, Aug 29, 2023 at 04:09:35PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Fix log recovery to proceed on a readonly mount if the primary sb
> > advertises unknown rocompat bits.  We used to allow this, but due to a
> > misunderstanding between Eric and Darrick back in 2018, we accidentally
> > changed the superblock write verifier to shutdown the fs over that exact
> > scenario.  As a result, the log cleaning that occurs at the end of the
> > mounting process fails.
> > 
> > We now allow writing of the superblock if there are unknown rocompat
> > bits set, but only if the filesystem is read only.  Hence we still have
> > to remove all ro state toggling that occurs around the log mount calls.
> 
> APart from the wording of the commit message, this all looks good.
> It took me a little while to parse what it meant paragraph meant,
> so can I suggest something like:
> 
> 	Log recovery has always run on read only mounts, even where the
> 	primary superblock advertises unknown rocompat bits. Due to a
> 	misunderstanding between Eric and Darrick back in 2018, we accidentally
> 	changed the superblock write verifier to shutdown the fs over that exact
> 	scenario.  As a result, the log cleaning that occurs at the end of the
> 	mounting process fails if there are unknown rocompat bits set.
> 
> 	As we now allow writing of the superblock if there are unknown
> 	rocompat bits set on a RO mount, we no longer want to turn off RO
> 	state to allow log recovery to succeed on a RO mount.  Hence we also
> 	remove all the (now unnecessary) RO state toggling from the log
> 	recovery path.

Ok, I've copied that in verbatim.  Thanks for reviewing these!

--D

> Other than that,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> -- 
> Dave Chinner
> david@fromorbit.com