* [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim @ 2026-03-10 17:54 Daeho Jeong 2026-03-11 13:44 ` [f2fs-dev] " Chao Yu 0 siblings, 1 reply; 11+ messages in thread From: Daeho Jeong @ 2026-03-10 17:54 UTC (permalink / raw) To: linux-kernel, linux-f2fs-devel, kernel-team; +Cc: 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-10 17:54 [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim Daeho Jeong @ 2026-03-11 13:44 ` Chao Yu 2026-03-11 16:05 ` Daeho Jeong 0 siblings, 1 reply; 11+ messages in thread From: Chao Yu @ 2026-03-11 13:44 UTC (permalink / raw) To: Daeho Jeong, linux-kernel, linux-f2fs-devel, kernel-team Cc: chao, Daeho Jeong 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) { ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-11 13:44 ` [f2fs-dev] " Chao Yu @ 2026-03-11 16:05 ` Daeho Jeong 2026-03-12 9:07 ` Chao Yu 0 siblings, 1 reply; 11+ messages in thread From: Daeho Jeong @ 2026-03-11 16:05 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, 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) { > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-11 16:05 ` Daeho Jeong @ 2026-03-12 9:07 ` Chao Yu 2026-03-12 15:28 ` Daeho Jeong 0 siblings, 1 reply; 11+ messages in thread From: Chao Yu @ 2026-03-12 9:07 UTC (permalink / raw) To: Daeho Jeong Cc: chao, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong 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) { >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-12 9:07 ` Chao Yu @ 2026-03-12 15:28 ` Daeho Jeong 2026-03-12 21:34 ` Daeho Jeong 2026-03-13 6:48 ` Chao Yu 0 siblings, 2 replies; 11+ messages in thread From: Daeho Jeong @ 2026-03-12 15:28 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, 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) { > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-12 15:28 ` Daeho Jeong @ 2026-03-12 21:34 ` Daeho Jeong 2026-03-13 6:48 ` Chao Yu 1 sibling, 0 replies; 11+ messages in thread From: Daeho Jeong @ 2026-03-12 21:34 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, 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) { > > >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-12 15:28 ` Daeho Jeong 2026-03-12 21:34 ` Daeho Jeong @ 2026-03-13 6:48 ` Chao Yu 2026-03-13 16:21 ` Daeho Jeong 1 sibling, 1 reply; 11+ messages in thread From: Chao Yu @ 2026-03-13 6:48 UTC (permalink / raw) To: Daeho Jeong Cc: chao, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong 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) { >>>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-13 6:48 ` Chao Yu @ 2026-03-13 16:21 ` Daeho Jeong 2026-03-13 23:46 ` Chao Yu 0 siblings, 1 reply; 11+ messages in thread From: Daeho Jeong @ 2026-03-13 16:21 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, 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) { > >>>> > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-13 16:21 ` Daeho Jeong @ 2026-03-13 23:46 ` Chao Yu 2026-03-13 23:50 ` Daeho Jeong 0 siblings, 1 reply; 11+ messages in thread From: Chao Yu @ 2026-03-13 23:46 UTC (permalink / raw) To: Daeho Jeong Cc: chao, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong 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) { >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-13 23:46 ` Chao Yu @ 2026-03-13 23:50 ` Daeho Jeong 2026-03-13 23:54 ` Chao Yu 0 siblings, 1 reply; 11+ messages in thread From: Daeho Jeong @ 2026-03-13 23:50 UTC (permalink / raw) To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel, kernel-team, 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) { > >>>>>> > >>>> > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [f2fs-dev] [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim 2026-03-13 23:50 ` Daeho Jeong @ 2026-03-13 23:54 ` Chao Yu 0 siblings, 0 replies; 11+ messages in thread From: Chao Yu @ 2026-03-13 23:54 UTC (permalink / raw) To: Daeho Jeong Cc: chao, linux-kernel, linux-f2fs-devel, kernel-team, Daeho Jeong 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) { >>>>>>>> >>>>>> >>>> >> ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-03-13 23:54 UTC | newest] Thread overview: 11+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-10 17:54 [PATCH] f2fs: fix to skip empty sections in f2fs_get_victim Daeho Jeong 2026-03-11 13:44 ` [f2fs-dev] " Chao Yu 2026-03-11 16:05 ` Daeho Jeong 2026-03-12 9:07 ` Chao Yu 2026-03-12 15:28 ` Daeho Jeong 2026-03-12 21:34 ` Daeho Jeong 2026-03-13 6:48 ` Chao Yu 2026-03-13 16:21 ` Daeho Jeong 2026-03-13 23:46 ` Chao Yu 2026-03-13 23:50 ` Daeho Jeong 2026-03-13 23:54 ` Chao Yu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox