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

[Daeho Jeong]

On Thu, Mar 12, 2026 at 8:28 AM Daeho Jeong <daeho43@xxxxxxxxx> 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.
> >
> > > 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)?
> I don't think this is just an unlucky case. Is this expected result?

Hmm, looks like INVALID_MTIME is used only for large section cases.
It's a bit complicated to process it. Let me digest it more. :(

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