| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 4 Apr 2023 09:25:21 +0800
From: Chao Yu <chao@...nel.org>
To: Jaegeuk Kim <jaegeuk@...nel.org>
Cc: yonggil.song@...sung.com,
"linux-f2fs-devel@...ts.sourceforge.net"
<linux-f2fs-devel@...ts.sourceforge.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] f2fs: Fix system crash due to lack of free space in
LFS
On 2023/4/4 1:01, Jaegeuk Kim wrote:
> On 04/01, Chao Yu wrote:
>> On 2023/3/21 8:12, Yonggil Song wrote:
>>> When f2fs tries to checkpoint during foreground gc in LFS mode, system
>>> crash occurs due to lack of free space if the amount of dirty node and
>>> dentry pages generated by data migration exceeds free space.
>>> The reproduction sequence is as follows.
>>>
>>> - 20GiB capacity block device (null_blk)
>>> - format and mount with LFS mode
>>> - create a file and write 20,000MiB
>>> - 4k random write on full range of the file
>>>
>>> RIP: 0010:new_curseg+0x48a/0x510 [f2fs]
>>> Code: 55 e7 f5 89 c0 48 0f af c3 48 8b 5d c0 48 c1 e8 20 83 c0 01 89 43 6c 48 83 c4 28 5b 41 5c 41 5d 41 5e 41 5f 5d c3 cc cc cc cc <0f> 0b f0 41 80 4f 48 04 45 85 f6 0f 84 ba fd ff ff e9 ef fe ff ff
>>> RSP: 0018:ffff977bc397b218 EFLAGS: 00010246
>>> RAX: 00000000000027b9 RBX: 0000000000000000 RCX: 00000000000027c0
>>> RDX: 0000000000000000 RSI: 00000000000027b9 RDI: ffff8c25ab4e74f8
>>> RBP: ffff977bc397b268 R08: 00000000000027b9 R09: ffff8c29e4a34b40
>>> R10: 0000000000000001 R11: ffff977bc397b0d8 R12: 0000000000000000
>>> R13: ffff8c25b4dd81a0 R14: 0000000000000000 R15: ffff8c2f667f9000
>>> FS: 0000000000000000(0000) GS:ffff8c344ec80000(0000) knlGS:0000000000000000
>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>> CR2: 000000c00055d000 CR3: 0000000e30810003 CR4: 00000000003706e0
>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>> Call Trace:
>>> <TASK>
>>> allocate_segment_by_default+0x9c/0x110 [f2fs]
>>> f2fs_allocate_data_block+0x243/0xa30 [f2fs]
>>> ? __mod_lruvec_page_state+0xa0/0x150
>>> do_write_page+0x80/0x160 [f2fs]
>>> f2fs_do_write_node_page+0x32/0x50 [f2fs]
>>> __write_node_page+0x339/0x730 [f2fs]
>>> f2fs_sync_node_pages+0x5a6/0x780 [f2fs]
>>> block_operations+0x257/0x340 [f2fs]
>>> f2fs_write_checkpoint+0x102/0x1050 [f2fs]
>>> f2fs_gc+0x27c/0x630 [f2fs]
>>> ? folio_mark_dirty+0x36/0x70
>>> f2fs_balance_fs+0x16f/0x180 [f2fs]
>>>
>>> This patch adds checking whether free sections are enough before checkpoint
>>> during gc.
>>>
>>> Signed-off-by: Yonggil Song <yonggil.song@...sung.com>
>>> ---
>>> fs/f2fs/gc.c | 10 ++++++++--
>>> fs/f2fs/gc.h | 2 ++
>>> fs/f2fs/segment.h | 27 ++++++++++++++++++++++-----
>>> 3 files changed, 32 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
>>> index 4546e01b2ee0..dd563866d3c9 100644
>>> --- a/fs/f2fs/gc.c
>>> +++ b/fs/f2fs/gc.c
>>> @@ -1773,6 +1773,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
>>> .iroot = RADIX_TREE_INIT(gc_list.iroot, GFP_NOFS),
>>> };
>>> unsigned int skipped_round = 0, round = 0;
>>> + unsigned int need_lower = 0, need_upper = 0;
>>> trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc,
>>> gc_control->nr_free_secs,
>>> @@ -1858,8 +1859,13 @@ int f2fs_gc(struct f2fs_sb_info *sbi, struct f2fs_gc_control *gc_control)
>>> }
>>> }
>>> - /* Write checkpoint to reclaim prefree segments */
>>> - if (free_sections(sbi) < NR_CURSEG_PERSIST_TYPE &&
>>> + ret = get_need_secs(sbi, &need_lower, &need_upper);
>>
>> Can we avoid calling has_curseg_enough_space() for this case?
>
> Why? :P
We won't check the return value of get_need_secs(), so it's not needed to call
has_curseg_enough_space() in get_need_secs() in this path, right?
Thanks,
>
>>
>> Maybe we can add one parameter curseg_no_space for get_need_secs() to get
>> result of has_curseg_enough_space()?
>>
>> static inline void get_need_secs(struct f2fs_sb_info *sbi,
>> unsigned int *lower, unsigned int *upper,
>> bool *curseg_no_space);
>> {
>> ...
>> *lower = node_secs + dent_secs;
>> *upper = *lower + (node_blocks ? 1 : 0) + (dent_blocks ? 1 : 0);
>>
>> if (curseg_no_space)
>> *curseg_no_space =
>> !has_curseg_enough_space(sbi, node_blocks, dent_blocks);
>> }
>>
>> Then we can use get_need_secs(, , NULL) in f2fs_gc(),
>> and use get_need_secs(, , &curseg_no_space) in has_not_enough_free_secs()?
>>
>> Thoughts?
>>
>> Thanks,
>>
>>> +
>>> + /*
>>> + * Write checkpoint to reclaim prefree segments.
>>> + * We need more three extra sections for writer's data/node/dentry.
>>> + */
>>> + if (free_sections(sbi) <= need_upper + NR_GC_CHECKPOINT_SECS &&
>>> prefree_segments(sbi)) {
>>> ret = f2fs_write_checkpoint(sbi, &cpc);
>>> if (ret)
>>> diff --git a/fs/f2fs/gc.h b/fs/f2fs/gc.h
>>> index 19b956c2d697..e81d22bf3772 100644
>>> --- a/fs/f2fs/gc.h
>>> +++ b/fs/f2fs/gc.h
>>> @@ -30,6 +30,8 @@
>>> /* Search max. number of dirty segments to select a victim segment */
>>> #define DEF_MAX_VICTIM_SEARCH 4096 /* covers 8GB */
>>> +#define NR_GC_CHECKPOINT_SECS (3) /* data/node/dentry sections */
>>> +
>>> struct f2fs_gc_kthread {
>>> struct task_struct *f2fs_gc_task;
>>> wait_queue_head_t gc_wait_queue_head;
>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
>>> index be8f2d7d007b..52a6d1ed4f24 100644
>>> --- a/fs/f2fs/segment.h
>>> +++ b/fs/f2fs/segment.h
>>> @@ -605,8 +605,12 @@ static inline bool has_curseg_enough_space(struct f2fs_sb_info *sbi,
>>> return true;
>>> }
>>> -static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi,
>>> - int freed, int needed)
>>> +/*
>>> + * calculate needed sections for dirty node/dentry
>>> + * and call has_curseg_enough_space
>>> + */
>>> +static inline bool get_need_secs(struct f2fs_sb_info *sbi,
>>> + unsigned int *lower, unsigned int *upper)
>>> {
>>> unsigned int total_node_blocks = get_pages(sbi, F2FS_DIRTY_NODES) +
>>> get_pages(sbi, F2FS_DIRTY_DENTS) +
>>> @@ -616,20 +620,33 @@ static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi,
>>> unsigned int dent_secs = total_dent_blocks / CAP_BLKS_PER_SEC(sbi);
>>> unsigned int node_blocks = total_node_blocks % CAP_BLKS_PER_SEC(sbi);
>>> unsigned int dent_blocks = total_dent_blocks % CAP_BLKS_PER_SEC(sbi);
>>> +
>>> + *lower = node_secs + dent_secs;
>>> + *upper = *lower + (node_blocks ? 1 : 0) + (dent_blocks ? 1 : 0);
>>> +
>>> + return !has_curseg_enough_space(sbi, node_blocks, dent_blocks);
>>> +}
>>> +
>>> +static inline bool has_not_enough_free_secs(struct f2fs_sb_info *sbi,
>>> + int freed, int needed)
>>> +{
>>> unsigned int free, need_lower, need_upper;
>>> + bool curseg_enough;
>>> if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
>>> return false;
>>> + curseg_enough = get_need_secs(sbi, &need_lower, &need_upper);
>>> +
>>> free = free_sections(sbi) + freed;
>>> - need_lower = node_secs + dent_secs + reserved_sections(sbi) + needed;
>>> - need_upper = need_lower + (node_blocks ? 1 : 0) + (dent_blocks ? 1 : 0);
>>> + need_lower += (needed + reserved_sections(sbi));
>>> + need_upper += (needed + reserved_sections(sbi));
>>> if (free > need_upper)
>>> return false;
>>> else if (free <= need_lower)
>>> return true;
>>> - return !has_curseg_enough_space(sbi, node_blocks, dent_blocks);
>>> + return curseg_enough;
>>> }
>>> static inline bool f2fs_is_checkpoint_ready(struct f2fs_sb_info *sbi)
Powered by blists - more mailing lists