Re: [f2fs-dev] [PATCH 2/2 v2] f2fs: avoid to select pinned section during checkpoint=disable
From: Chao Yu
Date: Thu May 05 2022 - 07:35:44 EST
On 2022/5/5 4:22, Jaegeuk Kim wrote:
The f2fs_gc uses a bitmap to indicate pinned sections, but when disabling
chckpoint, we call f2fs_gc() with NULL_SEGNO which selects the same dirty
segment as a victim all the time, resulting in checkpoint=disable failure.
Let's pick another one, if we fail to collect it.
Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
---
Change log from v1:
- keep sync condition to stop GC
fs/f2fs/gc.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4d47723523c3..441190ab8593 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1838,10 +1838,7 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
if (gc_type == FG_GC)
sbi->cur_victim_sec = NULL_SEGNO;
- if (sync)
- goto stop;
-
- if (!has_not_enough_free_secs(sbi, sec_freed, 0))
+ if ((!has_not_enough_free_secs(sbi, sec_freed, 0) || sync) && seg_freed)
Wait, it doesn't make sense that it needs to migrate one section successfully
when there is enough free sections, e.g. for background GC case.
I doubt above implementation may force bggc to migrate all dirty sections?
due to seg_freed will always be zero, because it increases only if gc_type
is FG_GC?
if (gc_type == FG_GC &&
get_valid_blocks(sbi, segno, false) == 0)
seg_freed++;
And I didn't see there is one or combinations of parameters can indicate
meaning: "it needs to migrate one section at least before exit", so how
about introducing another parameter for this requirement?
From: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
Date: Sat, 30 Apr 2022 03:27:20 -0700
Subject: [PATCH] f2fs: avoid to select pinned section during
checkpoint=disable
The f2fs_gc uses a bitmap to indicate pinned sections, but when disabling
chckpoint, we call f2fs_gc() with NULL_SEGNO which selects the same dirty
segment as a victim all the time, resulting in checkpoint=disable failure.
Let's pick another one, if we fail to collect it.
Signed-off-by: Jaegeuk Kim <jaegeuk@xxxxxxxxxx>
---
fs/f2fs/f2fs.h | 2 +-
fs/f2fs/file.c | 8 ++++----
fs/f2fs/gc.c | 17 ++++++++++++-----
fs/f2fs/segment.c | 2 +-
fs/f2fs/super.c | 2 +-
5 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 56adc3b68e14..0f8c426aed50 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -3787,7 +3787,7 @@ int f2fs_start_gc_thread(struct f2fs_sb_info *sbi);
void f2fs_stop_gc_thread(struct f2fs_sb_info *sbi);
block_t f2fs_start_bidx_of_node(unsigned int node_ofs, struct inode *inode);
int f2fs_gc(struct f2fs_sb_info *sbi, bool sync, bool background, bool force,
- unsigned int segno);
+ unsigned int segno, unsigned int nr_section);
void f2fs_build_gc_manager(struct f2fs_sb_info *sbi);
int f2fs_resize_fs(struct f2fs_sb_info *sbi, __u64 block_count);
int __init f2fs_create_garbage_collection_cache(void);
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index e4cf8b7b23aa..07d2fa6c774b 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -1684,7 +1684,7 @@ static int expand_inode_data(struct inode *inode, loff_t offset,
if (has_not_enough_free_secs(sbi, 0,
GET_SEC_FROM_SEG(sbi, overprovision_segments(sbi)))) {
f2fs_down_write(&sbi->gc_lock);
- err = f2fs_gc(sbi, true, false, false, NULL_SEGNO);
+ err = f2fs_gc(sbi, true, false, false, NULL_SEGNO, 0);
if (err && err != -ENODATA && err != -EAGAIN)
goto out_err;
}
@@ -2472,7 +2472,7 @@ static int f2fs_ioc_gc(struct file *filp, unsigned long arg)
f2fs_down_write(&sbi->gc_lock);
}
- ret = f2fs_gc(sbi, sync, true, false, NULL_SEGNO);
+ ret = f2fs_gc(sbi, sync, true, false, NULL_SEGNO, 0);
out:
mnt_drop_write_file(filp);
return ret;
@@ -2509,7 +2509,7 @@ static int __f2fs_ioc_gc_range(struct file *filp, struct f2fs_gc_range *range)
}
ret = f2fs_gc(sbi, range->sync, true, false,
- GET_SEGNO(sbi, range->start));
+ GET_SEGNO(sbi, range->start), 0);
if (ret) {
if (ret == -EBUSY)
ret = -EAGAIN;
@@ -2966,7 +2966,7 @@ static int f2fs_ioc_flush_device(struct file *filp, unsigned long arg)
sm->last_victim[GC_CB] = end_segno + 1;
sm->last_victim[GC_GREEDY] = end_segno + 1;
sm->last_victim[ALLOC_NEXT] = end_segno + 1;
- ret = f2fs_gc(sbi, true, true, true, start_segno);
+ ret = f2fs_gc(sbi, true, true, true, start_segno, 0);
if (ret == -EAGAIN)
ret = 0;
else if (ret < 0)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 4d47723523c3..2364ffe6bbbf 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -142,7 +142,7 @@ static int gc_thread_func(void *data)
sync_mode = false;
/* if return value is not zero, no victim was selected */
- if (f2fs_gc(sbi, sync_mode, !foreground, false, NULL_SEGNO))
+ if (f2fs_gc(sbi, sync_mode, !foreground, false, NULL_SEGNO, 0))
wait_ms = gc_th->no_gc_sleep_time;
if (foreground)
@@ -1751,7 +1751,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
}
int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
- bool background, bool force, unsigned int segno)
+ bool background, bool force, unsigned int segno,
+ unsigned int nr_section)
{
int gc_type = sync ? FG_GC : BG_GC;
int sec_freed = 0, seg_freed = 0, total_freed = 0;
@@ -1838,11 +1839,17 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
if (gc_type == FG_GC)
sbi->cur_victim_sec = NULL_SEGNO;
- if (sync)
- goto stop;
+
- if (!has_not_enough_free_secs(sbi, sec_freed, 0))
+ if (sync || !has_not_enough_free_secs(sbi, sec_freed, 0)) {
+ /*
+ * f2fs_disable_checkpoint() needs to migrate enough section
+ * before f2fs_gc() exits.
+ */
+ if (total_freed < nr_section)
+ goto gc_more;
goto stop;
+ }
if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 8c17fed8987e..3a3e2cec2ac4 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -524,7 +524,7 @@ void f2fs_balance_fs(struct f2fs_sb_info *sbi, bool need)
finish_wait(&sbi->gc_thread->fggc_wq, &wait);
} else {
f2fs_down_write(&sbi->gc_lock);
- f2fs_gc(sbi, false, false, false, NULL_SEGNO);
+ f2fs_gc(sbi, false, false, false, NULL_SEGNO, 0);
}
}
}
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index d06a577a1208..2f0ad9ffbf17 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2076,7 +2076,7 @@ static int f2fs_disable_checkpoint(struct f2fs_sb_info *sbi)
while (!f2fs_time_over(sbi, DISABLE_TIME)) {
f2fs_down_write(&sbi->gc_lock);
- err = f2fs_gc(sbi, true, false, false, NULL_SEGNO);
+ err = f2fs_gc(sbi, true, false, false, NULL_SEGNO, 1);
if (err == -ENODATA) {
err = 0;
break;
--
2.32.0
goto stop;
if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {