Re: [PATCH v5 1/2] f2fs: introduce proc/fs/f2fs/<dev>/fsck_stack node

[Chao Yu]

On 2021/8/14 2:33, 李扬韬 wrote:
> From: Chao Yu <chao@xxxxxxxxxx>
> Date: 2021-08-13 22:44:49
> To:  Yangtao Li <frank.li@xxxxxxxx>,jaegeuk@xxxxxxxxxx
> Cc:  linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxxx>
> > > ---
> > > 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