On 08/16, Chao Yu wrote:
> On 2021/8/16 12:02, 李扬韬 wrote:
> > 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 <[email protected]>
> > > > ---
> > > > 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" <[email protected]>
> > > > 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" <[email protected]>
> > > > +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.
>
> Jaegeuk, thoughts?
I thought that it'd be good to have an error history in debufs. For example,
we can have a set of reasons defined as enum and put the error counts per
SBI_NEED_FSCK along with error number like ENOMEM, EIO, and so on. The goal
would be to understand whether we're getting SBI_NEED_FSCK which triggers
fsck in practical.
>
> >
> > >
> > > > 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
> >
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
