On Fri, Mar 13, 2026 at 4:46 PM Chao Yu <[email protected]> wrote:
>
> On 2026/3/14 00:21, Daeho Jeong wrote:
> > On Thu, Mar 12, 2026 at 11:49 PM Chao Yu <[email protected]> wrote:
> >>
> >> On 3/12/2026 11:28 PM, Daeho Jeong wrote:
> >>> On Thu, Mar 12, 2026 at 2:07 AM Chao Yu <[email protected]> wrote:
> >>>>
> >>>> On 2026/3/12 00:05, Daeho Jeong wrote:
> >>>>> On Wed, Mar 11, 2026 at 6:44 AM Chao Yu <[email protected]> wrote:
> >>>>>>
> >>>>>> On 2026/3/11 01:54, Daeho Jeong wrote:
> >>>>>>> From: Daeho Jeong <[email protected]>
> >>>>>>>
> >>>>>>> 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 <[email protected]>
> >>>>>>> ---
> >>>>>>> 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?
Then, I think we agree on this check is enough, right?
> >
> > 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.
>
> Sorry, I remember the wrong place we fallback to allocate a free segment, see
> get_atssr_segment() below, inside get_ssr_segment() we only search dirty
> segment/section, once it failed, we call new_curseg() to find a free one.
>
> 3083 static int get_atssr_segment(struct f2fs_sb_info *sbi, int type,
> 3084 int target_type, int alloc_mode,
> 3085 unsigned long long age)
> 3086 {
> 3087 struct curseg_info *curseg = CURSEG_I(sbi, type);
> 3088 int ret = 0;
> 3089
> 3090 curseg->seg_type = target_type;
> 3091
> 3092 if (get_ssr_segment(sbi, type, alloc_mode, age)) {
> 3093 struct seg_entry *se = get_seg_entry(sbi,
> curseg->next_segno);
> 3094
> 3095 curseg->seg_type = se->type;
> 3096 ret = change_curseg(sbi, type);
> 3097 } else {
> 3098 /* allocate cold segment by default */
> 3099 curseg->seg_type = CURSEG_COLD_DATA;
> 3100 ret = new_curseg(sbi, type, true);
> 3101 }
> 3102 stat_inc_seg_type(sbi, curseg);
> 3103 return ret;
> 3104 }
>
> IIUC, in f2fs_get_victim(), we should never expect to find a free segment
> from dirty
> bitmap, except for the checkpoint disabled case, that's what we need to fix,
> right?
>
> Thanks,
>
> >
> >>
> >>>>
> >>>>> 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) {
> >>>>>>
> >>>>
> >>
>
_______________________________________________
Linux-f2fs-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
