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

[Chao Yu <chao@kernel.org>]

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?

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