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

[Chao Yu <chao@kernel.org>]

On 2026/3/14 07:50, Daeho Jeong wrote:
> On Fri, Mar 13, 2026 at 4:46 PM Chao Yu <chao@kernel.org> wrote:
>>
>> On 2026/3/14 00:21, Daeho Jeong wrote:
>>> On Thu, Mar 12, 2026 at 11:49 PM Chao Yu <chao@kernel.org> wrote:
>>>>
>>>> On 3/12/2026 11:28 PM, Daeho Jeong wrote:
>>>>> On Thu, Mar 12, 2026 at 2:07 AM Chao Yu <chao@kernel.org> wrote:
>>>>>>
>>>>>> 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.
>>>>
>>>> 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?

I think so, let me know once you have any other founds. ;)

Thanks,

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