Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim
From: Chao Yu
Date: Fri Mar 13 2026 - 02:49:41 EST
On 3/12/2026 11:28 PM, Daeho Jeong wrote:
On Thu, Mar 12, 2026 at 2:07 AM Chao Yu <chao@xxxxxxxxxx> wrote:
On 2026/3/12 00:05, Daeho Jeong wrote:
On Wed, Mar 11, 2026 at 6:44 AM Chao Yu <chao@xxxxxxxxxx> wrote:
On 2026/3/11 01:54, Daeho Jeong wrote:
From: Daeho Jeong <daehojeong@xxxxxxxxxx>
In age-based victim selection (ATGC, AT_SSR, or GC_CB), f2fs_get_victim
can encounter sections with zero valid blocks. This situation often
arises when checkpoint is disabled or due to race conditions between
SIT updates and dirty list management.
In such cases, f2fs_get_section_mtime() returns INVALID_MTIME, which
subsequently triggers a fatal f2fs_bug_on(sbi, mtime == INVALID_MTIME)
in add_victim_entry() or get_cb_cost().
This patch adds a check in f2fs_get_victim's selection loop to skip
sections with no valid blocks. This prevents unnecessary age
calculations for empty sections and avoids the associated kernel panic.
This change also allows removing redundant checks in add_victim_entry().
Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
---
fs/f2fs/gc.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 2e0f67946914..981eac629fe9 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -521,12 +521,6 @@ static void add_victim_entry(struct f2fs_sb_info *sbi,
struct sit_info *sit_i = SIT_I(sbi);
unsigned long long mtime = 0;
- if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
- if (p->gc_mode == GC_AT &&
- get_valid_blocks(sbi, segno, true) == 0)
- return;
- }
-
mtime = f2fs_get_section_mtime(sbi, segno);
f2fs_bug_on(sbi, mtime == INVALID_MTIME);
@@ -889,6 +883,9 @@ int f2fs_get_victim(struct f2fs_sb_info *sbi, unsigned int *result,
if (sec_usage_check(sbi, secno))
goto next;
+ if (!get_valid_blocks(sbi, segno, true))
+ goto next;
Well, for f2fs_get_victim(, AT_SSR), once there are no dirty segment, if we
don't count free segment as candidates, then, we can not find any valid victim?
Oh, AT_SSR needs to select the free section in this case?
I think so, for extreme case.
Oh, check the code again, it seems we select victim from dirty bitmap,
the victim should not be a free one...
But there is some exceptions:
locate_dirty_segment()
if (valid_blocks == 0 && (!is_sbi_flag_set(sbi, SBI_CP_DISABLED) ||
ckpt_valid_blocks == usable_blocks)) {
__locate_dirty_segment(sbi, segno, PRE);
__remove_dirty_segment(sbi, segno, DIRTY);
If valid_blocks equals to zero, but if the checkpoint is disabled and also
ckpt_valid_blocks doesn't equals to usable_blocks. The segment (or section)
will still be dirty state in dirty bitmap.
We need to handle this correctly in f2fs_get_victim() correctly before calling
into add_victim_entry() or get_gc_cost()?
/* Don't touch checkpointed data */
if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
if (p.alloc_mode == LFS) {
/*
* LFS is set to find source section during GC.
* The victim should have no checkpointed data.
*/
if (get_ckpt_valid_blocks(sbi, segno, true))
goto next;
} else {
/*
* SSR | AT_SSR are set to find target segment
* for writes which can be full by checkpointed
* and newly written blocks.
*/
if (!f2fs_segment_has_free_slot(sbi, segno))
goto next;
}
if (!get_valid_blocks(sbi, segno, true))
goto next;
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Can this be the fix?
I am confused. Why do we need the below logic?
Looks like WA for the AT_SSR case?
In f2fs_get_section_mtime()
out:
if (unlikely(mtime == INVALID_MTIME))
mtime -= 1;
return mtime;
There are two conditions, in a section:
a) if there are no valid blocks, it will return INVALID_MTIME.
b) if there are vaild blocks, it tries to return mtime which is calculated, but
if unlucky the calculated mtime is equal to INVALID_MTIME, in order to distinguish
from case a), we will return INVALID_MTIME - 1 instead.
If we find a free segment and pass it to f2fs_get_section_mtime() for
(!__is_large_section(sbi)) case.
What is the expected output of it? (INVALID_MTIME - 1)?
It depends on the status of section that free segment belong to:
If there is no valid block in the section, it will return INVALID_MTIME,
otherwise it will return calcuated mtime, or INVALID_MTIME - 1 for
extreme case that mtime is just unluckily equals to INVALID_MTIME.
Thanks,
I don't think this is just an unlucky case. Is this expected result?
Thanks,
Thanks,
+
/* Don't touch checkpointed data */
if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
if (p.alloc_mode == LFS) {