Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim

[Chao Yu <chao@kernel.org>]

On 2026/3/12 00:05, Daeho Jeong wrote:
> On Wed, Mar 11, 2026 at 6:44 AM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2026/3/11 01:54, Daeho Jeong wrote:
>>> From: Daeho Jeong <daehojeong@google.com>
>>>
>>> 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@google.com>
>>> ---
>>>    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.

> 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.

Thanks,

> 
> 
>>
>> Thanks,
>>
>>> +
>>>                /* Don't touch checkpointed data */
>>>                if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
>>>                        if (p.alloc_mode == LFS) {
>>