[PATCH 0/3] Simplify message helpers (avoid level parsing)

[PATCH 0/3] Simplify message helpers (avoid level parsing)

[David Sterba]

Based on patch updating memcpy/strscpy use in printk level parsing
https://lore.kernel.org/linux-btrfs/20251130005518.82065-1-thorsten.blum@linux.dev/
it'll be better to avoid the parsing completely. This patchset updates
the internal interfaces and passes loglevel directly to the helpers so
we can print the description and pick the ratelimit state.

David Sterba (3):
  btrfs: simplify internal btrfs_printk helpers
  btrfs: pass level to _btrfs_printk() to avoid parsing level from
    string
  btrfs: remove ASSERT compatibility for gcc < 8.x

 fs/btrfs/messages.c | 26 ++++------------
 fs/btrfs/messages.h | 76 ++++++++++++++++++++-------------------------
 2 files changed, 39 insertions(+), 63 deletions(-)

-- 
2.51.1

[PATCH 1/3] btrfs: simplify internal btrfs_printk helpers

[David Sterba]

The printk() can be compiled out depending on CONFIG_PRINTK, this is
reflected in our helpers. The indirection is provided by btrfs_printk()
used in the ratelimited and RCU wrapper macros.

Drop the btrfs_printk() helper and define the ratelimit and RCU helpers
directly when CONFIG_PRINTK is undefined. This will allow further
changes to the _btrfs_printk() interface (which is internal), any
message in other code should use the level-specific helpers.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/messages.h | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index d8c0bd17dcdaf0..7049976342a57a 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -23,9 +23,6 @@ void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 
 #ifdef CONFIG_PRINTK
 
-#define btrfs_printk(fs_info, fmt, args...)				\
-	_btrfs_printk(fs_info, fmt, ##args)
-
 __printf(2, 3)
 __cold
 void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
@@ -34,6 +31,13 @@ void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
 
 #define btrfs_printk(fs_info, fmt, args...) \
 	btrfs_no_printk(fs_info, fmt, ##args)
+
+#define btrfs_printk_in_rcu(fs_info, fmt, args...)			\
+	btrfs_no_printk(fs_info, fmt, ##args)
+
+#define btrfs_printk_rl_in_rcu(fs_info, fmt, args...)			\
+	btrfs_no_printk(fs_info, fmt, ##args)
+
 #endif
 
 /*
@@ -78,10 +82,12 @@ void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
 #define btrfs_debug_rl(fs_info, fmt, args...)	do { (void)(fs_info); } while(0)
 #endif
 
+#ifdef CONFIG_PRINTK
+
 #define btrfs_printk_in_rcu(fs_info, fmt, args...)	\
 do {							\
 	rcu_read_lock();				\
-	btrfs_printk(fs_info, fmt, ##args);		\
+	_btrfs_printk(fs_info, fmt, ##args);		\
 	rcu_read_unlock();				\
 } while (0)
 
@@ -93,10 +99,12 @@ do {								\
 								\
 	rcu_read_lock();					\
 	if (__ratelimit(&_rs))					\
-		btrfs_printk(fs_info, fmt, ##args);		\
+		_btrfs_printk(fs_info, fmt, ##args);		\
 	rcu_read_unlock();					\
 } while (0)
 
+#endif
+
 #ifdef CONFIG_BTRFS_ASSERT
 
 __printf(1, 2)
-- 
2.51.1

[PATCH 2/3] btrfs: pass level to _btrfs_printk() to avoid parsing level from string

[David Sterba]

There's code in _btrfs_printk() to parse the message level from the
input string so we can augment the message with the level description
for better visibility in the logs.

The parsing code has evolved over time, see commits:

- 40f7828b36e3b9 ("btrfs: better handle btrfs_printk() defaults")

- 262c5e86fec7cf ("printk/btrfs: handle more message headers")

- 533574c6bc30cf ("btrfs: use printk_get_level and printk_skip_level, add __printf, fix fallout")

- 4da35113426d16 ("btrfs: add varargs to btrfs_error")

As we are using the specific level helpers everywhere we can simply pass
the message level so we don't have to parse it. The proper printk()
message header is created as KERN_SOH + "level".

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/messages.c | 26 ++++++------------------
 fs/btrfs/messages.h | 49 ++++++++++++++++++++++-----------------------
 2 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
index 2f853de4447398..6190777924bff5 100644
--- a/fs/btrfs/messages.c
+++ b/fs/btrfs/messages.c
@@ -211,33 +211,19 @@ static struct ratelimit_state printk_limits[] = {
 	RATELIMIT_STATE_INIT(printk_limits[7], DEFAULT_RATELIMIT_INTERVAL, 100),
 };
 
-void __cold _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
+__printf(3, 4) __cold
+void _btrfs_printk(const struct btrfs_fs_info *fs_info, unsigned int level, const char *fmt, ...)
 {
-	char lvl[PRINTK_MAX_SINGLE_HEADER_LEN + 1] = "\0";
 	struct va_format vaf;
 	va_list args;
-	int kern_level;
-	const char *type = logtypes[4];
-	struct ratelimit_state *ratelimit = &printk_limits[4];
+	const char *type = logtypes[level];
+	struct ratelimit_state *ratelimit = &printk_limits[level];
 
 #ifdef CONFIG_PRINTK_INDEX
 	printk_index_subsys_emit("%sBTRFS %s (device %s): ", NULL, fmt);
 #endif
 
 	va_start(args, fmt);
-
-	while ((kern_level = printk_get_level(fmt)) != 0) {
-		size_t size = printk_skip_level(fmt) - fmt;
-
-		if (kern_level >= '0' && kern_level <= '7') {
-			memcpy(lvl, fmt,  size);
-			lvl[size] = '\0';
-			type = logtypes[kern_level - '0'];
-			ratelimit = &printk_limits[kern_level - '0'];
-		}
-		fmt += size;
-	}
-
 	vaf.fmt = fmt;
 	vaf.va = &args;
 
@@ -247,10 +233,10 @@ void __cold _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt,
 			char statestr[STATE_STRING_BUF_LEN];
 
 			btrfs_state_to_string(fs_info, statestr);
-			_printk("%sBTRFS %s (device %s%s): %pV\n", lvl, type,
+			_printk(KERN_SOH "%dBTRFS %s (device %s%s): %pV\n", level, type,
 				fs_info->sb->s_id, statestr, &vaf);
 		} else {
-			_printk("%sBTRFS %s: %pV\n", lvl, type, &vaf);
+			_printk(KERN_SOH "%dBTRFS %s: %pV\n", level, type, &vaf);
 		}
 	}
 
diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index 7049976342a57a..d4e4cad0609255 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -23,19 +23,18 @@ void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
 
 #ifdef CONFIG_PRINTK
 
-__printf(2, 3)
-__cold
-void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
+__printf(3, 4) __cold
+void _btrfs_printk(const struct btrfs_fs_info *fs_info, unsigned int level, const char *fmt, ...);
 
 #else
 
-#define btrfs_printk(fs_info, fmt, args...) \
+#define btrfs_printk_in_rcu(fs_info, level, fmt, args...)		\
 	btrfs_no_printk(fs_info, fmt, ##args)
 
-#define btrfs_printk_in_rcu(fs_info, fmt, args...)			\
+#define btrfs_printk_in_rcu(fs_info, level, fmt, args...)		\
 	btrfs_no_printk(fs_info, fmt, ##args)
 
-#define btrfs_printk_rl_in_rcu(fs_info, fmt, args...)			\
+#define btrfs_printk_rl_in_rcu(fs_info, level, fmt, args...)		\
 	btrfs_no_printk(fs_info, fmt, ##args)
 
 #endif
@@ -44,38 +43,38 @@ void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
  * Print a message with filesystem info, enclosed in RCU protection.
  */
 #define btrfs_crit(fs_info, fmt, args...) \
-	btrfs_printk_in_rcu(fs_info, KERN_CRIT fmt, ##args)
+	btrfs_printk_in_rcu(fs_info, LOGLEVEL_CRIT, fmt, ##args)
 #define btrfs_err(fs_info, fmt, args...) \
-	btrfs_printk_in_rcu(fs_info, KERN_ERR fmt, ##args)
+	btrfs_printk_in_rcu(fs_info, LOGLEVEL_ERR, fmt, ##args)
 #define btrfs_warn(fs_info, fmt, args...) \
-	btrfs_printk_in_rcu(fs_info, KERN_WARNING fmt, ##args)
+	btrfs_printk_in_rcu(fs_info, LOGLEVEL_WARNING, fmt, ##args)
 #define btrfs_info(fs_info, fmt, args...) \
-	btrfs_printk_in_rcu(fs_info, KERN_INFO fmt, ##args)
+	btrfs_printk_in_rcu(fs_info, LOGLEVEL_INFO, fmt, ##args)
 
 /*
  * Wrappers that use a ratelimited printk
  */
 #define btrfs_crit_rl(fs_info, fmt, args...) \
-	btrfs_printk_rl_in_rcu(fs_info, KERN_CRIT fmt, ##args)
+	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEL_CRIT, fmt, ##args)
 #define btrfs_err_rl(fs_info, fmt, args...) \
-	btrfs_printk_rl_in_rcu(fs_info, KERN_ERR fmt, ##args)
+	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEL_ERR, fmt, ##args)
 #define btrfs_warn_rl(fs_info, fmt, args...) \
-	btrfs_printk_rl_in_rcu(fs_info, KERN_WARNING fmt, ##args)
+	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEL_WARNING, fmt, ##args)
 #define btrfs_info_rl(fs_info, fmt, args...) \
-	btrfs_printk_rl_in_rcu(fs_info, KERN_INFO fmt, ##args)
+	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEL_INFO, fmt, ##args)
 
 #if defined(CONFIG_DYNAMIC_DEBUG)
 #define btrfs_debug(fs_info, fmt, args...)				\
 	_dynamic_func_call_no_desc(fmt, btrfs_printk_in_rcu,		\
-				   fs_info, KERN_DEBUG fmt, ##args)
+				   fs_info, LOGLEVEL_DEBUG, fmt, ##args)
 #define btrfs_debug_rl(fs_info, fmt, args...)				\
 	_dynamic_func_call_no_desc(fmt, btrfs_printk_rl_in_rcu,		\
-				   fs_info, KERN_DEBUG fmt, ##args)
+				   fs_info, LOGLEVEL_DEBUG, fmt, ##args)
 #elif defined(DEBUG)
 #define btrfs_debug(fs_info, fmt, args...) \
-	btrfs_printk_in_rcu(fs_info, KERN_DEBUG fmt, ##args)
+	btrfs_printk_in_rcu(fs_info, LOGLEVEL_DEBUG, fmt, ##args)
 #define btrfs_debug_rl(fs_info, fmt, args...) \
-	btrfs_printk_rl_in_rcu(fs_info, KERN_DEBUG fmt, ##args)
+	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEl_DEBUG, fmt, ##args)
 #else
 /* When printk() is no_printk(), expand to no-op. */
 #define btrfs_debug(fs_info, fmt, args...)	do { (void)(fs_info); } while(0)
@@ -84,14 +83,14 @@ void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
 
 #ifdef CONFIG_PRINTK
 
-#define btrfs_printk_in_rcu(fs_info, fmt, args...)	\
-do {							\
-	rcu_read_lock();				\
-	_btrfs_printk(fs_info, fmt, ##args);		\
-	rcu_read_unlock();				\
+#define btrfs_printk_in_rcu(fs_info, level, fmt, args...)	\
+do {								\
+	rcu_read_lock();					\
+	_btrfs_printk(fs_info, level, fmt, ##args);		\
+	rcu_read_unlock();					\
 } while (0)
 
-#define btrfs_printk_rl_in_rcu(fs_info, fmt, args...)		\
+#define btrfs_printk_rl_in_rcu(fs_info, level, fmt, args...)	\
 do {								\
 	static DEFINE_RATELIMIT_STATE(_rs,			\
 		DEFAULT_RATELIMIT_INTERVAL,			\
@@ -99,7 +98,7 @@ do {								\
 								\
 	rcu_read_lock();					\
 	if (__ratelimit(&_rs))					\
-		_btrfs_printk(fs_info, fmt, ##args);		\
+		_btrfs_printk(fs_info, level, fmt, ##args);	\
 	rcu_read_unlock();					\
 } while (0)
 
-- 
2.51.1

Re: [PATCH 2/3] btrfs: pass level to _btrfs_printk() to avoid parsing level from string

[Chris Mason]

David Sterba <dsterba@suse.com> wrote:
> There's code in _btrfs_printk() to parse the message level from the
> input string so we can augment the message with the level description
> for better visibility in the logs.

Hi everyone,

I'm running the btrfs for-next branch, along with the encryption patch set
through my AI review prompts:

https://github.com/masoncl/review-prompts

I'm working on some btrfs specific instructions, but hopefully these reviews
are already useful.  I've kept the headers to make it easier to double check
that I'm actually replying to the right email, but please let me know if any
parts of this are incorrect:

commit 01c8634c695d199de4a4669c5d684bbec148e71b
Author: David Sterba <dsterba@suse.com>

btrfs: pass level to _btrfs_printk() to avoid parsing level from string

This commit refactors the btrfs printk helpers to pass the message level
directly instead of parsing it from the format string, simplifying the
implementation by removing the parsing code from _btrfs_printk().

> diff --git a/fs/btrfs/messages.c b/fs/btrfs/messages.c
> index 2f853de444739..6190777924bff 100644
> --- a/fs/btrfs/messages.c
> +++ b/fs/btrfs/messages.c

[ ... ]

> diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
> index 7049976342a57..d4e4cad060925 100644
> --- a/fs/btrfs/messages.h
> +++ b/fs/btrfs/messages.h
> @@ -23,19 +23,18 @@ void btrfs_no_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...)
>
>  #ifdef CONFIG_PRINTK
>
> -__printf(2, 3)
> -__cold
> -void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);
> +__printf(3, 4) __cold
> +void _btrfs_printk(const struct btrfs_fs_info *fs_info, unsigned int level, const char *fmt, ...);
>
>  #else
>
> -#define btrfs_printk(fs_info, fmt, args...) \
> +#define btrfs_printk_in_rcu(fs_info, level, fmt, args...)		\
>  	btrfs_no_printk(fs_info, fmt, ##args)
>
> -#define btrfs_printk_in_rcu(fs_info, fmt, args...)			\
> +#define btrfs_printk_in_rcu(fs_info, level, fmt, args...)		\
>  	btrfs_no_printk(fs_info, fmt, ##args)

Does this define btrfs_printk_in_rcu twice? Looking at the original code, it
had btrfs_printk and btrfs_printk_in_rcu as separate macros, but both were
renamed to btrfs_printk_in_rcu in the refactoring.

>
> -#define btrfs_printk_rl_in_rcu(fs_info, fmt, args...)			\
> +#define btrfs_printk_rl_in_rcu(fs_info, level, fmt, args...)		\
>  	btrfs_no_printk(fs_info, fmt, ##args)

[ ... ]

> @@ -44,38 +43,38 @@ void _btrfs_printk(const struct btrfs_fs_info *fs_info, const char *fmt, ...);

[ ... ]

>  #if defined(CONFIG_DYNAMIC_DEBUG)
>  #define btrfs_debug(fs_info, fmt, args...)				\
>  	_dynamic_func_call_no_desc(fmt, btrfs_printk_in_rcu,		\
> -				   fs_info, KERN_DEBUG fmt, ##args)
> +				   fs_info, LOGLEVEL_DEBUG, fmt, ##args)
>  #define btrfs_debug_rl(fs_info, fmt, args...)				\
>  	_dynamic_func_call_no_desc(fmt, btrfs_printk_rl_in_rcu,		\
> -				   fs_info, KERN_DEBUG fmt, ##args)
> +				   fs_info, LOGLEVEL_DEBUG, fmt, ##args)
>  #elif defined(DEBUG)
>  #define btrfs_debug(fs_info, fmt, args...) \
> -	btrfs_printk_in_rcu(fs_info, KERN_DEBUG fmt, ##args)
> +	btrfs_printk_in_rcu(fs_info, LOGLEVEL_DEBUG, fmt, ##args)
>  #define btrfs_debug_rl(fs_info, fmt, args...) \
> -	btrfs_printk_rl_in_rcu(fs_info, KERN_DEBUG fmt, ##args)
> +	btrfs_printk_rl_in_rcu(fs_info, LOGLEVEl_DEBUG, fmt, ##args)
                                                ^^^^^^^^^^^^^^

Is the spelling correct here? It looks like "LOGLEVEl_DEBUG" has a lowercase
'l' before "_DEBUG" instead of "LOGLEVEL_DEBUG". This would break compilation
when DEBUG is defined but CONFIG_DYNAMIC_DEBUG is not.

>  #else
>  /* When printk() is no_printk(), expand to no-op. */
>  #define btrfs_debug(fs_info, fmt, args...)	do { (void)(fs_info); } while(0)

[ ... ]

[PATCH 3/3] btrfs: remove ASSERT compatibility for gcc < 8.x

[David Sterba]

The minimum gcc version is 8 since 118c40b7b50340 ("kbuild: require
gcc-8 and binutils-2.30"), the workaround for missing __VA_OPT__ support
is not needed.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/messages.h | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/fs/btrfs/messages.h b/fs/btrfs/messages.h
index d4e4cad0609255..943e53980945ea 100644
--- a/fs/btrfs/messages.h
+++ b/fs/btrfs/messages.h
@@ -120,7 +120,6 @@ static inline void verify_assert_printk_format(const char *fmt, ...) {
  */
 #define __REST_ARGS(_, ... ) __VA_OPT__(,) __VA_ARGS__
 
-#if defined(CONFIG_CC_IS_CLANG) || GCC_VERSION >= 80000
 /*
  * Assertion with optional printk() format.
  *
@@ -158,22 +157,6 @@ do {										\
 	}									\
 } while(0)
 
-#else
-
-/* For GCC < 8.x only the simple output. */
-
-#define ASSERT(cond, args...)							\
-do {										\
-	verify_assert_printk_format("check the format string" args);		\
-	if (!likely(cond)) {							\
-		pr_err("assertion failed: %s :: %ld, in %s:%d\n",		\
-			#cond, (long)(cond), __FILE__, __LINE__);		\
-		BUG();								\
-	}									\
-} while(0)
-
-#endif
-
 #else
 /* Compile check the @cond expression but don't generate any code. */
 #define ASSERT(cond, args...)			BUILD_BUG_ON_INVALID(cond)
-- 
2.51.1