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

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

[Daeho Jeong]

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;
+
 		/* Don't touch checkpointed data */
 		if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
 			if (p.alloc_mode == LFS) {
-- 
2.53.0.473.g4a7958ca14-goog

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

[Chao Yu]

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?

Thanks,

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

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

[Daeho Jeong]

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 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;


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

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

[Chao Yu]

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.

> 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.

Thanks,

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

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

[Daeho Jeong]

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.
>
> > 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?

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

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@gmail.com> 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.
> >
> > > 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) {
> > >>
> >

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

[Chao Yu]

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

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@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?

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

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

[Chao Yu]

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

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

[Daeho Jeong]

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?

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

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

[Chao Yu]

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