From: Chao Yu <chao@kernel.org>
To: 李扬韬 <frank.li@vivo.com>
Cc: jaegeuk@kernel.org, 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
Date: Sat, 14 Aug 2021 09:59:25 +0800 [thread overview]
Message-ID: <25e44e70-eca6-5e48-fca1-844f76ea3a06@kernel.org> (raw)
In-Reply-To: <ALgAlACUD67cbvnqcwuTRaqA.3.1628879583910.Hmail.frank.li@vivo.com>
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
>
next prev parent reply other threads:[~2021-08-14 1:59 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-08-13 12:32 [PATCH v5 1/2] f2fs: introduce proc/fs/f2fs/<dev>/fsck_stack node Yangtao Li
2021-08-13 12:32 ` [PATCH v5 2/2] f2fs: convert S_IRUGO to 0444 Yangtao Li
2021-08-13 14:44 ` [PATCH v5 1/2] f2fs: introduce proc/fs/f2fs/<dev>/fsck_stack node Chao Yu
2021-08-13 18:33 ` 李扬韬
2021-08-14 1:59 ` Chao Yu [this message]
2021-08-14 2:08 ` 李扬韬
2021-08-16 1:54 ` Chao Yu
2021-08-13 18:06 ` kernel test robot
2021-08-14 8:52 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=25e44e70-eca6-5e48-fca1-844f76ea3a06@kernel.org \
--to=chao@kernel.org \
--cc=frank.li@vivo.com \
--cc=jaegeuk@kernel.org \
--cc=linux-f2fs-devel@lists.sourceforge.net \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).