[PATCHSET v29.4 01/13] xfs: improve log incompat feature handling

[PATCHSET v29.4 01/13] xfs: improve log incompat feature handling

[Darrick J. Wong]

Hi all,

This patchset improves the performance of log incompat feature bit
handling by making a few changes to how the filesystem handles them.
First, we now only clear the bits during a clean unmount to reduce calls
to the (expensive) upgrade function to once per bit per mount.  Second,
we now only allow incompat feature upgrades for sysadmins or if the
sysadmin explicitly allows it via mount option.  Currently the only log
incompat user is logged xattrs, which requires CONFIG_XFS_DEBUG=y, so
there should be no user visible impact to this change.

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

This has been running on the djcloud for months with no problems.  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=log-incompat-permissions
---
Commits in this patchset:
 * xfs: only clear log incompat flags at clean unmount
 * xfs: only add log incompat features with explicit permission
---
 Documentation/admin-guide/xfs.rst                  |    7 +++
 .../filesystems/xfs/xfs-online-fsck-design.rst     |    3 -
 fs/xfs/xfs_log.c                                   |   28 -------------
 fs/xfs/xfs_log.h                                   |    2 -
 fs/xfs/xfs_log_priv.h                              |    3 -
 fs/xfs/xfs_log_recover.c                           |   15 -------
 fs/xfs/xfs_mount.c                                 |   34 ++++++++++++++++
 fs/xfs/xfs_mount.h                                 |    9 ++++
 fs/xfs/xfs_super.c                                 |   12 +++++-
 fs/xfs/xfs_xattr.c                                 |   42 +++-----------------
 10 files changed, 66 insertions(+), 89 deletions(-)

[PATCH 1/2] xfs: only clear log incompat flags at clean unmount

[Darrick J. Wong]

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

While reviewing the online fsck patchset, someone spied the
xfs_swapext_can_use_without_log_assistance function and wondered why we
go through this inverted-bitmask dance to avoid setting the
XFS_SB_FEAT_INCOMPAT_LOG_SWAPEXT feature.

(The same principles apply to the logged extended attribute update
feature bit in the since-merged LARP series.)

The reason for this dance is that xfs_add_incompat_log_feature is an
expensive operation -- it forces the log, pushes the AIL, and then if
nobody's beaten us to it, sets the feature bit and issues a synchronous
write of the primary superblock.  That could be a one-time cost
amortized over the life of the filesystem, but the log quiesce and cover
operations call xfs_clear_incompat_log_features to remove feature bits
opportunistically.  On a moderately loaded filesystem this leads to us
cycling those bits on and off over and over, which hurts performance.

Why do we clear the log incompat bits?  Back in ~2020 I think Dave and I
had a conversation on IRC[2] about what the log incompat bits represent.
IIRC in that conversation we decided that the log incompat bits protect
unrecovered log items so that old kernels won't try to recover them and
barf.  Since a clean log has no protected log items, we could clear the
bits at cover/quiesce time.

As Dave Chinner pointed out in the thread, clearing log incompat bits at
unmount time has positive effects for golden root disk image generator
setups, since the generator could be running a newer kernel than what
gets written to the golden image -- if there are log incompat fields set
in the golden image that was generated by a newer kernel/OS image
builder then the provisioning host cannot mount the filesystem even
though the log is clean and recovery is unnecessary to mount the
filesystem.

Given that it's expensive to set log incompat bits, we really only want
to do that once per bit per mount.  Therefore, I propose that we only
clear log incompat bits as part of writing a clean unmount record.  Do
this by adding an operational state flag to the xfs mount that guards
whether or not the feature bit clearing can actually take place.

This eliminates the l_incompat_users rwsem that we use to protect a log
cleaning operation from clearing a feature bit that a frontend thread is
trying to set -- this lock adds another way to fail w.r.t. locking.  For
the swapext series, I shard that into multiple locks just to work around
the lockdep complaints, and that's fugly.

Link: https://lore.kernel.org/linux-xfs/20240131230043.GA6180@frogsfrogsfrogs/
Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 .../filesystems/xfs/xfs-online-fsck-design.rst     |    3 -
 fs/xfs/xfs_log.c                                   |   28 -------------
 fs/xfs/xfs_log.h                                   |    2 -
 fs/xfs/xfs_log_priv.h                              |    3 -
 fs/xfs/xfs_log_recover.c                           |   15 -------
 fs/xfs/xfs_mount.c                                 |    8 +++-
 fs/xfs/xfs_mount.h                                 |    6 ++-
 fs/xfs/xfs_xattr.c                                 |   42 +++-----------------
 8 files changed, 19 insertions(+), 88 deletions(-)


diff --git a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst
index 6333697ba3e82..1d161752f09ed 100644
--- a/Documentation/filesystems/xfs/xfs-online-fsck-design.rst
+++ b/Documentation/filesystems/xfs/xfs-online-fsck-design.rst
@@ -4047,9 +4047,6 @@ series.
 | one ``struct rw_semaphore`` for each feature.                            |
 | The log cleaning code tries to take this rwsem in exclusive mode to      |
 | clear the bit; if the lock attempt fails, the feature bit remains set.   |
-| Filesystem code signals its intention to use a log incompat feature in a |
-| transaction by calling ``xlog_use_incompat_feat``, which takes the rwsem |
-| in shared mode.                                                          |
 | The code supporting a log incompat feature should create wrapper         |
 | functions to obtain the log feature and call                             |
 | ``xfs_add_incompat_log_feature`` to set the feature bits in the primary  |
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index a5f92e362a248..a604eac68ea9e 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1448,7 +1448,7 @@ xfs_log_work_queue(
  * Clear the log incompat flags if we have the opportunity.
  *
  * This only happens if we're about to log the second dummy transaction as part
- * of covering the log and we can get the log incompat feature usage lock.
+ * of covering the log.
  */
 static inline void
 xlog_clear_incompat(
@@ -1463,11 +1463,7 @@ xlog_clear_incompat(
 	if (log->l_covered_state != XLOG_STATE_COVER_DONE2)
 		return;
 
-	if (!down_write_trylock(&log->l_incompat_users))
-		return;
-
 	xfs_clear_incompat_log_features(mp);
-	up_write(&log->l_incompat_users);
 }
 
 /*
@@ -1585,8 +1581,6 @@ xlog_alloc_log(
 	}
 	log->l_sectBBsize = 1 << log2_size;
 
-	init_rwsem(&log->l_incompat_users);
-
 	xlog_get_iclog_buffer_size(mp, log);
 
 	spin_lock_init(&log->l_icloglock);
@@ -3869,23 +3863,3 @@ xfs_log_check_lsn(
 
 	return valid;
 }
-
-/*
- * Notify the log that we're about to start using a feature that is protected
- * by a log incompat feature flag.  This will prevent log covering from
- * clearing those flags.
- */
-void
-xlog_use_incompat_feat(
-	struct xlog		*log)
-{
-	down_read(&log->l_incompat_users);
-}
-
-/* Notify the log that we've finished using log incompat features. */
-void
-xlog_drop_incompat_feat(
-	struct xlog		*log)
-{
-	up_read(&log->l_incompat_users);
-}
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 2728886c29639..d69acf881153d 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -159,8 +159,6 @@ bool	xfs_log_check_lsn(struct xfs_mount *, xfs_lsn_t);
 xfs_lsn_t xlog_grant_push_threshold(struct xlog *log, int need_bytes);
 bool	  xlog_force_shutdown(struct xlog *log, uint32_t shutdown_flags);
 
-void xlog_use_incompat_feat(struct xlog *log);
-void xlog_drop_incompat_feat(struct xlog *log);
 int xfs_attr_use_log_assist(struct xfs_mount *mp);
 
 #endif	/* __XFS_LOG_H__ */
diff --git a/fs/xfs/xfs_log_priv.h b/fs/xfs/xfs_log_priv.h
index e30c06ec20e33..43881575cd498 100644
--- a/fs/xfs/xfs_log_priv.h
+++ b/fs/xfs/xfs_log_priv.h
@@ -450,9 +450,6 @@ struct xlog {
 	xfs_lsn_t		l_recovery_lsn;
 
 	uint32_t		l_iclog_roundoff;/* padding roundoff */
-
-	/* Users of log incompat features should take a read lock. */
-	struct rw_semaphore	l_incompat_users;
 };
 
 /*
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1251c81e55f98..36a1b4eeb39fa 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -3472,21 +3472,6 @@ xlog_recover_finish(
 	 */
 	xfs_log_force(log->l_mp, XFS_LOG_SYNC);
 
-	/*
-	 * Now that we've recovered the log and all the intents, we can clear
-	 * the log incompat feature bits in the superblock because there's no
-	 * longer anything to protect.  We rely on the AIL push to write out the
-	 * updated superblock after everything else.
-	 */
-	if (xfs_clear_incompat_log_features(log->l_mp)) {
-		error = xfs_sync_sb(log->l_mp, false);
-		if (error < 0) {
-			xfs_alert(log->l_mp,
-	"Failed to clear log incompat features on recovery");
-			return error;
-		}
-	}
-
 	xlog_recover_process_iunlinks(log);
 
 	/*
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index aabb25dc3efab..912f3972ab413 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1093,6 +1093,11 @@ xfs_unmountfs(
 				"Freespace may not be correct on next mount.");
 	xfs_unmount_check(mp);
 
+	/*
+	 * Indicate that it's ok to clear log incompat bits before cleaning
+	 * the log and writing the unmount record.
+	 */
+	xfs_set_done_with_log_incompat(mp);
 	xfs_log_unmount(mp);
 	xfs_da_unmount(mp);
 	xfs_uuid_unmount(mp);
@@ -1362,7 +1367,8 @@ xfs_clear_incompat_log_features(
 	if (!xfs_has_crc(mp) ||
 	    !xfs_sb_has_incompat_log_feature(&mp->m_sb,
 				XFS_SB_FEAT_INCOMPAT_LOG_ALL) ||
-	    xfs_is_shutdown(mp))
+	    xfs_is_shutdown(mp) ||
+	    !xfs_is_done_with_log_incompat(mp))
 		return false;
 
 	/*
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index e880aa48de68b..6ec038b88454c 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -412,6 +412,8 @@ __XFS_HAS_FEAT(nouuid, NOUUID)
 #define XFS_OPSTATE_WARNED_LARP		9
 /* Mount time quotacheck is running */
 #define XFS_OPSTATE_QUOTACHECK_RUNNING	10
+/* Do we want to clear log incompat flags? */
+#define XFS_OPSTATE_UNSET_LOG_INCOMPAT	11
 
 #define __XFS_IS_OPSTATE(name, NAME) \
 static inline bool xfs_is_ ## name (struct xfs_mount *mp) \
@@ -439,6 +441,7 @@ __XFS_IS_OPSTATE(quotacheck_running, QUOTACHECK_RUNNING)
 #else
 # define xfs_is_quotacheck_running(mp)	(false)
 #endif
+__XFS_IS_OPSTATE(done_with_log_incompat, UNSET_LOG_INCOMPAT)
 
 static inline bool
 xfs_should_warn(struct xfs_mount *mp, long nr)
@@ -457,7 +460,8 @@ xfs_should_warn(struct xfs_mount *mp, long nr)
 	{ (1UL << XFS_OPSTATE_WARNED_SCRUB),		"wscrub" }, \
 	{ (1UL << XFS_OPSTATE_WARNED_SHRINK),		"wshrink" }, \
 	{ (1UL << XFS_OPSTATE_WARNED_LARP),		"wlarp" }, \
-	{ (1UL << XFS_OPSTATE_QUOTACHECK_RUNNING),	"quotacheck" }
+	{ (1UL << XFS_OPSTATE_QUOTACHECK_RUNNING),	"quotacheck" }, \
+	{ (1UL << XFS_OPSTATE_UNSET_LOG_INCOMPAT),	"unset_log_incompat" }
 
 /*
  * Max and min values for mount-option defined I/O
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 364104e1b38ae..4ebf7052eb673 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -22,10 +22,7 @@
 
 /*
  * Get permission to use log-assisted atomic exchange of file extents.
- *
- * Callers must not be running any transactions or hold any inode locks, and
- * they must release the permission by calling xlog_drop_incompat_feat
- * when they're done.
+ * Callers must not be running any transactions or hold any ILOCKs.
  */
 static inline int
 xfs_attr_grab_log_assist(
@@ -33,16 +30,7 @@ xfs_attr_grab_log_assist(
 {
 	int			error = 0;
 
-	/*
-	 * Protect ourselves from an idle log clearing the logged xattrs log
-	 * incompat feature bit.
-	 */
-	xlog_use_incompat_feat(mp->m_log);
-
-	/*
-	 * If log-assisted xattrs are already enabled, the caller can use the
-	 * log assisted swap functions with the log-incompat reference we got.
-	 */
+	/* xattr update log intent items are already enabled */
 	if (xfs_sb_version_haslogxattrs(&mp->m_sb))
 		return 0;
 
@@ -52,31 +40,19 @@ xfs_attr_grab_log_assist(
 	 * a V5 filesystem for the superblock field, but we'll require rmap
 	 * or reflink to avoid having to deal with really old kernels.
 	 */
-	if (!xfs_has_reflink(mp) && !xfs_has_rmapbt(mp)) {
-		error = -EOPNOTSUPP;
-		goto drop_incompat;
-	}
+	if (!xfs_has_reflink(mp) && !xfs_has_rmapbt(mp))
+		return -EOPNOTSUPP;
 
 	/* Enable log-assisted xattrs. */
 	error = xfs_add_incompat_log_feature(mp,
 			XFS_SB_FEAT_INCOMPAT_LOG_XATTRS);
 	if (error)
-		goto drop_incompat;
+		return error;
 
 	xfs_warn_mount(mp, XFS_OPSTATE_WARNED_LARP,
  "EXPERIMENTAL logged extended attributes feature in use. Use at your own risk!");
 
 	return 0;
-drop_incompat:
-	xlog_drop_incompat_feat(mp->m_log);
-	return error;
-}
-
-static inline void
-xfs_attr_rele_log_assist(
-	struct xfs_mount	*mp)
-{
-	xlog_drop_incompat_feat(mp->m_log);
 }
 
 static inline bool
@@ -100,7 +76,6 @@ xfs_attr_change(
 	struct xfs_da_args	*args)
 {
 	struct xfs_mount	*mp = args->dp->i_mount;
-	bool			use_logging = false;
 	int			error;
 
 	ASSERT(!(args->op_flags & XFS_DA_OP_LOGGED));
@@ -111,14 +86,9 @@ xfs_attr_change(
 			return error;
 
 		args->op_flags |= XFS_DA_OP_LOGGED;
-		use_logging = true;
 	}
 
-	error = xfs_attr_set(args);
-
-	if (use_logging)
-		xfs_attr_rele_log_assist(mp);
-	return error;
+	return xfs_attr_set(args);
 }
 
 

Re: [PATCH 1/2] xfs: only clear log incompat flags at clean unmount

[Christoph Hellwig]

Looks good:

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

[PATCH 2/2] xfs: only add log incompat features with explicit permission

[Darrick J. Wong]

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

Only allow the addition of new log incompat features to the primary
superblock if the sysadmin provides explicit consent via a mount option
or if the process has administrative privileges.  This should prevent
surprises when trying to recover dirty logs on old kernels.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 Documentation/admin-guide/xfs.rst |    7 +++++++
 fs/xfs/xfs_mount.c                |   26 ++++++++++++++++++++++++++
 fs/xfs/xfs_mount.h                |    3 +++
 fs/xfs/xfs_super.c                |   12 +++++++++++-
 4 files changed, 47 insertions(+), 1 deletion(-)


diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index b67772cf36d6d..52acd95b2b754 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -21,6 +21,13 @@ Mount Options
 
 When mounting an XFS filesystem, the following options are accepted.
 
+  add_log_feat/noadd_log_feat
+        Permit unprivileged userspace to use functionality that requires
+        the addition of log incompat feature bits to the superblock.
+        The feature bits will be cleared during a clean unmount.
+        Old kernels cannot recover dirty logs if they do not recognize
+        all log incompat feature bits.
+
   allocsize=size
 	Sets the buffered I/O end-of-file preallocation size when
 	doing delayed allocation writeout (default size is 64KiB).
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 912f3972ab413..6fd4ceeab0e26 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1279,6 +1279,27 @@ xfs_force_summary_recalc(
 	xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
 }
 
+/*
+ * Allow the log feature upgrade only if the sysadmin permits it via mount
+ * option; or the caller is the administrator.  If the @want_audit parameter
+ * is true, then a denial due to insufficient privileges will be logged.
+ */
+bool
+xfs_can_add_incompat_log_features(
+	struct xfs_mount	*mp,
+	bool			want_audit)
+{
+	/* Always allowed if the mount option is set */
+	if (mp->m_features & XFS_FEAT_ADD_LOG_FEAT)
+		return true;
+
+	/* Allowed for administrators */
+	if (want_audit)
+		return capable(CAP_SYS_ADMIN);
+
+	return has_capability_noaudit(current, CAP_SYS_ADMIN);
+}
+
 /*
  * Enable a log incompat feature flag in the primary superblock.  The caller
  * cannot have any other transactions in progress.
@@ -1320,6 +1341,11 @@ xfs_add_incompat_log_feature(
 	if (xfs_sb_has_incompat_log_feature(&mp->m_sb, feature))
 		goto rele;
 
+	if (!xfs_can_add_incompat_log_features(mp, true)) {
+		error = -EOPNOTSUPP;
+		goto rele;
+	}
+
 	/*
 	 * Write the primary superblock to disk immediately, because we need
 	 * the log_incompat bit to be set in the primary super now to protect
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 6ec038b88454c..654d282234b1e 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -294,6 +294,7 @@ typedef struct xfs_mount {
 #define XFS_FEAT_NREXT64	(1ULL << 26)	/* large extent counters */
 
 /* Mount features */
+#define XFS_FEAT_ADD_LOG_FEAT	(1ULL << 47)	/* can add log incompat features */
 #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
 #define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
 #define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
@@ -356,6 +357,8 @@ __XFS_HAS_FEAT(bigtime, BIGTIME)
 __XFS_HAS_FEAT(needsrepair, NEEDSREPAIR)
 __XFS_HAS_FEAT(large_extent_counts, NREXT64)
 
+bool xfs_can_add_incompat_log_features(struct xfs_mount *mp, bool want_audit);
+
 /*
  * Mount features
  *
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 4b22c30ac97a4..679b99bed5499 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -103,7 +103,8 @@ enum {
 	Opt_filestreams, Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota,
 	Opt_prjquota, Opt_uquota, Opt_gquota, Opt_pquota,
 	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
-	Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum, Opt_add_log_feat,
+	Opt_noadd_log_feat,
 };
 
 static const struct fs_parameter_spec xfs_fs_parameters[] = {
@@ -148,6 +149,8 @@ static const struct fs_parameter_spec xfs_fs_parameters[] = {
 	fsparam_flag("nodiscard",	Opt_nodiscard),
 	fsparam_flag("dax",		Opt_dax),
 	fsparam_enum("dax",		Opt_dax_enum, dax_param_enums),
+	fsparam_flag("add_log_feat",	Opt_add_log_feat),
+	fsparam_flag("noadd_log_feat",	Opt_noadd_log_feat),
 	{}
 };
 
@@ -176,6 +179,7 @@ xfs_fs_show_options(
 		{ XFS_FEAT_LARGE_IOSIZE,	",largeio" },
 		{ XFS_FEAT_DAX_ALWAYS,		",dax=always" },
 		{ XFS_FEAT_DAX_NEVER,		",dax=never" },
+		{ XFS_FEAT_ADD_LOG_FEAT,	",add_log_feat" },
 		{ 0, NULL }
 	};
 	struct xfs_mount	*mp = XFS_M(root->d_sb);
@@ -1371,6 +1375,12 @@ xfs_fs_parse_param(
 		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
 		return 0;
 #endif
+	case Opt_add_log_feat:
+		parsing_mp->m_features |= XFS_FEAT_ADD_LOG_FEAT;
+		return 0;
+	case Opt_noadd_log_feat:
+		parsing_mp->m_features &= ~XFS_FEAT_ADD_LOG_FEAT;
+		return 0;
 	/* Following mount options will be removed in September 2025 */
 	case Opt_ikeep:
 		xfs_fs_warn_deprecated(fc, param, XFS_FEAT_IKEEP, true);

Re: [PATCH 2/2] xfs: only add log incompat features with explicit permission

[Christoph Hellwig]

Looks good:

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

(although the CAP_SYS_ADMIN override is a bit odd if we explicitly
asked to not do automagic feature upgrades)

Re: [PATCH 2/2] xfs: only add log incompat features with explicit permission

[Darrick J. Wong]

On Tue, Feb 27, 2024 at 10:09:20AM -0800, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> (although the CAP_SYS_ADMIN override is a bit odd if we explicitly
> asked to not do automagic feature upgrades)

Wellll... it's 'add_log_feat' (allow adding log features) or
"noadd_log_feat" (stop allowing adding log features).  There's no
setting for "don't allow, even for sysadmins".

I could turn it into a stringly typed option "log_feat={allow,forbid}"
but yuck. ;)

--D

[PATCHSET v30.1 02/15] xfs: improve log incompat feature handling

[Darrick J. Wong]

Hi all,

This patchset improves the performance of log incompat feature bit
handling by making a few changes to how the filesystem handles them.
First, we now only clear the bits during a clean unmount to reduce calls
to the (expensive) upgrade function to once per bit per mount.  Second,
we now only allow incompat feature upgrades for sysadmins or if the
sysadmin explicitly allows it via mount option.  Currently the only log
incompat user is logged xattrs, which requires CONFIG_XFS_DEBUG=y, so
there should be no user visible impact to this change.

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

This has been running on the djcloud for months with no problems.  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=log-incompat-permissions
---
Commits in this patchset:
 * xfs: only clear log incompat flags at clean unmount
 * xfs: only add log incompat features with explicit permission
---
 Documentation/admin-guide/xfs.rst                  |    7 +++
 .../filesystems/xfs/xfs-online-fsck-design.rst     |    3 -
 fs/xfs/xfs_log.c                                   |   28 -------------
 fs/xfs/xfs_log.h                                   |    2 -
 fs/xfs/xfs_log_priv.h                              |    3 -
 fs/xfs/xfs_log_recover.c                           |   15 -------
 fs/xfs/xfs_mount.c                                 |   34 ++++++++++++++++
 fs/xfs/xfs_mount.h                                 |    9 ++++
 fs/xfs/xfs_super.c                                 |   12 +++++-
 fs/xfs/xfs_xattr.c                                 |   42 +++-----------------
 10 files changed, 66 insertions(+), 89 deletions(-)

[PATCH 2/2] xfs: only add log incompat features with explicit permission

[Darrick J. Wong]

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

Only allow the addition of new log incompat features to the primary
superblock if the sysadmin provides explicit consent via a mount option
or if the process has administrative privileges.  This should prevent
surprises when trying to recover dirty logs on old kernels.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 Documentation/admin-guide/xfs.rst |    7 +++++++
 fs/xfs/xfs_mount.c                |   26 ++++++++++++++++++++++++++
 fs/xfs/xfs_mount.h                |    3 +++
 fs/xfs/xfs_super.c                |   12 +++++++++++-
 4 files changed, 47 insertions(+), 1 deletion(-)


diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
index b67772cf36d6d..52acd95b2b754 100644
--- a/Documentation/admin-guide/xfs.rst
+++ b/Documentation/admin-guide/xfs.rst
@@ -21,6 +21,13 @@ Mount Options
 
 When mounting an XFS filesystem, the following options are accepted.
 
+  add_log_feat/noadd_log_feat
+        Permit unprivileged userspace to use functionality that requires
+        the addition of log incompat feature bits to the superblock.
+        The feature bits will be cleared during a clean unmount.
+        Old kernels cannot recover dirty logs if they do not recognize
+        all log incompat feature bits.
+
   allocsize=size
 	Sets the buffered I/O end-of-file preallocation size when
 	doing delayed allocation writeout (default size is 64KiB).
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index d37ba10f5fa33..a0b271758f910 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1281,6 +1281,27 @@ xfs_force_summary_recalc(
 	xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
 }
 
+/*
+ * Allow the log feature upgrade only if the sysadmin permits it via mount
+ * option; or the caller is the administrator.  If the @want_audit parameter
+ * is true, then a denial due to insufficient privileges will be logged.
+ */
+bool
+xfs_can_add_incompat_log_features(
+	struct xfs_mount	*mp,
+	bool			want_audit)
+{
+	/* Always allowed if the mount option is set */
+	if (mp->m_features & XFS_FEAT_ADD_LOG_FEAT)
+		return true;
+
+	/* Allowed for administrators */
+	if (want_audit)
+		return capable(CAP_SYS_ADMIN);
+
+	return has_capability_noaudit(current, CAP_SYS_ADMIN);
+}
+
 /*
  * Enable a log incompat feature flag in the primary superblock.  The caller
  * cannot have any other transactions in progress.
@@ -1322,6 +1343,11 @@ xfs_add_incompat_log_feature(
 	if (xfs_sb_has_incompat_log_feature(&mp->m_sb, feature))
 		goto rele;
 
+	if (!xfs_can_add_incompat_log_features(mp, true)) {
+		error = -EOPNOTSUPP;
+		goto rele;
+	}
+
 	/*
 	 * Write the primary superblock to disk immediately, because we need
 	 * the log_incompat bit to be set in the primary super now to protect
diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
index 6ec038b88454c..654d282234b1e 100644
--- a/fs/xfs/xfs_mount.h
+++ b/fs/xfs/xfs_mount.h
@@ -294,6 +294,7 @@ typedef struct xfs_mount {
 #define XFS_FEAT_NREXT64	(1ULL << 26)	/* large extent counters */
 
 /* Mount features */
+#define XFS_FEAT_ADD_LOG_FEAT	(1ULL << 47)	/* can add log incompat features */
 #define XFS_FEAT_NOATTR2	(1ULL << 48)	/* disable attr2 creation */
 #define XFS_FEAT_NOALIGN	(1ULL << 49)	/* ignore alignment */
 #define XFS_FEAT_ALLOCSIZE	(1ULL << 50)	/* user specified allocation size */
@@ -356,6 +357,8 @@ __XFS_HAS_FEAT(bigtime, BIGTIME)
 __XFS_HAS_FEAT(needsrepair, NEEDSREPAIR)
 __XFS_HAS_FEAT(large_extent_counts, NREXT64)
 
+bool xfs_can_add_incompat_log_features(struct xfs_mount *mp, bool want_audit);
+
 /*
  * Mount features
  *
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index c21f10ab0f5db..a2dcbac6effe9 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -103,7 +103,8 @@ enum {
 	Opt_filestreams, Opt_quota, Opt_noquota, Opt_usrquota, Opt_grpquota,
 	Opt_prjquota, Opt_uquota, Opt_gquota, Opt_pquota,
 	Opt_uqnoenforce, Opt_gqnoenforce, Opt_pqnoenforce, Opt_qnoenforce,
-	Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum,
+	Opt_discard, Opt_nodiscard, Opt_dax, Opt_dax_enum, Opt_add_log_feat,
+	Opt_noadd_log_feat,
 };
 
 static const struct fs_parameter_spec xfs_fs_parameters[] = {
@@ -148,6 +149,8 @@ static const struct fs_parameter_spec xfs_fs_parameters[] = {
 	fsparam_flag("nodiscard",	Opt_nodiscard),
 	fsparam_flag("dax",		Opt_dax),
 	fsparam_enum("dax",		Opt_dax_enum, dax_param_enums),
+	fsparam_flag("add_log_feat",	Opt_add_log_feat),
+	fsparam_flag("noadd_log_feat",	Opt_noadd_log_feat),
 	{}
 };
 
@@ -176,6 +179,7 @@ xfs_fs_show_options(
 		{ XFS_FEAT_LARGE_IOSIZE,	",largeio" },
 		{ XFS_FEAT_DAX_ALWAYS,		",dax=always" },
 		{ XFS_FEAT_DAX_NEVER,		",dax=never" },
+		{ XFS_FEAT_ADD_LOG_FEAT,	",add_log_feat" },
 		{ 0, NULL }
 	};
 	struct xfs_mount	*mp = XFS_M(root->d_sb);
@@ -1368,6 +1372,12 @@ xfs_fs_parse_param(
 		xfs_mount_set_dax_mode(parsing_mp, result.uint_32);
 		return 0;
 #endif
+	case Opt_add_log_feat:
+		parsing_mp->m_features |= XFS_FEAT_ADD_LOG_FEAT;
+		return 0;
+	case Opt_noadd_log_feat:
+		parsing_mp->m_features &= ~XFS_FEAT_ADD_LOG_FEAT;
+		return 0;
 	/* Following mount options will be removed in September 2025 */
 	case Opt_ikeep:
 		xfs_fs_warn_deprecated(fc, param, XFS_FEAT_IKEEP, true);

Re: [PATCH 2/2] xfs: only add log incompat features with explicit permission

[Dave Chinner]

On Tue, Mar 26, 2024 at 06:51:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Only allow the addition of new log incompat features to the primary
> superblock if the sysadmin provides explicit consent via a mount option
> or if the process has administrative privileges.  This should prevent
> surprises when trying to recover dirty logs on old kernels.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

As I said originally when this was proposed, the logic needs to
default to allow log incompat features to be added rather than
disallow.

Essentially, having the default as "not allowed" means in future
every single XFS mount on every single system is going to have to
add this mount option to allow new log format features to used.

This "default = disallow" means our regression test systems will not
be exercising features based on this code without explicitly
expanding every independent test configuration matrix by another
dimension. This essentially means there will be almost no test
coverage for these dynamic features..

So, yeah, I think this needs to default to "allow", not "disallow".


> ---
>  Documentation/admin-guide/xfs.rst |    7 +++++++
>  fs/xfs/xfs_mount.c                |   26 ++++++++++++++++++++++++++
>  fs/xfs/xfs_mount.h                |    3 +++
>  fs/xfs/xfs_super.c                |   12 +++++++++++-
>  4 files changed, 47 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> index b67772cf36d6d..52acd95b2b754 100644
> --- a/Documentation/admin-guide/xfs.rst
> +++ b/Documentation/admin-guide/xfs.rst
> @@ -21,6 +21,13 @@ Mount Options
>  
>  When mounting an XFS filesystem, the following options are accepted.
>  
> +  add_log_feat/noadd_log_feat
> +        Permit unprivileged userspace to use functionality that requires
> +        the addition of log incompat feature bits to the superblock.
> +        The feature bits will be cleared during a clean unmount.
> +        Old kernels cannot recover dirty logs if they do not recognize
> +        all log incompat feature bits.
> +
>    allocsize=size
>  	Sets the buffered I/O end-of-file preallocation size when
>  	doing delayed allocation writeout (default size is 64KiB).
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index d37ba10f5fa33..a0b271758f910 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -1281,6 +1281,27 @@ xfs_force_summary_recalc(
>  	xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
>  }
>  
> +/*
> + * Allow the log feature upgrade only if the sysadmin permits it via mount
> + * option; or the caller is the administrator.  If the @want_audit parameter
> + * is true, then a denial due to insufficient privileges will be logged.
> + */
> +bool
> +xfs_can_add_incompat_log_features(
> +	struct xfs_mount	*mp,
> +	bool			want_audit)
> +{
> +	/* Always allowed if the mount option is set */
> +	if (mp->m_features & XFS_FEAT_ADD_LOG_FEAT)
> +		return true;

Please define a __XFS_HAS_FEAT() macro for this feature bit and
use xfs_has_log_features_enabled() wrapper for it.

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

Re: [PATCH 2/2] xfs: only add log incompat features with explicit permission

[Darrick J. Wong]

On Mon, Apr 08, 2024 at 09:00:11AM +1000, Dave Chinner wrote:
> On Tue, Mar 26, 2024 at 06:51:00PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Only allow the addition of new log incompat features to the primary
> > superblock if the sysadmin provides explicit consent via a mount option
> > or if the process has administrative privileges.  This should prevent
> > surprises when trying to recover dirty logs on old kernels.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> As I said originally when this was proposed, the logic needs to
> default to allow log incompat features to be added rather than
> disallow.
> 
> Essentially, having the default as "not allowed" means in future
> every single XFS mount on every single system is going to have to
> add this mount option to allow new log format features to used.
> 
> This "default = disallow" means our regression test systems will not
> be exercising features based on this code without explicitly
> expanding every independent test configuration matrix by another
> dimension. This essentially means there will be almost no test
> coverage for these dynamic features..
> 
> So, yeah, I think this needs to default to "allow", not "disallow".

This is moot -- I changed exchangerange to use a permanent incompat
feature so that we can guarantee to users that if the xfs_info output
says it's enabled then it's 100% ready to go.  Hence I no longer care
what happens to log incompat bits and this patch no longer needs to
exist.

--D

> 
> > ---
> >  Documentation/admin-guide/xfs.rst |    7 +++++++
> >  fs/xfs/xfs_mount.c                |   26 ++++++++++++++++++++++++++
> >  fs/xfs/xfs_mount.h                |    3 +++
> >  fs/xfs/xfs_super.c                |   12 +++++++++++-
> >  4 files changed, 47 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst
> > index b67772cf36d6d..52acd95b2b754 100644
> > --- a/Documentation/admin-guide/xfs.rst
> > +++ b/Documentation/admin-guide/xfs.rst
> > @@ -21,6 +21,13 @@ Mount Options
> >  
> >  When mounting an XFS filesystem, the following options are accepted.
> >  
> > +  add_log_feat/noadd_log_feat
> > +        Permit unprivileged userspace to use functionality that requires
> > +        the addition of log incompat feature bits to the superblock.
> > +        The feature bits will be cleared during a clean unmount.
> > +        Old kernels cannot recover dirty logs if they do not recognize
> > +        all log incompat feature bits.
> > +
> >    allocsize=size
> >  	Sets the buffered I/O end-of-file preallocation size when
> >  	doing delayed allocation writeout (default size is 64KiB).
> > diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> > index d37ba10f5fa33..a0b271758f910 100644
> > --- a/fs/xfs/xfs_mount.c
> > +++ b/fs/xfs/xfs_mount.c
> > @@ -1281,6 +1281,27 @@ xfs_force_summary_recalc(
> >  	xfs_fs_mark_sick(mp, XFS_SICK_FS_COUNTERS);
> >  }
> >  
> > +/*
> > + * Allow the log feature upgrade only if the sysadmin permits it via mount
> > + * option; or the caller is the administrator.  If the @want_audit parameter
> > + * is true, then a denial due to insufficient privileges will be logged.
> > + */
> > +bool
> > +xfs_can_add_incompat_log_features(
> > +	struct xfs_mount	*mp,
> > +	bool			want_audit)
> > +{
> > +	/* Always allowed if the mount option is set */
> > +	if (mp->m_features & XFS_FEAT_ADD_LOG_FEAT)
> > +		return true;
> 
> Please define a __XFS_HAS_FEAT() macro for this feature bit and
> use xfs_has_log_features_enabled() wrapper for it.
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>