Messages in this thread | | | Subject | Re: [PATCH v5 1/2] f2fs: introduce proc/fs/f2fs/<dev>/fsck_stack node | From | Chao Yu <> | Date | Sat, 14 Aug 2021 09:59:25 +0800 |
| |
On 2021/8/14 2:33, 李扬韬 wrote: > From: Chao Yu <chao@kernel.org> > Date: 2021-08-13 22:44:49 > To: Yangtao Li <frank.li@vivo.com>,jaegeuk@kernel.org > Cc: linux-f2fs-devel@lists.sourceforge.net,linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/2] f2fs: introduce proc/fs/f2fs/<dev>/fsck_stack node>On 2021/8/13 20:32, Yangtao Li wrote: >>> 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 expose all fsck stack history in procfs. >>> >>> Signed-off-by: Yangtao Li <frank.li@vivo.com> >>> --- >>> v5: >>> -fix implicit declaration of function 'stack_trace_save' >>> fs/f2fs/f2fs.h | 34 +++++++++++++++++++++++++++++++++- >>> fs/f2fs/sysfs.c | 26 ++++++++++++++++++++++++++ >>> 2 files changed, 59 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h >>> index 67faa43cc141..cbd06dea3c6a 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> >>> @@ -119,6 +121,8 @@ typedef u32 nid_t; >>> >>> #define COMPRESS_EXT_NUM 16 >>> >>> +#define FSCK_STACK_DEPTH 64 >>> + >>> struct f2fs_mount_info { >>> unsigned int opt; >>> int write_io_size_bits; /* Write IO size bits */ >>> @@ -1786,6 +1790,8 @@ struct f2fs_sb_info { >>> unsigned int compress_watermark; /* cache page watermark */ >>> atomic_t compress_page_hit; /* cache hit count */ >>> #endif >>> + depot_stack_handle_t *fsck_stack; >>> + unsigned int fsck_count; >>> }; >>> >>> struct f2fs_private_dio { >>> @@ -1997,9 +2003,35 @@ static inline bool is_sbi_flag_set(struct f2fs_sb_info *sbi, unsigned int type) >>> return test_bit(type, &sbi->s_flag); >>> } >>> >>> -static inline void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type) >>> +static void set_sbi_flag(struct f2fs_sb_info *sbi, unsigned int type) >>> { >>> set_bit(type, &sbi->s_flag); >>> + >>> + if (unlikely(type == SBI_NEED_FSCK)) { >> >> Hmm... I don't know what to say... > > Sorry, maybe there is a problem with my vim configuration. > The strange thing is that checkpatch.pl didn't check it out. > >> >>> + unsigned long entries[FSCK_STACK_DEPTH]; >>> + depot_stack_handle_t stack, *new; >>> + unsigned int nr_entries; >>> + int i; >>> + >>> + 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[i] == stack) >> >> stack need to be released here? > > We can't remove stack from depot, as we store them contiguously one after > another in a contiguous memory allocation. > > Or we can limit the recorded stack number. > > $ grep -nr "SBI_NEED_FSCK" fs/f2fs/ --include=*.c --include=*.h | wc -l > 53 > $ grep -nr "f2fs_bug_on" fs/f2fs/ --include=*.c --include=*.h | wc -l > 135
I didn't look into details of stack_depot_save(), two stack handles from below call paths will be the same?
- move_data_block - f2fs_wait_on_page_writeback - f2fs_bug_on
- ra_data_block - f2fs_wait_on_page_writeback - f2fs_bug_on
If they have different stack handles, combination number of set_sbi_flag(NEED_FSCK)/f2fs_bug_on and their callers will be far more than two hundred.
Thanks,
> > Since we only have two hundred possible settings here, considering that > the same stack will not be recorded, and the probability of occurrence will > not be high, so it is acceptable not to release? > > If this is the case, the subsequent allocation does not need to be released. > > Thx, > Yangtao >
| |