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

[PATCHSET 0/3] 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.

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/xfs_inode.c       |   14 ++++++++++----
 fs/xfs/xfs_log.c         |   23 ++++++-----------------
 fs/xfs/xfs_log_priv.h    |    7 +++++++
 fs/xfs/xfs_log_recover.c |   32 ++++++++++++++++++++++++++++++++
 4 files changed, 55 insertions(+), 21 deletions(-)

[PATCH 1/3] 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/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Darrick J. Wong]

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

Don't allow log recovery to proceed on a readonly mount if the primary
superblock advertises unknown rocompat bits.  We used to allow this, but
due to a misunderstanding between Dave and Darrick back in 2016, we
cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
log items, which is why we can't allow older kernels to recover them.

Fixes: b87049444ac4 ("xfs: introduce rmap btree definitions")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log.c         |   17 -----------------
 fs/xfs/xfs_log_recover.c |   25 +++++++++++++++++++++++++
 2 files changed, 25 insertions(+), 17 deletions(-)


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));
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 82c81d20459d..b4458b7fd6f7 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3354,6 +3354,7 @@ xlog_recover(
 	struct xlog	*log)
 {
 	xfs_daddr_t	head_blk, tail_blk;
+	bool		unknown_rocompat = false;
 	int		error;
 
 	/* find the tail of the log */
@@ -3370,6 +3371,12 @@ xlog_recover(
 	    !xfs_log_check_lsn(log->l_mp, log->l_mp->m_sb.sb_lsn))
 		return -EINVAL;
 
+	/* Detect unknown rocompat features in the superblock */
+	if (xfs_has_crc(log->l_mp) &&
+	    xfs_sb_has_ro_compat_feature(&log->l_mp->m_sb,
+					 XFS_SB_FEAT_RO_COMPAT_UNKNOWN))
+		unknown_rocompat = true;
+
 	if (tail_blk != head_blk) {
 		/* There used to be a comment here:
 		 *
@@ -3407,6 +3414,24 @@ xlog_recover(
 			return -EINVAL;
 		}
 
+		/*
+		 * Don't allow log recovery on a ro mount if there are unknown
+		 * ro compat bits set.  We used to allow this, but BUI/CUI log
+		 * items are protected by the REFLINK rocompat bit so now we
+		 * cannot.
+		 */
+		if (xfs_is_readonly(log->l_mp) && unknown_rocompat) {
+			xfs_alert(log->l_mp,
+"Superblock has unknown read-only compatible features (0x%x) enabled.",
+				 (log->l_mp->m_sb.sb_features_ro_compat &
+						XFS_SB_FEAT_RO_COMPAT_UNKNOWN));
+			xfs_warn(log->l_mp,
+"The log can not be fully and/or safely recovered by this kernel.");
+			xfs_warn(log->l_mp,
+"Please recover the log on a kernel that supports the unknown features.");
+			return -EINVAL;
+		}
+
 		/*
 		 * Delay log recovery if the debug hook is set. This is debug
 		 * instrumentation to coordinate simulation of I/O failures with

[PATCH 3/3] xfs: log is not writable if we have unknown rocompat features

[Darrick J. Wong]

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

Ever since commit 9e037cb7972f, the superblock write verifier will trip
if someone tries to write a superblock with unknown rocompat features.
However, we allow ro mounts of a filesystem with unknown rocompat
features if the log is clean, except that has been broken for years
because the end of an ro mount cleans the log, which logs and writes the
superblock.

Therefore, don't allow log writes to happen if there are unknown
rocompat features set.

Fixes: 9e037cb7972f ("xfs: check for unknown v5 feature bits in superblock write verifier")
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_log.c         |    6 ++++++
 fs/xfs/xfs_log_priv.h    |    7 +++++++
 fs/xfs/xfs_log_recover.c |    7 +++++++
 3 files changed, 20 insertions(+)


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 51c100c86177..c1bbc8040bcb 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -391,6 +391,8 @@ xfs_log_writable(
 		return false;
 	if (xlog_is_shutdown(mp->m_log))
 		return false;
+	if (xlog_is_readonly(mp->m_log))
+		return false;
 	return true;
 }
 
@@ -408,6 +410,8 @@ xfs_log_regrant(
 
 	if (xlog_is_shutdown(log))
 		return -EIO;
+	if (xlog_is_readonly(log))
+		return -EROFS;
 
 	XFS_STATS_INC(mp, xs_try_logspace);
 
@@ -471,6 +475,8 @@ xfs_log_reserve(
 
 	if (xlog_is_shutdown(log))
 		return -EIO;
+	if (xlog_is_readonly(log))
+		return -EROFS;
 
 	XFS_STATS_INC(mp, xs_try_logspace);
 
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index af87648331d5..14892e01de38 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -461,6 +461,7 @@ struct xlog {
 #define XLOG_IO_ERROR		2	/* log hit an I/O error, and being
 				   shutdown */
 #define XLOG_TAIL_WARN		3	/* log tail verify warning issued */
+#define XLOG_READONLY		4	/* cannot write to the log */
 
 static inline bool
 xlog_recovery_needed(struct xlog *log)
@@ -480,6 +481,12 @@ xlog_is_shutdown(struct xlog *log)
 	return test_bit(XLOG_IO_ERROR, &log->l_opstate);
 }
 
+static inline bool
+xlog_is_readonly(struct xlog *log)
+{
+	return test_bit(XLOG_READONLY, &log->l_opstate);
+}
+
 /*
  * Wait until the xlog_force_shutdown() has marked the log as shut down
  * so xlog_is_shutdown() will always return true.
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index b4458b7fd6f7..f8f13d5f79cd 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3450,6 +3450,13 @@ xlog_recover(
 
 		error = xlog_do_recover(log, head_blk, tail_blk);
 		set_bit(XLOG_RECOVERY_NEEDED, &log->l_opstate);
+	} else if (unknown_rocompat) {
+		/*
+		 * Log recovery wasn't needed, but if the superblock has
+		 * unknown rocompat features, don't allow log writes at all
+		 * because the sb write verifier will trip.
+		 */
+		set_bit(XLOG_READONLY, &log->l_opstate);
 	}
 	return error;
 }

[PATCH 4/3] xfs/270: actually test file readability

[Darrick J. Wong]

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

Make sure we can actually read files off the ro mounted filesystem that
has an unknown rocompat feature set.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/270 |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/xfs/270 b/tests/xfs/270
index 7447ce87be..511dfe9fcd 100755
--- a/tests/xfs/270
+++ b/tests/xfs/270
@@ -23,6 +23,9 @@ _require_scratch_nocheck
 _require_scratch_xfs_crc
 
 _scratch_mkfs_xfs >>$seqres.full 2>&1
+_scratch_mount
+echo moo > $SCRATCH_MNT/testfile
+_scratch_unmount
 
 # set the highest bit of features_ro_compat, use it as an unknown
 # feature bit. If one day this bit become known feature, please
@@ -68,6 +71,7 @@ if [ $? -ne 0 ]; then
 	_fail "ro mount test failed"
 else
 	# no hang/panic is fine
+	cat $SCRATCH_MNT/testfile > /dev/null
 	$FSSTRESS_PROG -d $SCRATCH_MNT -p 4 -n 400 >>$seqres.full 2>&1
 fi
 

[PATCH 5/3] xfs/270: actually test log recovery with unknown rocompat features

[Darrick J. Wong]

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

Make sure that log recovery will not succeed if there are unknown
rocompat features in the superblock and the log is dirty.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 tests/xfs/270     |   81 ++++++++++++++++++++++++++++++++++++-----------------
 tests/xfs/270.out |    2 +
 2 files changed, 57 insertions(+), 26 deletions(-)

diff --git a/tests/xfs/270 b/tests/xfs/270
index 511dfe9fcd..ee925b0fc6 100755
--- a/tests/xfs/270
+++ b/tests/xfs/270
@@ -21,41 +21,48 @@ _supported_fs xfs
 _require_scratch_nocheck
 # Only V5 XFS disallow rw mount/remount with unknown ro-compat features
 _require_scratch_xfs_crc
-
-_scratch_mkfs_xfs >>$seqres.full 2>&1
-_scratch_mount
-echo moo > $SCRATCH_MNT/testfile
-_scratch_unmount
+_require_scratch_shutdown
 
 # set the highest bit of features_ro_compat, use it as an unknown
 # feature bit. If one day this bit become known feature, please
 # change this case.
+set_bad_rocompat() {
+	ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
+	echo $ro_compat | grep -q -E '^0x[[:xdigit:]]$'
+	if [[ $? != 0  ]]; then
+		echo "features_ro_compat has an invalid value."
+		return 1
+	fi
 
-ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0")
-echo $ro_compat | grep -q -E '^0x[[:xdigit:]]$'
-if [[ $? != 0  ]]; then
-	echo "features_ro_compat has an invalid value."
-fi
+	ro_compat=$(echo $ro_compat | \
+			    awk '/^0x[[:xdigit:]]+/ {
+					printf("0x%x\n", or(strtonum($1), 0x80000000))
+				}')
 
-ro_compat=$(echo $ro_compat | \
-		    awk '/^0x[[:xdigit:]]+/ {
-				printf("0x%x\n", or(strtonum($1), 0x80000000))
-			}')
+	# write the new ro compat field to the superblock
+	_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
+					> $seqres.full 2>&1
 
-# write the new ro compat field to the superblock
-_scratch_xfs_set_metadata_field "features_ro_compat" "$ro_compat" "sb 0" \
-				> $seqres.full 2>&1
+	# read the newly set ro compat filed for verification
+	new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
+							2>/dev/null)
 
-# read the newly set ro compat filed for verification
-new_ro_compat=$(_scratch_xfs_get_metadata_field "features_ro_compat" "sb 0" \
-						2>/dev/null)
+	# verify the new ro_compat field is correct. Without xfsprogs commit
+	# f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()"),
+	# we can't get new_ro_compat value.
+	if [ "$new_ro_compat" != "$ro_compat" ]; then
+		echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
+		return 1
+	fi
+	return 0
+}
 
-# verify the new ro_compat field is correct. Without xfsprogs commit
-# f4afdcb0ad ("xfs_db: clean up the salvage read callsites in set_cur()"),
-# we can't get new_ro_compat value.
-if [ "$new_ro_compat" != "$ro_compat" ]; then
-	echo "Unable to set new features_ro_compat. Wanted $ro_compat, got $new_ro_compat"
-fi
+# Once with a clean filesystem...
+_scratch_mkfs_xfs >>$seqres.full 2>&1
+_scratch_mount
+echo moo > $SCRATCH_MNT/testfile
+_scratch_unmount
+set_bad_rocompat
 
 # rw mount with unknown ro-compat feature should fail
 echo "rw mount test"
@@ -85,6 +92,28 @@ fi
 
 _scratch_unmount
 
+# And again with a dirty filesystem...
+_scratch_mkfs_xfs >>$seqres.full 2>&1
+_scratch_mount
+echo moo > $SCRATCH_MNT/testfile
+$XFS_IO_PROG -x -c 'shutdown -f' "${SCRATCH_MNT}"
+_scratch_unmount
+set_bad_rocompat
+
+# rw mount with unknown ro-compat feature should fail
+echo "rw mount test"
+_try_scratch_mount 2>>$seqres.full
+if [ $? -eq 0 ]; then
+	_fail "rw mount test failed"
+fi
+
+# ro mount should not succeed due to log recovery
+echo "ro mount test"
+_try_scratch_mount -o ro 2>>$seqres.full
+if [ $? -eq 0 ]; then
+	_fail "ro mount test succeeded"
+fi
+
 # success, all done
 status=0
 exit
diff --git a/tests/xfs/270.out b/tests/xfs/270.out
index edf4c25489..a519d2f328 100644
--- a/tests/xfs/270.out
+++ b/tests/xfs/270.out
@@ -2,3 +2,5 @@ QA output created by 270
 rw mount test
 ro mount test
 rw remount test
+rw mount test
+ro mount test

Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Dave Chinner]

On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Don't allow log recovery to proceed on a readonly mount if the primary
> superblock advertises unknown rocompat bits.  We used to allow this, but
> due to a misunderstanding between Dave and Darrick back in 2016, we
> cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
> protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
> log items, which is why we can't allow older kernels to recover them.

Ok, this would work for kernels that don't know waht the
REFLINK/RMAP features are, but upstream kernels will never fail to
recover these items because these are known ROCOMPAT bits.

The reason this problem exists is that we've accidentally
conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had
of set a log incompat bit whenever they are used (similar to the
new ATTRI stuff setting log incompat bits), then older kernels
would not have allow log recovery even if the reflink/rmap RO_COMPAT
features were set and they didn't understand them.

However, we can't do that on current kernels because then older
kernels that understand reflink/rmap just fine would see an unknown
log incompat bit and refuse to replay the log. So it comes back to
how we handle unknown ROCOMPAT flags on older kernels, not current
upstream kernels.

i.e. this patch needs to be backported to kernels that don't know
anything about RMAP/REFLINK to be useful to anyone. i.e. kernels
older than 4.9 that don't know what rmap/reflink are.  I suspect
that there are very few supported kernels that old that this might
get backported to.

Hence I wonder if this change is necessary at all.  If we can
guarantee that anything adding a new log item type to the journal
sets a LOG_INCOMPAT flag, then we don't need to change the RO_COMPAT
handling in current kernels to avoid log recovery at all - the
existing LOG_INCOMPAT flag handling will do that for us....

Yes, we can have a new feature that is RO_COMPAT + LOG_INCOMPAT; the
reflink and rmap features should have been defined this way as
that's where we went wrong. It's too late to set LOG_INCOMPAT for
them, and so the only way to fix old supported kernels is to prevent
log recovery when unknown RO_COMPAT bits are set.

Hence I don't see this solution as necessary for any kernel recent
enough to support rmap/reflink, nor do I see it necessary to protect
against making the same mistake about RO_COMPAT features in the
future. Everyone now knows that a log format change requires
LOG_INCOMPAT, not RO_COMPAT, so we should not be making that mistake
again.....

Thoughts?

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: log is not writable if we have unknown rocompat features

[Dave Chinner]

On Thu, Aug 24, 2023 at 04:21:52PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Ever since commit 9e037cb7972f, the superblock write verifier will trip
> if someone tries to write a superblock with unknown rocompat features.
> However, we allow ro mounts of a filesystem with unknown rocompat
> features if the log is clean, except that has been broken for years
> because the end of an ro mount cleans the log, which logs and writes the
> superblock.
> 
> Therefore, don't allow log writes to happen if there are unknown
> rocompat features set.
> 
> Fixes: 9e037cb7972f ("xfs: check for unknown v5 feature bits in superblock write verifier")
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>

Yup, this makes sense.

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

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Darrick J. Wong]

On Fri, Aug 25, 2023 at 11:07:56AM +1000, Dave Chinner wrote:
> On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Don't allow log recovery to proceed on a readonly mount if the primary
> > superblock advertises unknown rocompat bits.  We used to allow this, but
> > due to a misunderstanding between Dave and Darrick back in 2016, we
> > cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
> > protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
> > log items, which is why we can't allow older kernels to recover them.
> 
> Ok, this would work for kernels that don't know waht the
> REFLINK/RMAP features are, but upstream kernels will never fail to
> recover these items because these are known ROCOMPAT bits.
> 
> The reason this problem exists is that we've accidentally
> conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had
> of set a log incompat bit whenever they are used (similar to the
> new ATTRI stuff setting log incompat bits), then older kernels
> would not have allow log recovery even if the reflink/rmap RO_COMPAT
> features were set and they didn't understand them.
> 
> However, we can't do that on current kernels because then older
> kernels that understand reflink/rmap just fine would see an unknown
> log incompat bit and refuse to replay the log. So it comes back to
> how we handle unknown ROCOMPAT flags on older kernels, not current
> upstream kernels.
> 
> i.e. this patch needs to be backported to kernels that don't know
> anything about RMAP/REFLINK to be useful to anyone. i.e. kernels
> older than 4.9 that don't know what rmap/reflink are.  I suspect
> that there are very few supported kernels that old that this might
> get backported to.
> 
> Hence I wonder if this change is necessary at all.  If we can
> guarantee that anything adding a new log item type to the journal
> sets a LOG_INCOMPAT flag, then we don't need to change the RO_COMPAT
> handling in current kernels to avoid log recovery at all - the
> existing LOG_INCOMPAT flag handling will do that for us....
> 
> Yes, we can have a new feature that is RO_COMPAT + LOG_INCOMPAT; the
> reflink and rmap features should have been defined this way as
> that's where we went wrong. It's too late to set LOG_INCOMPAT for
> them, and so the only way to fix old supported kernels is to prevent
> log recovery when unknown RO_COMPAT bits are set.
> 
> Hence I don't see this solution as necessary for any kernel recent
> enough to support rmap/reflink, nor do I see it necessary to protect
> against making the same mistake about RO_COMPAT features in the
> future. Everyone now knows that a log format change requires
> LOG_INCOMPAT, not RO_COMPAT, so we should not be making that mistake
> again.....
> 
> Thoughts?

Hmm.  The most annoying thing about LOG_INCOMPAT features is that
turning them on requires a synchronous write to the primary sb along
with a transaction to log the sb that is immediately forced to disk.
Every time the log cleans itself it clears the LOG_INCOMPAT features,
and then we have to do that /again/.

Parent pointers, since they require log intent items to guarantee the
dirent and pptr update, cycle the logged xattr LOG_INCOMPAT feature on
and off repeatedly.  A couple of weeks ago I decided to elide all that
LOG_INCOMPAT cycling if parent pointers are enabled, and fstests runtime
went from 4.9 hours back down to 4.4.  (Parent pointers, for whatever
reason, got an INCOMPAT feature bit so it's ok).  I was a little
surprised that xfs_log_clean ran that much, but there we go.

A different way to solve the cycling problem could be to start a timer
after the last caller drops l_incompat_xattrs and only clear the feature
bit after 5 minutes of idleness or unmount, instead of the next time the
log cleans itself.

Alternately, we drop this patch and declare an INCOMPAT_RMAPREFLINKV2
feature that wraps up all the other broken bits that we've found since
2016 (overly large log reservations, incorrect units in xattr block
reservation calculations, etc.)

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Darrick J. Wong]

On Thu, Aug 24, 2023 at 09:04:17PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 25, 2023 at 11:07:56AM +1000, Dave Chinner wrote:
> > On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Don't allow log recovery to proceed on a readonly mount if the primary
> > > superblock advertises unknown rocompat bits.  We used to allow this, but
> > > due to a misunderstanding between Dave and Darrick back in 2016, we
> > > cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
> > > protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
> > > log items, which is why we can't allow older kernels to recover them.
> > 
> > Ok, this would work for kernels that don't know waht the
> > REFLINK/RMAP features are, but upstream kernels will never fail to
> > recover these items because these are known ROCOMPAT bits.
> > 
> > The reason this problem exists is that we've accidentally
> > conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had
> > of set a log incompat bit whenever they are used (similar to the
> > new ATTRI stuff setting log incompat bits), then older kernels
> > would not have allow log recovery even if the reflink/rmap RO_COMPAT
> > features were set and they didn't understand them.
> > 
> > However, we can't do that on current kernels because then older
> > kernels that understand reflink/rmap just fine would see an unknown
> > log incompat bit and refuse to replay the log. So it comes back to
> > how we handle unknown ROCOMPAT flags on older kernels, not current
> > upstream kernels.
> > 
> > i.e. this patch needs to be backported to kernels that don't know
> > anything about RMAP/REFLINK to be useful to anyone. i.e. kernels
> > older than 4.9 that don't know what rmap/reflink are.  I suspect
> > that there are very few supported kernels that old that this might
> > get backported to.

Seeing as the oldest LTS kernel now is 4.14, I agree with you, let's
just forget this whole patch and try to remember not to hide LOG
INCOMPAT features behind RO COMPAT flags again. :)

If we do that, then all we need to do is change xfs_validate_sb_write
not to complain about unknown rocompat features if the xfs is readonly,
and remove all the code that temporarily clears the readonly state
around the log mount calls.

Log recovery then goes back to supporting recovery even in the presence
of unknown rocompat bits, and patch 3 becomes unnecessary...

> > Hence I wonder if this change is necessary at all.  If we can
> > guarantee that anything adding a new log item type to the journal
> > sets a LOG_INCOMPAT flag, then we don't need to change the RO_COMPAT
> > handling in current kernels to avoid log recovery at all - the
> > existing LOG_INCOMPAT flag handling will do that for us....
> > 
> > Yes, we can have a new feature that is RO_COMPAT + LOG_INCOMPAT; the
> > reflink and rmap features should have been defined this way as
> > that's where we went wrong. It's too late to set LOG_INCOMPAT for
> > them, and so the only way to fix old supported kernels is to prevent
> > log recovery when unknown RO_COMPAT bits are set.
> > 
> > Hence I don't see this solution as necessary for any kernel recent
> > enough to support rmap/reflink, nor do I see it necessary to protect
> > against making the same mistake about RO_COMPAT features in the
> > future. Everyone now knows that a log format change requires
> > LOG_INCOMPAT, not RO_COMPAT, so we should not be making that mistake
> > again.....
> > 
> > Thoughts?

...though this below is still true.

> Hmm.  The most annoying thing about LOG_INCOMPAT features is that
> turning them on requires a synchronous write to the primary sb along
> with a transaction to log the sb that is immediately forced to disk.
> Every time the log cleans itself it clears the LOG_INCOMPAT features,
> and then we have to do that /again/.
> 
> Parent pointers, since they require log intent items to guarantee the
> dirent and pptr update, cycle the logged xattr LOG_INCOMPAT feature on
> and off repeatedly.  A couple of weeks ago I decided to elide all that
> LOG_INCOMPAT cycling if parent pointers are enabled, and fstests runtime
> went from 4.9 hours back down to 4.4.  (Parent pointers, for whatever
> reason, got an INCOMPAT feature bit so it's ok).  I was a little
> surprised that xfs_log_clean ran that much, but there we go.
> 
> A different way to solve the cycling problem could be to start a timer
> after the last caller drops l_incompat_xattrs and only clear the feature
> bit after 5 minutes of idleness or unmount, instead of the next time the
> log cleans itself.
> 
> Alternately, we drop this patch and declare an INCOMPAT_RMAPREFLINKV2
> feature that wraps up all the other broken bits that we've found since
> 2016 (overly large log reservations, incorrect units in xattr block
> reservation calculations, etc.)
> 
> --D
> 
> > -Dave.
> > -- 
> > Dave Chinner
> > david@fromorbit.com

Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Dave Chinner]

On Mon, Aug 28, 2023 at 12:08:22PM -0700, Darrick J. Wong wrote:
> On Thu, Aug 24, 2023 at 09:04:17PM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 25, 2023 at 11:07:56AM +1000, Dave Chinner wrote:
> > > On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Don't allow log recovery to proceed on a readonly mount if the primary
> > > > superblock advertises unknown rocompat bits.  We used to allow this, but
> > > > due to a misunderstanding between Dave and Darrick back in 2016, we
> > > > cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
> > > > protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
> > > > log items, which is why we can't allow older kernels to recover them.
> > > 
> > > Ok, this would work for kernels that don't know waht the
> > > REFLINK/RMAP features are, but upstream kernels will never fail to
> > > recover these items because these are known ROCOMPAT bits.
> > > 
> > > The reason this problem exists is that we've accidentally
> > > conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had
> > > of set a log incompat bit whenever they are used (similar to the
> > > new ATTRI stuff setting log incompat bits), then older kernels
> > > would not have allow log recovery even if the reflink/rmap RO_COMPAT
> > > features were set and they didn't understand them.
> > > 
> > > However, we can't do that on current kernels because then older
> > > kernels that understand reflink/rmap just fine would see an unknown
> > > log incompat bit and refuse to replay the log. So it comes back to
> > > how we handle unknown ROCOMPAT flags on older kernels, not current
> > > upstream kernels.
> > > 
> > > i.e. this patch needs to be backported to kernels that don't know
> > > anything about RMAP/REFLINK to be useful to anyone. i.e. kernels
> > > older than 4.9 that don't know what rmap/reflink are.  I suspect
> > > that there are very few supported kernels that old that this might
> > > get backported to.
> 
> Seeing as the oldest LTS kernel now is 4.14, I agree with you, let's
> just forget this whole patch and try to remember not to hide LOG
> INCOMPAT features behind RO COMPAT flags again. :)
> 
> If we do that, then all we need to do is change xfs_validate_sb_write
> not to complain about unknown rocompat features if the xfs is readonly,
> and remove all the code that temporarily clears the readonly state
> around the log mount calls.
> 
> Log recovery then goes back to supporting recovery even in the presence
> of unknown rocompat bits, and patch 3 becomes unnecessary...

*nod*

> ...though this below is still true.
> 
> > Hmm.  The most annoying thing about LOG_INCOMPAT features is that
> > turning them on requires a synchronous write to the primary sb along
> > with a transaction to log the sb that is immediately forced to disk.
> > Every time the log cleans itself it clears the LOG_INCOMPAT features,
> > and then we have to do that /again/.
> > 
> > Parent pointers, since they require log intent items to guarantee the
> > dirent and pptr update, cycle the logged xattr LOG_INCOMPAT feature on
> > and off repeatedly.  A couple of weeks ago I decided to elide all that
> > LOG_INCOMPAT cycling if parent pointers are enabled, and fstests runtime
> > went from 4.9 hours back down to 4.4.  (Parent pointers, for whatever
> > reason, got an INCOMPAT feature bit so it's ok).  I was a little
> > surprised that xfs_log_clean ran that much, but there we go.

Sure, but that's only a problem for operations that are a pure
log format change. With parent pointers, we have an INCOMPAT bit
because we have a new attr filter bit that older kernels will flag
as corruption, and that means we don't need a log incompat bit for
the attr logging. IOWs, if xfs_has_parent_pointers() is true, then
we don't need to set the ATTRI log incompat bit, ever, because the
parent pointer incompat feature bit implies ATTRI log items are in use and all
kernels that understand the PP incompat bit also understand the
ATTRI log items....

> > A different way to solve the cycling problem could be to start a timer
> > after the last caller drops l_incompat_xattrs and only clear the feature
> > bit after 5 minutes of idleness or unmount, instead of the next time the
> > log cleans itself.

Well, we only clear it from the xfs_log_worker() if the log needs
covering, so the AIL and iclogs need to be empty before the worker
will clear the incompat bit. That's on a 30s timer already, so
perhaps all we need to do is extend the log covering timeout if
there are incompat log flags set....

> > Alternately, we drop this patch and declare an INCOMPAT_RMAPREFLINKV2
> > feature that wraps up all the other broken bits that we've found since
> > 2016 (overly large log reservations, incorrect units in xattr block
> > reservation calculations, etc.)

It's tempting, but let's try to put that off until we really need
to....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/3] xfs: don't allow log recovery when unknown rocompat bits are set

[Darrick J. Wong]

On Tue, Aug 29, 2023 at 07:47:47AM +1000, Dave Chinner wrote:
> On Mon, Aug 28, 2023 at 12:08:22PM -0700, Darrick J. Wong wrote:
> > On Thu, Aug 24, 2023 at 09:04:17PM -0700, Darrick J. Wong wrote:
> > > On Fri, Aug 25, 2023 at 11:07:56AM +1000, Dave Chinner wrote:
> > > > On Thu, Aug 24, 2023 at 04:21:46PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > Don't allow log recovery to proceed on a readonly mount if the primary
> > > > > superblock advertises unknown rocompat bits.  We used to allow this, but
> > > > > due to a misunderstanding between Dave and Darrick back in 2016, we
> > > > > cannot do that anymore.  The XFS_SB_FEAT_RO_COMPAT_RMAPBT feature (4.8)
> > > > > protects RUI log items, and the REFLINK feature (4.9) protects CUI/BUI
> > > > > log items, which is why we can't allow older kernels to recover them.
> > > > 
> > > > Ok, this would work for kernels that don't know waht the
> > > > REFLINK/RMAP features are, but upstream kernels will never fail to
> > > > recover these items because these are known ROCOMPAT bits.
> > > > 
> > > > The reason this problem exists is that we've accidentally
> > > > conflated RO_COMPAT with LOG_INCOMPAT. If RUI/CUI/BUI creation had
> > > > of set a log incompat bit whenever they are used (similar to the
> > > > new ATTRI stuff setting log incompat bits), then older kernels
> > > > would not have allow log recovery even if the reflink/rmap RO_COMPAT
> > > > features were set and they didn't understand them.
> > > > 
> > > > However, we can't do that on current kernels because then older
> > > > kernels that understand reflink/rmap just fine would see an unknown
> > > > log incompat bit and refuse to replay the log. So it comes back to
> > > > how we handle unknown ROCOMPAT flags on older kernels, not current
> > > > upstream kernels.
> > > > 
> > > > i.e. this patch needs to be backported to kernels that don't know
> > > > anything about RMAP/REFLINK to be useful to anyone. i.e. kernels
> > > > older than 4.9 that don't know what rmap/reflink are.  I suspect
> > > > that there are very few supported kernels that old that this might
> > > > get backported to.
> > 
> > Seeing as the oldest LTS kernel now is 4.14, I agree with you, let's
> > just forget this whole patch and try to remember not to hide LOG
> > INCOMPAT features behind RO COMPAT flags again. :)
> > 
> > If we do that, then all we need to do is change xfs_validate_sb_write
> > not to complain about unknown rocompat features if the xfs is readonly,
> > and remove all the code that temporarily clears the readonly state
> > around the log mount calls.
> > 
> > Log recovery then goes back to supporting recovery even in the presence
> > of unknown rocompat bits, and patch 3 becomes unnecessary...
> 
> *nod*
> 
> > ...though this below is still true.
> > 
> > > Hmm.  The most annoying thing about LOG_INCOMPAT features is that
> > > turning them on requires a synchronous write to the primary sb along
> > > with a transaction to log the sb that is immediately forced to disk.
> > > Every time the log cleans itself it clears the LOG_INCOMPAT features,
> > > and then we have to do that /again/.
> > > 
> > > Parent pointers, since they require log intent items to guarantee the
> > > dirent and pptr update, cycle the logged xattr LOG_INCOMPAT feature on
> > > and off repeatedly.  A couple of weeks ago I decided to elide all that
> > > LOG_INCOMPAT cycling if parent pointers are enabled, and fstests runtime
> > > went from 4.9 hours back down to 4.4.  (Parent pointers, for whatever
> > > reason, got an INCOMPAT feature bit so it's ok).  I was a little
> > > surprised that xfs_log_clean ran that much, but there we go.
> 
> Sure, but that's only a problem for operations that are a pure
> log format change. With parent pointers, we have an INCOMPAT bit
> because we have a new attr filter bit that older kernels will flag
> as corruption, and that means we don't need a log incompat bit for
> the attr logging. IOWs, if xfs_has_parent_pointers() is true, then
> we don't need to set the ATTRI log incompat bit, ever, because the
> parent pointer incompat feature bit implies ATTRI log items are in use and all
> kernels that understand the PP incompat bit also understand the
> ATTRI log items....

Aha, that's why parent pointers got an INCOMPAT flag, thanks for the
reminder.

> > > A different way to solve the cycling problem could be to start a timer
> > > after the last caller drops l_incompat_xattrs and only clear the feature
> > > bit after 5 minutes of idleness or unmount, instead of the next time the
> > > log cleans itself.
> 
> Well, we only clear it from the xfs_log_worker() if the log needs
> covering, so the AIL and iclogs need to be empty before the worker
> will clear the incompat bit. That's on a 30s timer already, so
> perhaps all we need to do is extend the log covering timeout if
> there are incompat log flags set....

Well for non-pptr LARP I don't care since it's a debugging flag, but I
suppose for swapext we might want to consider something like that.
Though I think I might want to preserve the "30 seconds until log
commits" default behavior and not crank that up to 5 minutes.

> > > Alternately, we drop this patch and declare an INCOMPAT_RMAPREFLINKV2
> > > feature that wraps up all the other broken bits that we've found since
> > > 2016 (overly large log reservations, incorrect units in xattr block
> > > reservation calculations, etc.)
> 
> It's tempting, but let's try to put that off until we really need
> to....

Ok.

--D

> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com