[PATCHSET 0/5] xfs: last pile of LARP cleanups for 5.19

[PATCHSET 0/5] xfs: last pile of LARP cleanups for 5.19

[Darrick J. Wong]

Hi all,

This final series contains a two key cleanups for the new LARP code.
Most of it is refactoring and tweaking the code that creates kernel log
messages about enabling and disabling features -- we should be warning
about LARP being turned on once per day, instead of once per insmod
cycle; we shouldn't be spamming the logs so aggressively about turning
*off* log incompat features.

The second part of the series refactors the LARP code responsible for
getting (and releasing) permission to use xattr log items.  The
implementation code doesn't belong in xfs_log.c, and calls to logging
functions don't belong in libxfs -- they really should be done by the
VFS implementation functions before they start calling into libraries.
As a side effect, we now amortize the cost of gaining xattr log item
permission across entire attrmulti calls.

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=larp-cleanups-5.19
---
 fs/xfs/libxfs/xfs_attr.c |   12 +-------
 fs/xfs/scrub/scrub.c     |   17 +-----------
 fs/xfs/xfs_acl.c         |   10 +++++++
 fs/xfs/xfs_fsops.c       |    7 +----
 fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
 fs/xfs/xfs_ioctl.h       |    2 +
 fs/xfs/xfs_ioctl32.c     |    4 ++-
 fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
 fs/xfs/xfs_log.c         |   41 -----------------------------
 fs/xfs/xfs_log.h         |    1 -
 fs/xfs/xfs_message.h     |   12 ++++++++
 fs/xfs/xfs_mount.c       |    1 -
 fs/xfs/xfs_super.h       |    2 +
 fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
 14 files changed, 135 insertions(+), 86 deletions(-)

[PATCH 1/5] xfs: don't log every time we clear the log incompat flags

[Darrick J. Wong]

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

There's no need to spam the logs every time we clear the log incompat
flags -- if someone is periodically using one of these features, they'll
be cleared every time the log tries to clean itself, which can get
pretty chatty:

$ dmesg | grep -i clear
[ 5363.894711] XFS (sdd): Clearing log incompat feature flags.
[ 5365.157516] XFS (sdd): Clearing log incompat feature flags.
[ 5369.388543] XFS (sdd): Clearing log incompat feature flags.
[ 5371.281246] XFS (sdd): Clearing log incompat feature flags.

These aren't high value messages either -- nothing's gone wrong, and
nobody's trying anything tricky.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/xfs_mount.c |    1 -
 1 file changed, 1 deletion(-)


diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index 0c0bcbd4949d..daa8d29c46b4 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -1356,7 +1356,6 @@ xfs_clear_incompat_log_features(
 
 	if (xfs_sb_has_incompat_log_feature(&mp->m_sb,
 				XFS_SB_FEAT_INCOMPAT_LOG_ALL)) {
-		xfs_info(mp, "Clearing log incompat feature flags.");
 		xfs_sb_remove_incompat_log_features(&mp->m_sb);
 		ret = true;
 	}

[PATCH 2/5] xfs: refactor the code to warn about something once per day

[Darrick J. Wong]

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

Refactor the code that warns about something once per day, so that we
actually do this consistently -- emit the warning once per day.  Don't
bother emitting the "XXXX messages suppressed" messages because it's
hard to associate the suppression message with the message suppressed
when the period is very long.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/scrub/scrub.c |   17 ++---------------
 fs/xfs/xfs_fsops.c   |    7 +------
 fs/xfs/xfs_message.h |   12 ++++++++++++
 3 files changed, 15 insertions(+), 21 deletions(-)


diff --git a/fs/xfs/scrub/scrub.c b/fs/xfs/scrub/scrub.c
index b11870d07c56..1fc3247fb594 100644
--- a/fs/xfs/scrub/scrub.c
+++ b/fs/xfs/scrub/scrub.c
@@ -340,20 +340,6 @@ static const struct xchk_meta_ops meta_scrub_ops[] = {
 	},
 };
 
-/* This isn't a stable feature, warn once per day. */
-static inline void
-xchk_experimental_warning(
-	struct xfs_mount	*mp)
-{
-	static struct ratelimit_state scrub_warning = RATELIMIT_STATE_INIT(
-			"xchk_warning", 86400 * HZ, 1);
-	ratelimit_set_flags(&scrub_warning, RATELIMIT_MSG_ON_RELEASE);
-
-	if (__ratelimit(&scrub_warning))
-		xfs_alert(mp,
-"EXPERIMENTAL online scrub feature in use. Use at your own risk!");
-}
-
 static int
 xchk_validate_inputs(
 	struct xfs_mount		*mp,
@@ -478,7 +464,8 @@ xfs_scrub_metadata(
 	if (error)
 		goto out;
 
-	xchk_experimental_warning(mp);
+	xfs_warn_daily(mp,
+ "EXPERIMENTAL online scrub feature in use. Use at your own risk!");
 
 	sc = kmem_zalloc(sizeof(struct xfs_scrub), KM_NOFS | KM_MAYFAIL);
 	if (!sc) {
diff --git a/fs/xfs/xfs_fsops.c b/fs/xfs/xfs_fsops.c
index 888839e75d11..88b1cc6f9d51 100644
--- a/fs/xfs/xfs_fsops.c
+++ b/fs/xfs/xfs_fsops.c
@@ -149,12 +149,7 @@ xfs_growfs_data_private(
 		error = xfs_resizefs_init_new_ags(tp, &id, oagcount, nagcount,
 						  delta, &lastag_extended);
 	} else {
-		static struct ratelimit_state shrink_warning = \
-			RATELIMIT_STATE_INIT("shrink_warning", 86400 * HZ, 1);
-		ratelimit_set_flags(&shrink_warning, RATELIMIT_MSG_ON_RELEASE);
-
-		if (__ratelimit(&shrink_warning))
-			xfs_alert(mp,
+		xfs_warn_daily(mp,
 	"EXPERIMENTAL online shrink feature in use. Use at your own risk!");
 
 		error = xfs_ag_shrink_space(mp, &tp, nagcount - 1, -delta);
diff --git a/fs/xfs/xfs_message.h b/fs/xfs/xfs_message.h
index 55ee464ab59f..24336e18dff2 100644
--- a/fs/xfs/xfs_message.h
+++ b/fs/xfs/xfs_message.h
@@ -55,6 +55,15 @@ do {									\
 		func(dev, fmt, ##__VA_ARGS__);				\
 } while (0)
 
+#define xfs_printk_daily(func, dev, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs, 86400 * HZ, 1);		\
+									\
+	ratelimit_set_flags(&_rs, RATELIMIT_MSG_ON_RELEASE);		\
+	if (__ratelimit(&_rs))						\
+		func(dev, fmt, ##__VA_ARGS__);				\
+} while (0)
+
 #define xfs_printk_once(func, dev, fmt, ...)			\
 	DO_ONCE_LITE(func, dev, fmt, ##__VA_ARGS__)
 
@@ -75,6 +84,9 @@ do {									\
 #define xfs_debug_ratelimited(dev, fmt, ...)				\
 	xfs_printk_ratelimited(xfs_debug, dev, fmt, ##__VA_ARGS__)
 
+#define xfs_warn_daily(dev, fmt, ...)				\
+	xfs_printk_daily(xfs_warn, dev, fmt, ##__VA_ARGS__)
+
 #define xfs_warn_once(dev, fmt, ...)				\
 	xfs_printk_once(xfs_warn, dev, fmt, ##__VA_ARGS__)
 #define xfs_notice_once(dev, fmt, ...)				\

[PATCH 3/5] xfs: warn about LARP once per day

[Darrick J. Wong]

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

Since LARP is an experimental debug-only feature, we should try to warn
about it being in use once per day, not once per reboot.

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


diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 9dc748abdf33..edd077e055d5 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3910,8 +3910,8 @@ xfs_attr_use_log_assist(
 	if (error)
 		goto drop_incompat;
 
-	xfs_warn_once(mp,
-"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
+	xfs_warn_daily(mp,
+ "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
 
 	return 0;
 drop_incompat:

Re: [PATCH 3/5] xfs: warn about LARP once per day

[Dave Chinner]

On Sun, May 22, 2022 at 08:28:30AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> Since LARP is an experimental debug-only feature, we should try to warn
> about it being in use once per day, not once per reboot.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/xfs_log.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 9dc748abdf33..edd077e055d5 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -3910,8 +3910,8 @@ xfs_attr_use_log_assist(
>  	if (error)
>  		goto drop_incompat;
>  
> -	xfs_warn_once(mp,
> -"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> +	xfs_warn_daily(mp,
> + "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");

I think even this is wrong. We need this to warn once per *mount*
like we do with all other experimental features, not once or once
per day.  i.e. we could have 10 filesystems mounted and only one of
them will warn that EXPERIMENTAL features are in use.

We really need all filesystems that use an experimental feature to
warn about the use of said feature, not just a single filesystem.
That will make this consistent with the way we warn once (and once
only) at mount time about EXPERIMENTAL features that are enabled at
mount time...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/5] xfs: warn about LARP once per day

[Darrick J. Wong]

On Mon, May 23, 2022 at 08:54:04AM +1000, Dave Chinner wrote:
> On Sun, May 22, 2022 at 08:28:30AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > Since LARP is an experimental debug-only feature, we should try to warn
> > about it being in use once per day, not once per reboot.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_log.c |    4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > index 9dc748abdf33..edd077e055d5 100644
> > --- a/fs/xfs/xfs_log.c
> > +++ b/fs/xfs/xfs_log.c
> > @@ -3910,8 +3910,8 @@ xfs_attr_use_log_assist(
> >  	if (error)
> >  		goto drop_incompat;
> >  
> > -	xfs_warn_once(mp,
> > -"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> > +	xfs_warn_daily(mp,
> > + "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> 
> I think even this is wrong. We need this to warn once per *mount*
> like we do with all other experimental features, not once or once
> per day.  i.e. we could have 10 filesystems mounted and only one of
> them will warn that EXPERIMENTAL features are in use.
> 
> We really need all filesystems that use an experimental feature to
> warn about the use of said feature, not just a single filesystem.
> That will make this consistent with the way we warn once (and once
> only) at mount time about EXPERIMENTAL features that are enabled at
> mount time...

Ok.  I was thinking we could have an unsigned long m_warned then all
we'd need to do is convert the existing three callers (scrub, shrink,
larp) to something like:

	if (!test_and_set_bit(XFS_WARNED_FUBAR, &mp->m_warned))
		xfs_warn(mp,
 "EXPERIMENTAL fubar feature is enabled, use at your own risk!");

Also, any thoughts on the last two patches?

--D

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

Re: [PATCH 3/5] xfs: warn about LARP once per day

[Dave Chinner]

On Sun, May 22, 2022 at 06:16:57PM -0700, Darrick J. Wong wrote:
> On Mon, May 23, 2022 at 08:54:04AM +1000, Dave Chinner wrote:
> > On Sun, May 22, 2022 at 08:28:30AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Since LARP is an experimental debug-only feature, we should try to warn
> > > about it being in use once per day, not once per reboot.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/xfs_log.c |    4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > index 9dc748abdf33..edd077e055d5 100644
> > > --- a/fs/xfs/xfs_log.c
> > > +++ b/fs/xfs/xfs_log.c
> > > @@ -3910,8 +3910,8 @@ xfs_attr_use_log_assist(
> > >  	if (error)
> > >  		goto drop_incompat;
> > >  
> > > -	xfs_warn_once(mp,
> > > -"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> > > +	xfs_warn_daily(mp,
> > > + "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> > 
> > I think even this is wrong. We need this to warn once per *mount*
> > like we do with all other experimental features, not once or once
> > per day.  i.e. we could have 10 filesystems mounted and only one of
> > them will warn that EXPERIMENTAL features are in use.
> > 
> > We really need all filesystems that use an experimental feature to
> > warn about the use of said feature, not just a single filesystem.
> > That will make this consistent with the way we warn once (and once
> > only) at mount time about EXPERIMENTAL features that are enabled at
> > mount time...
> 
> Ok.  I was thinking we could have an unsigned long m_warned then all
> we'd need to do is convert the existing three callers (scrub, shrink,
> larp) to something like:
> 
> 	if (!test_and_set_bit(XFS_WARNED_FUBAR, &mp->m_warned))
> 		xfs_warn(mp,
>  "EXPERIMENTAL fubar feature is enabled, use at your own risk!");

Just use an m_opstate bit. We've got heaps, and these will
eventually get reclaimed anyway....

> Also, any thoughts on the last two patches?

Not yet, been doing tree and test stuff so far today.

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

Re: [PATCH 3/5] xfs: warn about LARP once per day

[Darrick J. Wong]

On Mon, May 23, 2022 at 12:51:04PM +1000, Dave Chinner wrote:
> On Sun, May 22, 2022 at 06:16:57PM -0700, Darrick J. Wong wrote:
> > On Mon, May 23, 2022 at 08:54:04AM +1000, Dave Chinner wrote:
> > > On Sun, May 22, 2022 at 08:28:30AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Since LARP is an experimental debug-only feature, we should try to warn
> > > > about it being in use once per day, not once per reboot.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  fs/xfs/xfs_log.c |    4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> > > > index 9dc748abdf33..edd077e055d5 100644
> > > > --- a/fs/xfs/xfs_log.c
> > > > +++ b/fs/xfs/xfs_log.c
> > > > @@ -3910,8 +3910,8 @@ xfs_attr_use_log_assist(
> > > >  	if (error)
> > > >  		goto drop_incompat;
> > > >  
> > > > -	xfs_warn_once(mp,
> > > > -"EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> > > > +	xfs_warn_daily(mp,
> > > > + "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
> > > 
> > > I think even this is wrong. We need this to warn once per *mount*
> > > like we do with all other experimental features, not once or once
> > > per day.  i.e. we could have 10 filesystems mounted and only one of
> > > them will warn that EXPERIMENTAL features are in use.
> > > 
> > > We really need all filesystems that use an experimental feature to
> > > warn about the use of said feature, not just a single filesystem.
> > > That will make this consistent with the way we warn once (and once
> > > only) at mount time about EXPERIMENTAL features that are enabled at
> > > mount time...
> > 
> > Ok.  I was thinking we could have an unsigned long m_warned then all
> > we'd need to do is convert the existing three callers (scrub, shrink,
> > larp) to something like:
> > 
> > 	if (!test_and_set_bit(XFS_WARNED_FUBAR, &mp->m_warned))
> > 		xfs_warn(mp,
> >  "EXPERIMENTAL fubar feature is enabled, use at your own risk!");
> 
> Just use an m_opstate bit. We've got heaps, and these will
> eventually get reclaimed anyway....

Ok, I'll do that tomorrow.

> > Also, any thoughts on the last two patches?
> 
> Not yet, been doing tree and test stuff so far today.

<nod>

--D

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

[PATCH 4/5] xfs: tell xfs_attr_set if the log is actually letting us use LARP mode

[Darrick J. Wong]

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

Add a means for xfs_attr_use_log_assist to tell its caller if we have a
lock on using LARP mode.  This will be used in the next patch it will be
useful to turn it on for a batch update.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |    8 ++++----
 fs/xfs/xfs_log.c         |   10 +++++++---
 fs/xfs/xfs_log.h         |    2 +-
 3 files changed, 12 insertions(+), 8 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 9f14aca29ec4..1de3db88e006 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -982,7 +982,7 @@ xfs_attr_set(
 	int			error, local;
 	int			rmt_blks = 0;
 	unsigned int		total;
-	bool			use_logging = xfs_has_larp(mp);
+	bool			need_rele = false;
 
 	if (xfs_is_shutdown(dp->i_mount))
 		return -EIO;
@@ -1027,8 +1027,8 @@ xfs_attr_set(
 		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
 	}
 
-	if (use_logging) {
-		error = xfs_attr_use_log_assist(mp);
+	if (xfs_has_larp(mp)) {
+		error = xfs_attr_use_log_assist(mp, &need_rele);
 		if (error)
 			return error;
 	}
@@ -1101,7 +1101,7 @@ xfs_attr_set(
 out_unlock:
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
 drop_incompat:
-	if (use_logging)
+	if (need_rele)
 		xlog_drop_incompat_feat(mp->m_log);
 	return error;
 
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index edd077e055d5..bd41e0dc95ff 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3879,15 +3879,17 @@ xlog_drop_incompat_feat(
 }
 
 /*
- * Get permission to use log-assisted atomic exchange of file extents.
+ * Get permission to use log-assisted extended attribute updates.
  *
  * 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.
+ * when they're done.  The @need_rele parameter will be set to true if the
+ * caller should drop permission after the call.
  */
 int
 xfs_attr_use_log_assist(
-	struct xfs_mount	*mp)
+	struct xfs_mount	*mp,
+	bool			*need_rele)
 {
 	int			error = 0;
 
@@ -3896,6 +3898,7 @@ xfs_attr_use_log_assist(
 	 * incompat feature bit.
 	 */
 	xlog_use_incompat_feat(mp->m_log);
+	*need_rele = true;
 
 	/*
 	 * If log-assisted xattrs are already enabled, the caller can use the
@@ -3916,5 +3919,6 @@ xfs_attr_use_log_assist(
 	return 0;
 drop_incompat:
 	xlog_drop_incompat_feat(mp->m_log);
+	*need_rele = false;
 	return error;
 }
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index f3ce046a7d45..166d2310af0b 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -160,6 +160,6 @@ 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);
+int xfs_attr_use_log_assist(struct xfs_mount *mp, bool *need_rele);
 
 #endif	/* __XFS_LOG_H__ */

[PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Darrick J. Wong]

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

libxfs itself should never be messing with whether or not to enable
logging for extended attribute updates -- this decision should be made
on a case-by-case basis by libxfs callers.  Move the code that actually
enables the log features to xfs_xattr.c, and adjust the callers.

This removes an awkward coupling point between libxfs and what would be
libxlog, if the XFS log were actually its own library.  Furthermore, it
makes bulk attribute updates and inode security initialization a tiny
bit more efficient, since they now avoid cycling the log feature between
every single xattr.

Signed-off-by: Darrick J. Wong <djwong@kernel.org>
---
 fs/xfs/libxfs/xfs_attr.c |   12 +-------
 fs/xfs/xfs_acl.c         |   10 +++++++
 fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
 fs/xfs/xfs_ioctl.h       |    2 +
 fs/xfs/xfs_ioctl32.c     |    4 ++-
 fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
 fs/xfs/xfs_log.c         |   45 --------------------------------
 fs/xfs/xfs_log.h         |    1 -
 fs/xfs/xfs_super.h       |    2 +
 fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
 10 files changed, 120 insertions(+), 68 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_attr.c b/fs/xfs/libxfs/xfs_attr.c
index 1de3db88e006..682ad9563fe0 100644
--- a/fs/xfs/libxfs/xfs_attr.c
+++ b/fs/xfs/libxfs/xfs_attr.c
@@ -982,7 +982,6 @@ xfs_attr_set(
 	int			error, local;
 	int			rmt_blks = 0;
 	unsigned int		total;
-	bool			need_rele = false;
 
 	if (xfs_is_shutdown(dp->i_mount))
 		return -EIO;
@@ -1027,12 +1026,6 @@ xfs_attr_set(
 		rmt_blks = xfs_attr3_rmt_blocks(mp, XFS_XATTR_SIZE_MAX);
 	}
 
-	if (xfs_has_larp(mp)) {
-		error = xfs_attr_use_log_assist(mp, &need_rele);
-		if (error)
-			return error;
-	}
-
 	/*
 	 * Root fork attributes can use reserved data blocks for this
 	 * operation if necessary
@@ -1040,7 +1033,7 @@ xfs_attr_set(
 	xfs_init_attr_trans(args, &tres, &total);
 	error = xfs_trans_alloc_inode(dp, &tres, total, 0, rsvd, &args->trans);
 	if (error)
-		goto drop_incompat;
+		return error;
 
 	if (args->value || xfs_inode_hasattr(dp)) {
 		error = xfs_iext_count_may_overflow(dp, XFS_ATTR_FORK,
@@ -1100,9 +1093,6 @@ xfs_attr_set(
 	error = xfs_trans_commit(args->trans);
 out_unlock:
 	xfs_iunlock(dp, XFS_ILOCK_EXCL);
-drop_incompat:
-	if (need_rele)
-		xlog_drop_incompat_feat(mp->m_log);
 	return error;
 
 out_trans_cancel:
diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index 3df9c1782ead..b0928175a3f4 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -17,6 +17,7 @@
 #include "xfs_error.h"
 #include "xfs_acl.h"
 #include "xfs_trans.h"
+#include "xfs_log.h"
 
 #include <linux/posix_acl_xattr.h>
 
@@ -174,10 +175,12 @@ int
 __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 {
 	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
 	struct xfs_da_args	args = {
 		.dp		= ip,
 		.attr_filter	= XFS_ATTR_ROOT,
 	};
+	bool			need_rele = false;
 	int			error;
 
 	switch (type) {
@@ -202,8 +205,15 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 		xfs_acl_to_disk(args.value, acl);
 	}
 
+	if (xfs_has_larp(mp)) {
+		error = xfs_xattr_use_log_assist(mp, &need_rele);
+		if (error)
+			return error;
+	}
+
 	error = xfs_attr_set(&args);
 	kmem_free(args.value);
+	xfs_xattr_rele_log_assist(mp, need_rele);
 
 	/*
 	 * If the attribute didn't exist to start with that's fine.
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 0e5cb7936206..b22c7c87ec08 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -37,6 +37,7 @@
 #include "xfs_health.h"
 #include "xfs_reflink.h"
 #include "xfs_ioctl.h"
+#include "xfs_log.h"
 
 #include <linux/mount.h>
 #include <linux/namei.h>
@@ -501,7 +502,8 @@ xfs_attrmulti_attr_set(
 	unsigned char		*name,
 	const unsigned char	__user *ubuf,
 	uint32_t		len,
-	uint32_t		flags)
+	uint32_t		flags,
+	bool			*need_rele)
 {
 	struct xfs_da_args	args = {
 		.dp		= XFS_I(inode),
@@ -510,6 +512,7 @@ xfs_attrmulti_attr_set(
 		.name		= name,
 		.namelen	= strlen(name),
 	};
+	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
 	int			error;
 
 	if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
@@ -524,6 +527,12 @@ xfs_attrmulti_attr_set(
 		args.valuelen = len;
 	}
 
+	if (!(*need_rele) && xfs_has_larp(mp)) {
+		error = xfs_xattr_use_log_assist(mp, need_rele);
+		if (error)
+			return error;
+	}
+
 	error = xfs_attr_set(&args);
 	if (!error && (flags & XFS_IOC_ATTR_ROOT))
 		xfs_forget_acl(inode, name);
@@ -539,7 +548,8 @@ xfs_ioc_attrmulti_one(
 	void __user		*uname,
 	void __user		*value,
 	uint32_t		*len,
-	uint32_t		flags)
+	uint32_t		flags,
+	bool			*need_rele)
 {
 	unsigned char		*name;
 	int			error;
@@ -563,7 +573,8 @@ xfs_ioc_attrmulti_one(
 		error = mnt_want_write_file(parfilp);
 		if (error)
 			break;
-		error = xfs_attrmulti_attr_set(inode, name, value, *len, flags);
+		error = xfs_attrmulti_attr_set(inode, name, value, *len, flags,
+				need_rele);
 		mnt_drop_write_file(parfilp);
 		break;
 	default:
@@ -584,6 +595,7 @@ xfs_attrmulti_by_handle(
 	xfs_attr_multiop_t	*ops;
 	xfs_fsop_attrmulti_handlereq_t am_hreq;
 	struct dentry		*dentry;
+	bool			need_rele = false;
 	unsigned int		i, size;
 
 	if (!capable(CAP_SYS_ADMIN))
@@ -615,8 +627,10 @@ xfs_attrmulti_by_handle(
 		ops[i].am_error = xfs_ioc_attrmulti_one(parfilp,
 				d_inode(dentry), ops[i].am_opcode,
 				ops[i].am_attrname, ops[i].am_attrvalue,
-				&ops[i].am_length, ops[i].am_flags);
+				&ops[i].am_length, ops[i].am_flags,
+				&need_rele);
 	}
+	xfs_xattr_rele_log_assist(XFS_I(d_inode(dentry))->i_mount, need_rele);
 
 	if (copy_to_user(am_hreq.ops, ops, size))
 		error = -EFAULT;
diff --git a/fs/xfs/xfs_ioctl.h b/fs/xfs/xfs_ioctl.h
index d4abba2c13c1..18a7e0853277 100644
--- a/fs/xfs/xfs_ioctl.h
+++ b/fs/xfs/xfs_ioctl.h
@@ -31,7 +31,7 @@ xfs_readlink_by_handle(
 
 int xfs_ioc_attrmulti_one(struct file *parfilp, struct inode *inode,
 		uint32_t opcode, void __user *uname, void __user *value,
-		uint32_t *len, uint32_t flags);
+		uint32_t *len, uint32_t flags, bool *need_rele);
 int xfs_ioc_attr_list(struct xfs_inode *dp, void __user *ubuf,
 		      size_t bufsize, int flags,
 		      struct xfs_attrlist_cursor __user *ucursor);
diff --git a/fs/xfs/xfs_ioctl32.c b/fs/xfs/xfs_ioctl32.c
index 2f54b701eead..a3874ce4469c 100644
--- a/fs/xfs/xfs_ioctl32.c
+++ b/fs/xfs/xfs_ioctl32.c
@@ -372,6 +372,7 @@ xfs_compat_attrmulti_by_handle(
 	compat_xfs_fsop_attrmulti_handlereq_t	am_hreq;
 	struct dentry				*dentry;
 	unsigned int				i, size;
+	bool					need_rele = false;
 
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
@@ -404,8 +405,9 @@ xfs_compat_attrmulti_by_handle(
 				d_inode(dentry), ops[i].am_opcode,
 				compat_ptr(ops[i].am_attrname),
 				compat_ptr(ops[i].am_attrvalue),
-				&ops[i].am_length, ops[i].am_flags);
+				&ops[i].am_length, ops[i].am_flags, &need_rele);
 	}
+	xfs_xattr_rele_log_assist(XFS_I(d_inode(dentry))->i_mount, need_rele);
 
 	if (copy_to_user(compat_ptr(am_hreq.ops), ops, size))
 		error = -EFAULT;
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index e912b7fee714..5a59c1a14344 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -24,6 +24,7 @@
 #include "xfs_iomap.h"
 #include "xfs_error.h"
 #include "xfs_ioctl.h"
+#include "xfs_log.h"
 
 #include <linux/posix_acl.h>
 #include <linux/security.h>
@@ -50,6 +51,8 @@ xfs_initxattrs(
 {
 	const struct xattr	*xattr;
 	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	bool			*need_rele = fs_info;
 	int			error = 0;
 
 	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
@@ -61,6 +64,13 @@ xfs_initxattrs(
 			.value		= xattr->value,
 			.valuelen	= xattr->value_len,
 		};
+
+		if (!(*need_rele) && xfs_has_larp(mp)) {
+			error = xfs_xattr_use_log_assist(mp, need_rele);
+			if (error)
+				return error;
+		}
+
 		error = xfs_attr_set(&args);
 		if (error < 0)
 			break;
@@ -77,12 +87,17 @@ xfs_initxattrs(
 
 STATIC int
 xfs_init_security(
-	struct inode	*inode,
-	struct inode	*dir,
-	const struct qstr *qstr)
+	struct inode		*inode,
+	struct inode		*dir,
+	const struct qstr	*qstr)
 {
-	return security_inode_init_security(inode, dir, qstr,
-					     &xfs_initxattrs, NULL);
+	bool			need_rele = false;
+	int			error;
+
+	error = security_inode_init_security(inode, dir, qstr, &xfs_initxattrs,
+			&need_rele);
+	xfs_xattr_rele_log_assist(XFS_I(inode)->i_mount, need_rele);
+	return error;
 }
 
 static void
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index bd41e0dc95ff..1e972f884a81 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -3877,48 +3877,3 @@ xlog_drop_incompat_feat(
 {
 	up_read(&log->l_incompat_users);
 }
-
-/*
- * Get permission to use log-assisted extended attribute updates.
- *
- * 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.  The @need_rele parameter will be set to true if the
- * caller should drop permission after the call.
- */
-int
-xfs_attr_use_log_assist(
-	struct xfs_mount	*mp,
-	bool			*need_rele)
-{
-	int			error = 0;
-
-	/*
-	 * Protect ourselves from an idle log clearing the logged xattrs log
-	 * incompat feature bit.
-	 */
-	xlog_use_incompat_feat(mp->m_log);
-	*need_rele = true;
-
-	/*
-	 * If log-assisted xattrs are already enabled, the caller can use the
-	 * log assisted swap functions with the log-incompat reference we got.
-	 */
-	if (xfs_sb_version_haslogxattrs(&mp->m_sb))
-		return 0;
-
-	/* Enable log-assisted xattrs. */
-	error = xfs_add_incompat_log_feature(mp,
-			XFS_SB_FEAT_INCOMPAT_LOG_XATTRS);
-	if (error)
-		goto drop_incompat;
-
-	xfs_warn_daily(mp,
- "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
-
-	return 0;
-drop_incompat:
-	xlog_drop_incompat_feat(mp->m_log);
-	*need_rele = false;
-	return error;
-}
diff --git a/fs/xfs/xfs_log.h b/fs/xfs/xfs_log.h
index 166d2310af0b..67b839ccaa38 100644
--- a/fs/xfs/xfs_log.h
+++ b/fs/xfs/xfs_log.h
@@ -160,6 +160,5 @@ 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, bool *need_rele);
 
 #endif	/* __XFS_LOG_H__ */
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index 167d23f92ffe..65f839c5a7cd 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -92,6 +92,8 @@ extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
 
 extern const struct export_operations xfs_export_operations;
 extern const struct xattr_handler *xfs_xattr_handlers[];
+int xfs_xattr_use_log_assist(struct xfs_mount *mp, bool *need_rele);
+void xfs_xattr_rele_log_assist(struct xfs_mount *mp, bool need_rele);
 extern const struct quotactl_ops xfs_quotactl_operations;
 
 extern void xfs_reinit_percpu_counters(struct xfs_mount *mp);
diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
index 7a044afd4c46..356971223f26 100644
--- a/fs/xfs/xfs_xattr.c
+++ b/fs/xfs/xfs_xattr.c
@@ -15,6 +15,7 @@
 #include "xfs_da_btree.h"
 #include "xfs_attr.h"
 #include "xfs_acl.h"
+#include "xfs_log.h"
 
 #include <linux/posix_acl_xattr.h>
 
@@ -39,6 +40,61 @@ xfs_xattr_get(const struct xattr_handler *handler, struct dentry *unused,
 	return args.valuelen;
 }
 
+/*
+ * Get permission to use log-assisted extended attribute updates.
+ *
+ * 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.  The @need_rele parameter will be set to true if the
+ * caller should drop permission after the call.
+ */
+int
+xfs_xattr_use_log_assist(
+	struct xfs_mount	*mp,
+	bool			*need_rele)
+{
+	int			error = 0;
+
+	/*
+	 * Protect ourselves from an idle log clearing the logged xattrs log
+	 * incompat feature bit.
+	 */
+	xlog_use_incompat_feat(mp->m_log);
+	*need_rele = true;
+
+	/*
+	 * If log-assisted xattrs are already enabled, the caller can use the
+	 * log assisted swap functions with the log-incompat reference we got.
+	 */
+	if (xfs_sb_version_haslogxattrs(&mp->m_sb))
+		return 0;
+
+	/* Enable log-assisted xattrs. */
+	error = xfs_add_incompat_log_feature(mp,
+			XFS_SB_FEAT_INCOMPAT_LOG_XATTRS);
+	if (error)
+		goto drop_incompat;
+
+	xfs_warn_daily(mp,
+ "EXPERIMENTAL logged extended attributes feature added. Use at your own risk!");
+
+	return 0;
+drop_incompat:
+	xlog_drop_incompat_feat(mp->m_log);
+	*need_rele = false;
+	return error;
+}
+
+/* Release permission to use log-assisted xattr updates. */
+void
+xfs_xattr_rele_log_assist(
+	struct xfs_mount	*mp,
+	bool			need_rele)
+{
+	if (need_rele)
+		xlog_drop_incompat_feat(mp->m_log);
+}
+
 static int
 xfs_xattr_set(const struct xattr_handler *handler,
 	      struct user_namespace *mnt_userns, struct dentry *unused,
@@ -54,11 +110,20 @@ xfs_xattr_set(const struct xattr_handler *handler,
 		.value		= (void *)value,
 		.valuelen	= size,
 	};
+	struct xfs_mount	*mp = XFS_I(inode)->i_mount;
+	bool			need_rele = false;
 	int			error;
 
+	if (xfs_has_larp(mp)) {
+		error = xfs_xattr_use_log_assist(mp, &need_rele);
+		if (error)
+			return error;
+	}
+
 	error = xfs_attr_set(&args);
 	if (!error && (handler->flags & XFS_ATTR_ROOT))
 		xfs_forget_acl(inode, name);
+	xfs_xattr_rele_log_assist(mp, need_rele);
 	return error;
 }
 

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Dave Chinner]

On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <djwong@kernel.org>
> 
> libxfs itself should never be messing with whether or not to enable
> logging for extended attribute updates -- this decision should be made
> on a case-by-case basis by libxfs callers.  Move the code that actually
> enables the log features to xfs_xattr.c, and adjust the callers.
> 
> This removes an awkward coupling point between libxfs and what would be
> libxlog, if the XFS log were actually its own library.  Furthermore, it
> makes bulk attribute updates and inode security initialization a tiny
> bit more efficient, since they now avoid cycling the log feature between
> every single xattr.
> 
> Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> ---
>  fs/xfs/libxfs/xfs_attr.c |   12 +-------
>  fs/xfs/xfs_acl.c         |   10 +++++++
>  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
>  fs/xfs/xfs_ioctl.h       |    2 +
>  fs/xfs/xfs_ioctl32.c     |    4 ++-
>  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
>  fs/xfs/xfs_log.c         |   45 --------------------------------
>  fs/xfs/xfs_log.h         |    1 -
>  fs/xfs/xfs_super.h       |    2 +
>  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
>  10 files changed, 120 insertions(+), 68 deletions(-)

This seems like the wrong way to approach this. I would have defined
a wrapper function for xfs_attr_set() to do the log state futzing,
not moved it all into callers that don't need (or want) to know
anything about how attrs are logged internally....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Darrick J. Wong]

On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > libxfs itself should never be messing with whether or not to enable
> > logging for extended attribute updates -- this decision should be made
> > on a case-by-case basis by libxfs callers.  Move the code that actually
> > enables the log features to xfs_xattr.c, and adjust the callers.
> > 
> > This removes an awkward coupling point between libxfs and what would be
> > libxlog, if the XFS log were actually its own library.  Furthermore, it
> > makes bulk attribute updates and inode security initialization a tiny
> > bit more efficient, since they now avoid cycling the log feature between
> > every single xattr.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_attr.c |   12 +-------
> >  fs/xfs/xfs_acl.c         |   10 +++++++
> >  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
> >  fs/xfs/xfs_ioctl.h       |    2 +
> >  fs/xfs/xfs_ioctl32.c     |    4 ++-
> >  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
> >  fs/xfs/xfs_log.c         |   45 --------------------------------
> >  fs/xfs/xfs_log.h         |    1 -
> >  fs/xfs/xfs_super.h       |    2 +
> >  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> >  10 files changed, 120 insertions(+), 68 deletions(-)
> 
> This seems like the wrong way to approach this. I would have defined
> a wrapper function for xfs_attr_set() to do the log state futzing,
> not moved it all into callers that don't need (or want) to know
> anything about how attrs are logged internally....

I started doing this, and within a few hours realized that I'd set upon
yet *another* refactoring of xfs_attr_set.  I'm not willing to do that
so soon after Allison's refactoring, so I'm dropping this patch.

--D

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

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Dave Chinner]

On Mon, May 23, 2022 at 12:12:21PM -0700, Darrick J. Wong wrote:
> On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> > On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > libxfs itself should never be messing with whether or not to enable
> > > logging for extended attribute updates -- this decision should be made
> > > on a case-by-case basis by libxfs callers.  Move the code that actually
> > > enables the log features to xfs_xattr.c, and adjust the callers.
> > > 
> > > This removes an awkward coupling point between libxfs and what would be
> > > libxlog, if the XFS log were actually its own library.  Furthermore, it
> > > makes bulk attribute updates and inode security initialization a tiny
> > > bit more efficient, since they now avoid cycling the log feature between
> > > every single xattr.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  fs/xfs/libxfs/xfs_attr.c |   12 +-------
> > >  fs/xfs/xfs_acl.c         |   10 +++++++
> > >  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
> > >  fs/xfs/xfs_ioctl.h       |    2 +
> > >  fs/xfs/xfs_ioctl32.c     |    4 ++-
> > >  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
> > >  fs/xfs/xfs_log.c         |   45 --------------------------------
> > >  fs/xfs/xfs_log.h         |    1 -
> > >  fs/xfs/xfs_super.h       |    2 +
> > >  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> > >  10 files changed, 120 insertions(+), 68 deletions(-)
> > 
> > This seems like the wrong way to approach this. I would have defined
> > a wrapper function for xfs_attr_set() to do the log state futzing,
> > not moved it all into callers that don't need (or want) to know
> > anything about how attrs are logged internally....
> 
> I started doing this, and within a few hours realized that I'd set upon
> yet *another* refactoring of xfs_attr_set.  I'm not willing to do that
> so soon after Allison's refactoring, so I'm dropping this patch.

I don't see why this ends up being a problem - xfs_attr_set() is
only called by code in fs/xfs/*.c, so adding a wrapper function
that just does this:

int
xfs_attr_change(
	struct xfs_da_args      *args)
{
	struct xfs_mount	*mp = args->dp->i_mount;

	if (xfs_has_larp(mp)) {
		error = xfs_attr_use_log_assist(mp);
		if (error)
			return error;
	}

	error = xfs_attr_set(args);
	if (xfs_has_larp(mp))
		xlog_drop_incompat_feat(mp->m_log);
	return error;
}

into one of the files in fs/xfs will get this out of libxfs, won't
it?

What am I missing here?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Darrick J. Wong]

On Tue, May 24, 2022 at 08:56:43AM +1000, Dave Chinner wrote:
> On Mon, May 23, 2022 at 12:12:21PM -0700, Darrick J. Wong wrote:
> > On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> > > On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > libxfs itself should never be messing with whether or not to enable
> > > > logging for extended attribute updates -- this decision should be made
> > > > on a case-by-case basis by libxfs callers.  Move the code that actually
> > > > enables the log features to xfs_xattr.c, and adjust the callers.
> > > > 
> > > > This removes an awkward coupling point between libxfs and what would be
> > > > libxlog, if the XFS log were actually its own library.  Furthermore, it
> > > > makes bulk attribute updates and inode security initialization a tiny
> > > > bit more efficient, since they now avoid cycling the log feature between
> > > > every single xattr.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_attr.c |   12 +-------
> > > >  fs/xfs/xfs_acl.c         |   10 +++++++
> > > >  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
> > > >  fs/xfs/xfs_ioctl.h       |    2 +
> > > >  fs/xfs/xfs_ioctl32.c     |    4 ++-
> > > >  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
> > > >  fs/xfs/xfs_log.c         |   45 --------------------------------
> > > >  fs/xfs/xfs_log.h         |    1 -
> > > >  fs/xfs/xfs_super.h       |    2 +
> > > >  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  10 files changed, 120 insertions(+), 68 deletions(-)
> > > 
> > > This seems like the wrong way to approach this. I would have defined
> > > a wrapper function for xfs_attr_set() to do the log state futzing,
> > > not moved it all into callers that don't need (or want) to know
> > > anything about how attrs are logged internally....
> > 
> > I started doing this, and within a few hours realized that I'd set upon
> > yet *another* refactoring of xfs_attr_set.  I'm not willing to do that
> > so soon after Allison's refactoring, so I'm dropping this patch.
> 
> I don't see why this ends up being a problem - xfs_attr_set() is
> only called by code in fs/xfs/*.c, so adding a wrapper function
> that just does this:
> 
> int
> xfs_attr_change(
> 	struct xfs_da_args      *args)
> {
> 	struct xfs_mount	*mp = args->dp->i_mount;
> 
> 	if (xfs_has_larp(mp)) {
> 		error = xfs_attr_use_log_assist(mp);
> 		if (error)
> 			return error;
> 	}
> 
> 	error = xfs_attr_set(args);
> 	if (xfs_has_larp(mp))

Race condition here ^^^ if we race with someone changing the debug knob,
we'll either drop something we never took, or leak something we did
take.

> 		xlog_drop_incompat_feat(mp->m_log);
> 	return error;
> }
> 
> into one of the files in fs/xfs will get this out of libxfs, won't
> it?
> 
> What am I missing here?

After the last year and a half I've gotten in the bad habit of trying to
anticipate the likely style objections of various reviewers to try to
get patches into upstream with as few objections as possible, which then
leads me down the path of more and more scope creep from the voices
inside my head:

"These cleanups should be split into smaller changes for easy
backporting."

"Setting xattr arguments via the da_args struct is a mess, make them
function parameters."

"It's nasty to have xfs_attr_change take 7 parameters, just make an
xfs_attrchange_args struct with the pieces we need, and use it to fill
out the da args internally."

"These calling conventions are still crap, transaction allocation
shouldn't even be in libxfs at all!"

"Why don't you make attr_change have its own flags namespace, and then
set attr_filter and attr_flags from that?  This would decouple the
interfaces and make them easier to figure out next time."

"There are too many little xfs_attr functions and it's really hard to
grok what they all do."

OTOH if you'd be willing to take just that attr_change bit above (with
the race condition fixed, I *would* send you that in patch form.

--D

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

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Dave Chinner]

On Mon, May 23, 2022 at 05:35:40PM -0700, Darrick J. Wong wrote:
> On Tue, May 24, 2022 at 08:56:43AM +1000, Dave Chinner wrote:
> > On Mon, May 23, 2022 at 12:12:21PM -0700, Darrick J. Wong wrote:
> > > On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> > > > On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > 
> > > > > libxfs itself should never be messing with whether or not to enable
> > > > > logging for extended attribute updates -- this decision should be made
> > > > > on a case-by-case basis by libxfs callers.  Move the code that actually
> > > > > enables the log features to xfs_xattr.c, and adjust the callers.
> > > > > 
> > > > > This removes an awkward coupling point between libxfs and what would be
> > > > > libxlog, if the XFS log were actually its own library.  Furthermore, it
> > > > > makes bulk attribute updates and inode security initialization a tiny
> > > > > bit more efficient, since they now avoid cycling the log feature between
> > > > > every single xattr.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > ---
> > > > >  fs/xfs/libxfs/xfs_attr.c |   12 +-------
> > > > >  fs/xfs/xfs_acl.c         |   10 +++++++
> > > > >  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
> > > > >  fs/xfs/xfs_ioctl.h       |    2 +
> > > > >  fs/xfs/xfs_ioctl32.c     |    4 ++-
> > > > >  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
> > > > >  fs/xfs/xfs_log.c         |   45 --------------------------------
> > > > >  fs/xfs/xfs_log.h         |    1 -
> > > > >  fs/xfs/xfs_super.h       |    2 +
> > > > >  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  10 files changed, 120 insertions(+), 68 deletions(-)
> > > > 
> > > > This seems like the wrong way to approach this. I would have defined
> > > > a wrapper function for xfs_attr_set() to do the log state futzing,
> > > > not moved it all into callers that don't need (or want) to know
> > > > anything about how attrs are logged internally....
> > > 
> > > I started doing this, and within a few hours realized that I'd set upon
> > > yet *another* refactoring of xfs_attr_set.  I'm not willing to do that
> > > so soon after Allison's refactoring, so I'm dropping this patch.
> > 
> > I don't see why this ends up being a problem - xfs_attr_set() is
> > only called by code in fs/xfs/*.c, so adding a wrapper function
> > that just does this:
> > 
> > int
> > xfs_attr_change(
> > 	struct xfs_da_args      *args)
> > {
> > 	struct xfs_mount	*mp = args->dp->i_mount;
> > 
> > 	if (xfs_has_larp(mp)) {
> > 		error = xfs_attr_use_log_assist(mp);
> > 		if (error)
> > 			return error;
> > 	}
> > 
> > 	error = xfs_attr_set(args);
> > 	if (xfs_has_larp(mp))
> 
> Race condition here ^^^ if we race with someone changing the debug knob,
> we'll either drop something we never took, or leak something we did
> take.

True, but largely irrelevant for the purposes of demonstration....

> 
> > 		xlog_drop_incompat_feat(mp->m_log);
> > 	return error;
> > }
> > 
> > into one of the files in fs/xfs will get this out of libxfs, won't
> > it?
> > 
> > What am I missing here?
> 
> After the last year and a half I've gotten in the bad habit of trying to
> anticipate the likely style objections of various reviewers to try to
> get patches into upstream with as few objections as possible, which then
> leads me down the path of more and more scope creep from the voices
> inside my head:
> 
> "These cleanups should be split into smaller changes for easy
> backporting."
>
> "Setting xattr arguments via the da_args struct is a mess, make them
> function parameters."
> 
> "It's nasty to have xfs_attr_change take 7 parameters, just make an
> xfs_attrchange_args struct with the pieces we need, and use it to fill
> out the da args internally."
> 
> "These calling conventions are still crap, transaction allocation
> shouldn't even be in libxfs at all!"
> 
> "Why don't you make attr_change have its own flags namespace, and then
> set attr_filter and attr_flags from that?  This would decouple the
> interfaces and make them easier to figure out next time."
> 
> "There are too many little xfs_attr functions and it's really hard to
> grok what they all do."

Yes, I understand that we need to re-layer the attr code to get rid
of all the twisty passages that are all alike, but we don't need to
do that right now and it avoids tying a simple change up in knots
that prevent progress from being made.

[FWIW, I have a basic plan for that - split all the sf/leaf/node
stuff out of xfs_attr_leaf.c into xfs_attr_sf/leaf/node.c, move all
the sf/leaf/node add/remove/change/lookup stuff out of xfs_attr.c
into the above files. High level API is in xfs_attr.c (same as
xfs_dir.c for directories) and everything external interfaces with
that API. Which we can then tailor to just the information needed
to set up attr and da_args inside xfs_attr.c....

Then we can start cleaning up all the internal attr APIs, fix all
the whacky quirks like sf add will do a remove if it's actually a
replace, clean up the lookup interfaces, etc. ]

> OTOH if you'd be willing to take just that attr_change bit above (with
> the race condition fixed, I *would* send you that in patch form.

Yes, I think that's just fine. Simple is often best...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 5/5] xfs: move xfs_attr_use_log_assist out of libxfs

[Darrick J. Wong]

On Tue, May 24, 2022 at 11:02:49AM +1000, Dave Chinner wrote:
> On Mon, May 23, 2022 at 05:35:40PM -0700, Darrick J. Wong wrote:
> > On Tue, May 24, 2022 at 08:56:43AM +1000, Dave Chinner wrote:
> > > On Mon, May 23, 2022 at 12:12:21PM -0700, Darrick J. Wong wrote:
> > > > On Mon, May 23, 2022 at 01:34:45PM +1000, Dave Chinner wrote:
> > > > > On Sun, May 22, 2022 at 08:28:42AM -0700, Darrick J. Wong wrote:
> > > > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > > > 
> > > > > > libxfs itself should never be messing with whether or not to enable
> > > > > > logging for extended attribute updates -- this decision should be made
> > > > > > on a case-by-case basis by libxfs callers.  Move the code that actually
> > > > > > enables the log features to xfs_xattr.c, and adjust the callers.
> > > > > > 
> > > > > > This removes an awkward coupling point between libxfs and what would be
> > > > > > libxlog, if the XFS log were actually its own library.  Furthermore, it
> > > > > > makes bulk attribute updates and inode security initialization a tiny
> > > > > > bit more efficient, since they now avoid cycling the log feature between
> > > > > > every single xattr.
> > > > > > 
> > > > > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > > > > ---
> > > > > >  fs/xfs/libxfs/xfs_attr.c |   12 +-------
> > > > > >  fs/xfs/xfs_acl.c         |   10 +++++++
> > > > > >  fs/xfs/xfs_ioctl.c       |   22 +++++++++++++---
> > > > > >  fs/xfs/xfs_ioctl.h       |    2 +
> > > > > >  fs/xfs/xfs_ioctl32.c     |    4 ++-
> > > > > >  fs/xfs/xfs_iops.c        |   25 ++++++++++++++----
> > > > > >  fs/xfs/xfs_log.c         |   45 --------------------------------
> > > > > >  fs/xfs/xfs_log.h         |    1 -
> > > > > >  fs/xfs/xfs_super.h       |    2 +
> > > > > >  fs/xfs/xfs_xattr.c       |   65 ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  10 files changed, 120 insertions(+), 68 deletions(-)
> > > > > 
> > > > > This seems like the wrong way to approach this. I would have defined
> > > > > a wrapper function for xfs_attr_set() to do the log state futzing,
> > > > > not moved it all into callers that don't need (or want) to know
> > > > > anything about how attrs are logged internally....
> > > > 
> > > > I started doing this, and within a few hours realized that I'd set upon
> > > > yet *another* refactoring of xfs_attr_set.  I'm not willing to do that
> > > > so soon after Allison's refactoring, so I'm dropping this patch.
> > > 
> > > I don't see why this ends up being a problem - xfs_attr_set() is
> > > only called by code in fs/xfs/*.c, so adding a wrapper function
> > > that just does this:
> > > 
> > > int
> > > xfs_attr_change(
> > > 	struct xfs_da_args      *args)
> > > {
> > > 	struct xfs_mount	*mp = args->dp->i_mount;
> > > 
> > > 	if (xfs_has_larp(mp)) {
> > > 		error = xfs_attr_use_log_assist(mp);
> > > 		if (error)
> > > 			return error;
> > > 	}
> > > 
> > > 	error = xfs_attr_set(args);
> > > 	if (xfs_has_larp(mp))
> > 
> > Race condition here ^^^ if we race with someone changing the debug knob,
> > we'll either drop something we never took, or leak something we did
> > take.
> 
> True, but largely irrelevant for the purposes of demonstration....
> 
> > 
> > > 		xlog_drop_incompat_feat(mp->m_log);
> > > 	return error;
> > > }
> > > 
> > > into one of the files in fs/xfs will get this out of libxfs, won't
> > > it?
> > > 
> > > What am I missing here?
> > 
> > After the last year and a half I've gotten in the bad habit of trying to
> > anticipate the likely style objections of various reviewers to try to
> > get patches into upstream with as few objections as possible, which then
> > leads me down the path of more and more scope creep from the voices
> > inside my head:
> > 
> > "These cleanups should be split into smaller changes for easy
> > backporting."
> >
> > "Setting xattr arguments via the da_args struct is a mess, make them
> > function parameters."
> > 
> > "It's nasty to have xfs_attr_change take 7 parameters, just make an
> > xfs_attrchange_args struct with the pieces we need, and use it to fill
> > out the da args internally."
> > 
> > "These calling conventions are still crap, transaction allocation
> > shouldn't even be in libxfs at all!"
> > 
> > "Why don't you make attr_change have its own flags namespace, and then
> > set attr_filter and attr_flags from that?  This would decouple the
> > interfaces and make them easier to figure out next time."
> > 
> > "There are too many little xfs_attr functions and it's really hard to
> > grok what they all do."
> 
> Yes, I understand that we need to re-layer the attr code to get rid
> of all the twisty passages that are all alike, but we don't need to
> do that right now and it avoids tying a simple change up in knots
> that prevent progress from being made.
> 
> [FWIW, I have a basic plan for that - split all the sf/leaf/node
> stuff out of xfs_attr_leaf.c into xfs_attr_sf/leaf/node.c, move all
> the sf/leaf/node add/remove/change/lookup stuff out of xfs_attr.c
> into the above files. High level API is in xfs_attr.c (same as
> xfs_dir.c for directories) and everything external interfaces with
> that API. Which we can then tailor to just the information needed
> to set up attr and da_args inside xfs_attr.c....
> 
> Then we can start cleaning up all the internal attr APIs, fix all
> the whacky quirks like sf add will do a remove if it's actually a
> replace, clean up the lookup interfaces, etc. ]
> 
> > OTOH if you'd be willing to take just that attr_change bit above (with
> > the race condition fixed, I *would* send you that in patch form.
> 
> Yes, I think that's just fine. Simple is often best...

Ok.  Since I only just noticed the for-next update, I'll rebase on that,
make this change, and send that out for testing overnight.

--D

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