diff mbox series

[RFC,v2,2/4] jbd2: introduce some new log interfaces

Message ID f19b925451351040a7e831bb1c96f062421c8ce8.1611402263.git.brookxu@tencent.com
State Superseded
Headers show
Series make jbd2 debug switch per device | expand

Commit Message

brookxu Jan. 23, 2021, noon UTC 
From: Chunguang Xu <brookxu@tencent.com>

Compared to directly using numbers to indicate levels, using abstract
error, warn, notice, info, debug to indicate levels may be more
convenient for code reading and writing. Similar to other kernel
modules, some basic log interfaces are introduced.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 include/linux/jbd2.h | 59 +++++++++++++++++++++++++++++++-------------
 1 file changed, 42 insertions(+), 17 deletions(-)

Comments

Jan Kara Jan. 25, 2021, 2:54 p.m. UTC | #1 
On Sat 23-01-21 20:00:44, Chunguang Xu wrote:
> From: Chunguang Xu <brookxu@tencent.com>
> 
> Compared to directly using numbers to indicate levels, using abstract
> error, warn, notice, info, debug to indicate levels may be more
> convenient for code reading and writing. Similar to other kernel
> modules, some basic log interfaces are introduced.
> 
> Signed-off-by: Chunguang Xu <brookxu@tencent.com>

One more thing I've noticed when reading this patch:

> +
> +#ifdef CONFIG_JBD2_DEBUG
> +/*
> + * Define JBD2_EXPENSIVE_CHECKING to enable more expensive internal
> + * consistency checks.  By default we don't do this unless
> + * CONFIG_JBD2_DEBUG is on.
> + */
> +#define JBD2_EXPENSIVE_CHECKING
> +extern ushort jbd2_journal_enable_debug;
> +void jbd2_log(int level, journal_t *j, const char *file, const char *func,
> +		      unsigned int line, const char *fmt, ...);
> +
> +#define JBD2_ERR	1	/* error conditions */
> +#define JBD2_WARN	2	/* warning conditions */
> +#define JBD2_NOTICE	3	/* normal but significant condition */
> +#define JBD2_INFO	4	/* informational */
> +#define JBD2_DEBUG	5	/* debug-level messages */

This is actually not true. All the jbd_debug() messages are really debug
messages, not errors, not warnings. It is just a different level of detail.
Honestly, these days, I'd rather discard all the levels, use pr_debug()
function to print these messages inside jdb2_debug() and defer to
CONFIG_DYNAMIC_DEBUG framework for configuration of which messages are
interesting for a particular debug session.

								Honza

> +
> +#define jbd2_err(j, fmt, a...)						\
> +	jbd2_log(JBD2_ERR, j, __FILE__, __func__, __LINE__, (fmt), ##a)
> +
> +#define jbd2_warn(j, fmt, a...)						\
> +	jbd2_log(JBD2_WARN, j, __FILE__, __func__, __LINE__, (fmt), ##a)
> +
> +#define jbd2_notice(j, fmt, a...)					\
> +	jbd2_log(JBD2_NOTICE, j, __FILE__, __func__, __LINE__, (fmt), ##a)
> +
> +#define jbd2_info(j, fmt, a...)						\
> +	jbd2_log(JBD2_INFO, j, __FILE__, __func__, __LINE__, (fmt), ##a)
> +
> +#define jbd2_debug(j, fmt, a...)					\
> +	jbd2_log(JBD2_DEBUG, j, __FILE__, __func__, __LINE__, (fmt), ##a)
> +
> +#else
> +
> +#define jbd2_err(j, fmt, a...)
> +#define jbd2_warn(j, fmt, a...)
> +#define jbd2_notice(j, fmt, a...)
> +#define jbd2_info(j, fmt, a...)
> +#define jbd2_debug(j, fmt, a...)
> +
> +#endif
>  #endif
>  
>  /*
> -- 
> 2.30.0
>
brookxu Jan. 25, 2021, 3:53 p.m. UTC | #2 
Jan Kara wrote on 2021/1/25 22:54:
> On Sat 23-01-21 20:00:44, Chunguang Xu wrote:
>> From: Chunguang Xu <brookxu@tencent.com>
>>
>> Compared to directly using numbers to indicate levels, using abstract
>> error, warn, notice, info, debug to indicate levels may be more
>> convenient for code reading and writing. Similar to other kernel
>> modules, some basic log interfaces are introduced.
>>
>> Signed-off-by: Chunguang Xu <brookxu@tencent.com>
> 
> One more thing I've noticed when reading this patch:
> 
>> +
>> +#ifdef CONFIG_JBD2_DEBUG
>> +/*
>> + * Define JBD2_EXPENSIVE_CHECKING to enable more expensive internal
>> + * consistency checks.  By default we don't do this unless
>> + * CONFIG_JBD2_DEBUG is on.
>> + */
>> +#define JBD2_EXPENSIVE_CHECKING
>> +extern ushort jbd2_journal_enable_debug;
>> +void jbd2_log(int level, journal_t *j, const char *file, const char *func,
>> +		      unsigned int line, const char *fmt, ...);
>> +
>> +#define JBD2_ERR	1	/* error conditions */
>> +#define JBD2_WARN	2	/* warning conditions */
>> +#define JBD2_NOTICE	3	/* normal but significant condition */
>> +#define JBD2_INFO	4	/* informational */
>> +#define JBD2_DEBUG	5	/* debug-level messages */
> 
> This is actually not true. All the jbd_debug() messages are really debug
> messages, not errors, not warnings. It is just a different level of detail.
> Honestly, these days, I'd rather discard all the levels, use pr_debug()
> function to print these messages inside jdb2_debug() and defer to
> CONFIG_DYNAMIC_DEBUG framework for configuration of which messages are
> interesting for a particular debug session.

From a certain point, this is indeed, maybe the changes here are not necessary.
Thanks.

> 								Honza
> 
>> +
>> +#define jbd2_err(j, fmt, a...)						\
>> +	jbd2_log(JBD2_ERR, j, __FILE__, __func__, __LINE__, (fmt), ##a)
>> +
>> +#define jbd2_warn(j, fmt, a...)						\
>> +	jbd2_log(JBD2_WARN, j, __FILE__, __func__, __LINE__, (fmt), ##a)
>> +
>> +#define jbd2_notice(j, fmt, a...)					\
>> +	jbd2_log(JBD2_NOTICE, j, __FILE__, __func__, __LINE__, (fmt), ##a)
>> +
>> +#define jbd2_info(j, fmt, a...)						\
>> +	jbd2_log(JBD2_INFO, j, __FILE__, __func__, __LINE__, (fmt), ##a)
>> +
>> +#define jbd2_debug(j, fmt, a...)					\
>> +	jbd2_log(JBD2_DEBUG, j, __FILE__, __func__, __LINE__, (fmt), ##a)
>> +
>> +#else
>> +
>> +#define jbd2_err(j, fmt, a...)
>> +#define jbd2_warn(j, fmt, a...)
>> +#define jbd2_notice(j, fmt, a...)
>> +#define jbd2_info(j, fmt, a...)
>> +#define jbd2_debug(j, fmt, a...)
>> +
>> +#endif
>>  #endif
>>  
>>  /*
>> -- 
>> 2.30.0
>>
diff mbox series

Patch

diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 600a2ea8324a..b33bb5681cae 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -47,23 +47,6 @@ 
  */
 #define JBD2_DEFAULT_MAX_COMMIT_AGE 5
 
-#ifdef CONFIG_JBD2_DEBUG
-/*
- * Define JBD2_EXPENSIVE_CHECKING to enable more expensive internal
- * consistency checks.  By default we don't do this unless
- * CONFIG_JBD2_DEBUG is on.
- */
-#define JBD2_EXPENSIVE_CHECKING
-extern ushort jbd2_journal_enable_debug;
-void __jbd2_debug(int level, const char *file, const char *func,
-		  unsigned int line, const char *fmt, ...);
-
-#define jbd_debug(n, fmt, a...) \
-	__jbd2_debug((n), __FILE__, __func__, __LINE__, (fmt), ##a)
-#else
-#define jbd_debug(n, fmt, a...)    /**/
-#endif
-
 extern void *jbd2_alloc(size_t size, gfp_t flags);
 extern void jbd2_free(void *ptr, size_t size);
 
@@ -104,6 +87,48 @@  typedef struct jbd2_journal_handle handle_t;	/* Atomic operation type */
  * This is an opaque datatype.
  **/
 typedef struct journal_s	journal_t;	/* Journal control structure */
+
+#ifdef CONFIG_JBD2_DEBUG
+/*
+ * Define JBD2_EXPENSIVE_CHECKING to enable more expensive internal
+ * consistency checks.  By default we don't do this unless
+ * CONFIG_JBD2_DEBUG is on.
+ */
+#define JBD2_EXPENSIVE_CHECKING
+extern ushort jbd2_journal_enable_debug;
+void jbd2_log(int level, journal_t *j, const char *file, const char *func,
+		      unsigned int line, const char *fmt, ...);
+
+#define JBD2_ERR	1	/* error conditions */
+#define JBD2_WARN	2	/* warning conditions */
+#define JBD2_NOTICE	3	/* normal but significant condition */
+#define JBD2_INFO	4	/* informational */
+#define JBD2_DEBUG	5	/* debug-level messages */
+
+#define jbd2_err(j, fmt, a...)						\
+	jbd2_log(JBD2_ERR, j, __FILE__, __func__, __LINE__, (fmt), ##a)
+
+#define jbd2_warn(j, fmt, a...)						\
+	jbd2_log(JBD2_WARN, j, __FILE__, __func__, __LINE__, (fmt), ##a)
+
+#define jbd2_notice(j, fmt, a...)					\
+	jbd2_log(JBD2_NOTICE, j, __FILE__, __func__, __LINE__, (fmt), ##a)
+
+#define jbd2_info(j, fmt, a...)						\
+	jbd2_log(JBD2_INFO, j, __FILE__, __func__, __LINE__, (fmt), ##a)
+
+#define jbd2_debug(j, fmt, a...)					\
+	jbd2_log(JBD2_DEBUG, j, __FILE__, __func__, __LINE__, (fmt), ##a)
+
+#else
+
+#define jbd2_err(j, fmt, a...)
+#define jbd2_warn(j, fmt, a...)
+#define jbd2_notice(j, fmt, a...)
+#define jbd2_info(j, fmt, a...)
+#define jbd2_debug(j, fmt, a...)
+
+#endif
 #endif
 
 /*