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

[Daeho Jeong]

On Thu, Mar 12, 2026 at 11:49 PM Chao Yu <chao@xxxxxxxxxx> wrote:
>
> 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?

Did you say AT_SSR can use a free segment? If we put this condition
here, AT_SSR will not use a free segment anymore.

>
> >>
> >>> 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) {
> >>>>
> >>
>