lkml.org 
[lkml]   [2021]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v6] f2fs: introduce /sys/fs/f2fs/<disk>/fsck_stack node
    From
    Date
    On 2021/8/16 15:23, 李扬韬 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 <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.
    >>
    >>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().
    > >
    >
    > So this is a double-edged sword, and the person who uses it must be aware of this issue.
    > The release seems unrealistic under the current circumstances. This is also the reason

    You mean the handles allocated from stack_depot_save() should not be released, right?

    > why I add a switch, instead of enabling it by default after opening the configuration.

    Well, I still didn't get why we can not release the memory allocated by krealloc().

    Thanks,

    >
    > Yangtao
    >
    >

    \
     
     \ /
      Last update: 2021-08-19 10:06    [W:7.028 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site