From: Byungki Lee <[email protected]>
If there's not enough free sections each of which consistis of large segments,
we can hit no free section for upcoming section allocation. Let's reclaim some
prefree segments by writing checkpoints.
Signed-off-by: Byungki Lee <[email protected]>
Signed-off-by: Jaegeuk Kim <[email protected]>
---
fs/f2fs/gc.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index eddd2e81a2a2..4d47723523c3 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -1841,23 +1841,31 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
if (sync)
goto stop;
- if (has_not_enough_free_secs(sbi, sec_freed, 0)) {
- if (skipped_round <= MAX_SKIP_GC_COUNT ||
- skipped_round * 2 < round) {
- segno = NULL_SEGNO;
- goto gc_more;
- }
+ if (!has_not_enough_free_secs(sbi, sec_freed, 0))
+ goto stop;
- if (first_skipped < last_skipped &&
- (last_skipped - first_skipped) >
- sbi->skipped_gc_rwsem) {
- f2fs_drop_inmem_pages_all(sbi, true);
- segno = NULL_SEGNO;
- goto gc_more;
- }
- if (gc_type == FG_GC && !is_sbi_flag_set(sbi, SBI_CP_DISABLED))
+ if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
+
+ /* Write checkpoint to reclaim prefree segments */
+ if (free_sections(sbi) < NR_CURSEG_PERSIST_TYPE &&
+ prefree_segments(sbi) &&
+ !is_sbi_flag_set(sbi, SBI_CP_DISABLED)) {
ret = f2fs_write_checkpoint(sbi, &cpc);
- }
+ if (ret)
+ goto stop;
+ }
+ segno = NULL_SEGNO;
+ goto gc_more;
+ }
+ if (first_skipped < last_skipped &&
+ (last_skipped - first_skipped) >
+ sbi->skipped_gc_rwsem) {
+ f2fs_drop_inmem_pages_all(sbi, true);
+ segno = NULL_SEGNO;
+ goto gc_more;
+ }
+ if (gc_type == FG_GC && !is_sbi_flag_set(sbi, SBI_CP_DISABLED))
+ ret = f2fs_write_checkpoint(sbi, &cpc);
stop:
SIT_I(sbi)->last_victim[ALLOC_NEXT] = 0;
SIT_I(sbi)->last_victim[FLUSH_DEVICE] = init_segno;
--
2.36.0.464.gb9c8b46e94-goog
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 <[email protected]>
---
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..5ba8104e138b 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) && seg_freed)
goto stop;
if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
--
2.36.0.464.gb9c8b46e94-goog
On 2022/5/4 4:30, 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 <[email protected]>
> ---
> 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..5ba8104e138b 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;
Semantics of 'sync' parameter should be: migrate one section at least before
exit?
So, should it be?
if (seg_freed && (sync || !has_not_enough_free_secs(sbi, sec_freed, 0)))
goto stop;
Thanks,
> -
> - if (!has_not_enough_free_secs(sbi, sec_freed, 0))
> + if (!has_not_enough_free_secs(sbi, sec_freed, 0) && seg_freed)
> goto stop;
>
> if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
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 <[email protected]>
---
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)
goto stop;
if (skipped_round <= MAX_SKIP_GC_COUNT || skipped_round * 2 < round) {
--
2.36.0.464.gb9c8b46e94-goog
On 2022/5/4 4:30, Jaegeuk Kim wrote:
> From: Byungki Lee <[email protected]>
>
> If there's not enough free sections each of which consistis of large segments,
> we can hit no free section for upcoming section allocation. Let's reclaim some
> prefree segments by writing checkpoints.
>
> Signed-off-by: Byungki Lee <[email protected]>
> Signed-off-by: Jaegeuk Kim <[email protected]>
Reviewed-by: Chao Yu <[email protected]>
Thanks,
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 <[email protected]>
> ---
>
> 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 <[email protected]>
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 <[email protected]>
---
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) {