Re:Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node

[李扬韬 <frank.li@vivo.com>]

>> HI Chao,
>>>> SBI_NEED_FSCK is an indicator that fsck.f2fs needs to be triggered,
>>>> this flag is set in too many places. For some scenes that are not very
>>>> reproducible, adding stack information will help locate the problem.
>>>>
>>>> Let's record all fsck stack history, I added F2FS_FSCK_STACK_TRACE
>>>> configuration options and sysfs nodes. After opening the configuration
>>>> options and enabling the node, it will start recording. The recorded
>>>> stack information will not be clear, and we can get information form
>>>> kernel log.
>>>>
>>>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>>>> ---
>>>>    Documentation/ABI/testing/sysfs-fs-f2fs |  7 ++++
>>>>    fs/f2fs/Kconfig                         | 10 ++++++
>>>>    fs/f2fs/f2fs.h                          | 45 +++++++++++++++++++++++++
>>>>    fs/f2fs/sysfs.c                         | 27 +++++++++++++++
>>>>    4 files changed, 89 insertions(+)
>>>>
>>>> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> index ef4b9218ae1e..047c398093cf 100644
>>>> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
>>>> @@ -493,3 +493,10 @@ Contact:	"Chao Yu" <yuchao0@huawei.com>
>>>>    Description:	When ATGC is on, it controls age threshold to bypass GCing young
>>>>    		candidates whose age is not beyond the threshold, by default it was
>>>>    		initialized as 604800 seconds (equals to 7 days).
>>>> +
>>>> +What:		/sys/fs/f2fs/<disk>/fsck_stack
>>>> +Date:		August 2021
>>>> +Contact:	"Yangtao Li" <frank.li@vivo.com>
>>>> +Description:	Controls to enable/disable fsck stack trace, you can get stack
>>>> +		information from kernel log. Note that the recorded stack information
>>>> +		will not be cleared.
>>>
>>> Again, please don't add this into sysfs.
>
>Oh, I missed to check the details...
>
>> 
>> I added this node, part of the idea is to trigger the export of stack information.
>> There is no information transmitted through sysfs here, but the record of the stack is switched on and off.
>> If don't export this information through procfs and sysfs, is there a more appropriate way?
>
>Well, I doubt why we should export stack info via proc/sysfs node or

>sysfs switch.>
>Those info will always be needed to troubleshoot issues no matter in
>user or eng version of Android, can we just print them directly into
>kernel message... what I concern is we may lost the bug scene due to

>no one can help to trigger dmesg printing via sysfs.


We can collect these stacks in user space by monitoring whether the sbi status contains fsck.

In addition, I noticed that there are some places, f2fs_bug_on is set in some places,
and set_sbi_flag(sbi, SBI_NEED_FSCK) is used in a small part. Why not change everything
to f2fs_bug_on? In this way, when CONFIG_F2FS_CHECK_FS is turned on, it will actively
panic, allowing us to discover problems in time. If follow the above approach, it is better
to have a separate F2FS_FSCK_STACK_TRACE configuration?


Thx，>
>Jaegeuk, thoughts?
>
>> 
>>>
>>>> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
>>>> index 7669de7b49ce..f451e567e4a8 100644
>>>> --- a/fs/f2fs/Kconfig
>>>> +++ b/fs/f2fs/Kconfig
>>>> @@ -135,3 +135,13 @@ config F2FS_FS_LZORLE
>>>>    	default y
>>>>    	help
>>>>    	  Support LZO-RLE compress algorithm, if unsure, say Y.
>>>> +
>>>> +config F2FS_FSCK_STACK_TRACE
>>>
>>> I don't think we need another config to wrap this functionality, may be we
>>> can use F2FS_CHECK_FS instead.
>> 
>> OK.
>> 
>>>
>>>> +	bool "F2FS fsck stack information record"
>>>> +	depends on F2FS_FS
>>>> +	depends on STACKDEPOT
>>>> +	default y
>>>> +	help
>>>> +	 Support printing out fsck stack history. With this, you have to
>>>> +	 turn on "fsck_stack" sysfs node. Then you can get information
>>>> +	 from kernel log.
>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>> index ee8eb33e2c25..b2d1d1a5a3fc 100644
>>>> --- a/fs/f2fs/f2fs.h
>>>> +++ b/fs/f2fs/f2fs.h
>>>> @@ -24,6 +24,8 @@
>>>>    #include <linux/quotaops.h>
>>>>    #include <linux/part_stat.h>
>>>>    #include <crypto/hash.h>
>>>> +#include <linux/stackdepot.h>
>>>> +#include <linux/stacktrace.h>
>>>>    
>>>>    #include <linux/fscrypt.h>
>>>>    #include <linux/fsverity.h>
>>>> @@ -117,6 +119,8 @@ typedef u32 nid_t;
>>>>    
>>>>    #define COMPRESS_EXT_NUM		16
>>>>    
>>>> +#define FSCK_STACK_DEPTH		64
>>>
>>> 16?
>> 
>> OK.
>> 
>>>
>>>> +
>>>>    struct f2fs_mount_info {
>>>>    	unsigned int opt;
>>>>    	int write_io_size_bits;		/* Write IO size bits */
>>>> @@ -1748,6 +1752,11 @@ struct f2fs_sb_info {
>>>>    	unsigned int compress_watermark;	/* cache page watermark */
>>>>    	atomic_t compress_page_hit;		/* cache hit count */
>>>>    #endif
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	depot_stack_handle_t *fsck_stack_history;
>>>> +	unsigned int fsck_count;
>>>> +	bool fsck_stack;
>>>
>>> IMO, all bug_on()s are corner cases, and catching those stacks won't cost
>>> much, so we can just use CONFIG_XXX to enable/disable this feature.
>> 
>> F2FS_CHECK_FS ？
>> 
>>>
>>>> +#endif
>>>>    };
>>>>    
>>>>    struct f2fs_private_dio {
>>>> @@ -1954,6 +1963,38 @@ static inline struct address_space *NODE_MAPPING(struct f2fs_sb_info *sbi)
>>>>    	return sbi->node_inode->i_mapping;
>>>>    }
>>>>    
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +static void fsck_stack_trace(struct f2fs_sb_info *sbi)
>>>> +{
>>>> +	unsigned long entries[FSCK_STACK_DEPTH];
>>>> +	depot_stack_handle_t stack, *new;
>>>> +	unsigned int nr_entries;
>>>> +	int i;
>>>> +
>>>> +	if (!sbi->fsck_stack)
>>>> +		return;
>>>> +
>>>> +	nr_entries = stack_trace_save(entries, ARRAY_SIZE(entries), 0);
>>>> +	nr_entries = filter_irq_stacks(entries, nr_entries);
>>>> +	stack = stack_depot_save(entries, nr_entries, GFP_KERNEL);
>>>> +	if (!stack)
>>>> +		return;
>>>> +
>>>> +	/* Try to find an existing entry for this backtrace */
>>>> +	for (i = 0; i < sbi->fsck_count; i++)
>>>> +		if (sbi->fsck_stack_history[i] == stack)
>>>> +			return;
>>>> +
>>>> +	new = krealloc(sbi->fsck_stack_history, (sbi->fsck_count + 1) *
>>>> +		       sizeof(*sbi->fsck_stack_history), GFP_KERNEL);
>>>> +	if (!new)
>>>> +		return;
>>>> +
>>>> +	sbi->fsck_stack_history = new;
>>>> +	sbi->fsck_stack_history[sbi->fsck_count++] = stack;
>>>
>>> It will case memory leak after f2fs module exits.
>> 
>> So let's enable this feature when f2fs is not a module and enable F2FS_CHECK_FS.
>
>I mean it needs to free .fsck_stack_history during umount().
>
>Thanks,
>
>> 
>>>
>>>> +}
>>>> +#endif
>>>> +
>>>>    static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    {
>>>>    	return test_bit(type, &sbi->s_flag);
>>>> @@ -1962,6 +2003,10 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>>    {
>>>>    	set_bit(type, &sbi->s_flag);
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	if (unlikely(type == SBI_NEED_FSCK))
>>>> +		fsck_stack_trace(sbi);
>>>> +#endif
>>>>    }
>>>>    
>>>>    static inline void clear_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type)
>>>> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
>>>> index 204de4c2c818..4e786bb797e7 100644
>>>> --- a/fs/f2fs/sysfs.c
>>>> +++ b/fs/f2fs/sysfs.c
>>>> @@ -306,6 +306,26 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
>>>>    	if (!strcmp(a->attr.name, "compr_new_inode"))
>>>>    		return sysfs_emit(buf, "%u\n", sbi->compr_new_inode);
>>>>    #endif
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	if (!strcmp(a->attr.name, "fsck_stack")) {
>>>> +		unsigned long *entries;
>>>> +		unsigned int nr_entries;
>>>> +		unsigned int i;
>>>> +		int count;
>>>> +
>>>> +		count = sysfs_emit(buf, "%u\n", sbi->fsck_stack);
>>>> +		if (!sbi->fsck_stack)
>>>> +			return count;
>>>> +
>>>> +		for (i = 0; i < sbi->fsck_count; i++) {
>>>> +			nr_entries = stack_depot_fetch(sbi->fsck_stack_history[i], &entries);
>>>> +			if (!entries)
>>>> +				return count;
>>>> +			stack_trace_print(entries, nr_entries, 0);
>>>> +		}
>>>> +		return count;
>>>> +	}
>>>> +#endif
>>>>    
>>>>    	ui = (unsigned int *)(ptr + a->offset);
>>>>    
>>>> @@ -740,6 +760,10 @@ F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_candidate_count, max_candidate_cou
>>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_weight, age_weight);
>>>>    F2FS_RW_ATTR(ATGC_INFO, atgc_management, atgc_age_threshold, age_threshold);
>>>>    
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, fsck_stack, fsck_stack);
>>>> +#endif
>>>> +
>>>>    #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
>>>>    static struct attribute *f2fs_attrs[] = {
>>>>    	ATTR_LIST(gc_urgent_sleep_time),
>>>> @@ -812,6 +836,9 @@ static struct attribute *f2fs_attrs[] = {
>>>>    	ATTR_LIST(atgc_candidate_count),
>>>>    	ATTR_LIST(atgc_age_weight),
>>>>    	ATTR_LIST(atgc_age_threshold),
>>>> +#ifdef CONFIG_F2FS_FSCK_STACK_TRACE
>>>> +	ATTR_LIST(fsck_stack),
>>>> +#endif
>>>>    	NULL,
>>>>    };
>>>>    ATTRIBUTE_GROUPS(f2fs);
>>>>
>> 
>> Thx,
>> Yangtao
>>