From: Jaegeuk Kim <jaegeuk@kernel.org>
To: Chao Yu <chao@kernel.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"linux-f2fs-devel@lists.sourceforge.net"
<linux-f2fs-devel@lists.sourceforge.net>
Subject: Re: [f2fs-dev] [PATCH v2] f2fs: Fix system crash due to lack of free space in LFS
Date: Tue, 4 Apr 2023 14:26:09 -0700 [thread overview]
Message-ID: <ZCyV8ZSrj1rlJslZ@google.com> (raw)
In-Reply-To: <87c74724-da3d-c5fd-b70d-b80a48ec058e@kernel.org>
On 04/04, Chao Yu wrote:
> 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@samsung.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?
I see. Thanks. I think we can do like this:
Signed-off-by: Yonggil Song <yonggil.song@samsung.com>
[Jaegeuk Kim: code clean-up]
Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
fs/f2fs/gc.c | 10 ++++++++--
fs/f2fs/gc.h | 2 ++
fs/f2fs/segment.h | 39 ++++++++++++++++++++++++++++++---------
3 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 5261b5b5e8d1..56c53dbe05c9 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1805,6 +1805,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 upper_secs;
trace_f2fs_gc_begin(sbi->sb, gc_type, gc_control->no_bg_gc,
gc_control->nr_free_secs,
@@ -1890,8 +1891,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 &&
+ __get_secs_required(sbi, NULL, &upper_secs, NULL);
+
+ /*
+ * Write checkpoint to reclaim prefree segments.
+ * We need more three extra sections for writer's data/node/dentry.
+ */
+ if (free_sections(sbi) <= upper_secs + 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 5ad6ac63e13f..28a00942802c 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 99e34d32c5c6..ac2e35170f2d 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -595,8 +595,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 void __get_secs_required(struct f2fs_sb_info *sbi,
+ unsigned int *lower_p, unsigned int *upper_p, bool *curseg_p)
{
unsigned int total_node_blocks = get_pages(sbi, F2FS_DIRTY_NODES) +
get_pages(sbi, F2FS_DIRTY_DENTS) +
@@ -606,20 +610,37 @@ 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);
- unsigned int free, need_lower, need_upper;
+
+ if (lower_p)
+ *lower_p = node_secs + dent_secs;
+ if (upper_p)
+ *upper_p = node_secs + dent_secs +
+ (node_blocks ? 1 : 0) + (dent_blocks ? 1 : 0);
+ if (curseg_p)
+ *curseg_p = 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_secs, lower_secs, upper_secs;
+ bool curseg_space;
if (unlikely(is_sbi_flag_set(sbi, SBI_POR_DOING)))
return false;
- 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);
+ __get_secs_required(sbi, &lower_secs, &upper_secs, &curseg_space);
+
+ free_secs = free_sections(sbi) + freed;
+ lower_secs += needed + reserved_sections(sbi);
+ upper_secs += needed + reserved_sections(sbi);
- if (free > need_upper)
+ if (free_secs > upper_secs)
return false;
- else if (free <= need_lower)
+ else if (free_secs <= lower_secs)
return true;
- return !has_curseg_enough_space(sbi, node_blocks, dent_blocks);
+ return !curseg_space;
}
static inline bool f2fs_is_checkpoint_ready(struct f2fs_sb_info *sbi)
--
2.40.0.348.gf938b09366-goog
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
next prev parent reply other threads:[~2023-04-04 21:26 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20230321001251epcms2p4c1fd48495643dbfca2cf82a433490bb8@epcms2p4>
2023-03-21 0:12 ` [f2fs-dev] [PATCH v2] f2fs: Fix system crash due to lack of free space in LFS Yonggil Song
2023-03-27 16:00 ` patchwork-bot+f2fs
2023-04-01 2:29 ` Chao Yu
2023-04-03 17:01 ` Jaegeuk Kim
2023-04-04 1:25 ` Chao Yu
2023-04-04 21:26 ` Jaegeuk Kim [this message]
2023-04-05 0:35 ` Chao Yu
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=ZCyV8ZSrj1rlJslZ@google.com \
--to=jaegeuk@kernel.org \
--cc=chao@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).