Re: [PATCH v3 2/2] Add XFS messages to printk index

[Petr Mladek <pmladek@suse.com>]

On Fri 2022-03-25 10:19:46, Jonathan Lassoff wrote:
> In order for end users to quickly react to new issues that come up in
> production, it is proving useful to leverage the printk indexing system.
> This printk index enables kernel developers to use calls to printk()
> with changeable ad-hoc format strings, while still enabling end users
> to detect changes from release to release.
> 
> So that detailed XFS messages are captures by this printk index, this
> patch wraps the xfs_<level> and xfs_alert_tag functions.
> 
> Signed-off-by: Jonathan Lassoff <jof@thejof.com>

> --- a/fs/xfs/xfs_message.h
> +++ b/fs/xfs/xfs_message.h
> @@ -6,34 +6,45 @@
>  
>  struct xfs_mount;
>  
> +#define xfs_printk_index_wrap(kern_level, mp, fmt, ...)		\
> +({								\
> +	printk_index_subsys_emit("%sXFS%s: ", kern_level, fmt);	\

I would probably use "%sXFS: " for the first parameter as
a compromise here.

It affects how the printk formats are shown in debugfs. With the
current patch I see in /sys/kernel/debug/printk/index/vmlinux:

<4> fs/xfs/libxfs/xfs_ag.c:877 xfs_ag_shrink_space "%sXFS%s: Error %d reserving per-AG metadata reserve pool."
<1> fs/xfs/libxfs/xfs_ag.c:151 xfs_initialize_perag_data "%sXFS%s: AGF corruption. Please run xfs_repair."
<4> fs/xfs/libxfs/xfs_alloc.c:2429 xfs_agfl_reset "%sXFS%s: WARNING: Reset corrupted AGFL on AG %u. %d blocks leaked. Please unmount and run xfs_repair."
<4> fs/xfs/libxfs/xfs_alloc.c:262 xfs_alloc_get_rec "%sXFS%s: start block 0x%x block count 0x%x"
<4> fs/xfs/libxfs/xfs_alloc.c:260 xfs_alloc_get_rec "%sXFS%s: %s Freespace BTree record corruption in AG %d detected!"
<1> fs/xfs/libxfs/xfs_attr_remote.c:304 xfs_attr_rmtval_copyout "%sXFS%s: remote attribute header mismatch bno/off/len/owner (0x%llx/0x%x/Ox%x/0x%llx)"
<4> fs/xfs/libxfs/xfs_bmap.c:1129 xfs_iread_bmbt_block "%sXFS%s: corrupt dinode %llu, (btree extents)."

In reality, the prefix is chosen in __xfs_printk() at runtime:

	+ "%sXFS (%s): "	when mp->m_super is defined
	+ "%sXFS: "		otherwise

It means that "%sXFS: " is not perfect but it looks closer to reality
than "%sXFS%s: ".


Otherwise, the patch looks good to me. Feel free to use:

Reviewed-by: Petr Mladek <pmladek@suse.com>

Best Regards,
Petr