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

From: 李扬韬
Date: Mon Aug 16 2021 - 02:53:13 EST

Next message: Huacai Chen: "[PATCH] futex: Fix fault_in_user_writeable()"
Previous message: Salvatore Bonaccorso: "Re: [PATCH v2] net: hso: do not call unregister if not registered"
In reply to: Chao Yu: "Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node"
Next in thread: Jaegeuk Kim: "Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

>> 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@xxxxxxxx>
>>>> ---
>>>> 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@xxxxxxxxxx>
>>>> 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@xxxxxxxx>
>>>> +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
>>

Next message: Huacai Chen: "[PATCH] futex: Fix fault_in_user_writeable()"
Previous message: Salvatore Bonaccorso: "Re: [PATCH v2] net: hso: do not call unregister if not registered"
In reply to: Chao Yu: "Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node"
Next in thread: Jaegeuk Kim: "Re: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]