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

[Chris Down <chris@chrisdown.name>]

Hi Petr,

Petr Mladek writes:
>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: ".

I think we do actually want "%sXFS%s: " here. Without that, it's not possible to 
be confident marrying up a userspace detector to its original printk 
counterpart if the detector actually looks at what's in mp->m_super->s_id (eg.  
to exclude or include some device).

Some messages in practice also typically only ever come out with mp->m_super 
present, so the userspace detector is likely to accomodate for that whether it 
uses the data in mp->m_super->s_id or not. Since we can't detect which printks 
those are at compile time, we pretty much have to use "%sXFS%s: ".

Thanks,

Chris