| Message ID | 20250403232107.2960-1-yohan.joung@sk.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,v5] f2fs: prevent the current section from being selected as a victim during GC | expand |
Hi Yohan,
I modified this patch after applying the clean up by
https://lore.kernel.org/linux-f2fs-devel/20250404195442.413945-1-jaegeuk@kernel.org/T/#u
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -486,6 +486,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
free_i->free_sections++;
+ if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno)
+ sbi->next_victim_seg[BG_GC] = NULL_SEGNO;
+ if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno)
+ sbi->next_victim_seg[FG_GC] = NULL_SEGNO;
+
unlock_out:
spin_unlock(&free_i->segmap_lock);
}
On 04/04, yohan.joung wrote:
> When selecting a victim using next_victim_seg in a large section, the
> selected section might already have been cleared and designated as the
> new current section, making it actively in use.
> This behavior causes inconsistency between the SIT and SSA.
>
> F2FS-fs (dm-54): Inconsistent segment (70961) type [0, 1] in SSA and SIT
> Call trace:
> dump_backtrace+0xe8/0x10c
> show_stack+0x18/0x28
> dump_stack_lvl+0x50/0x6c
> dump_stack+0x18/0x28
> f2fs_stop_checkpoint+0x1c/0x3c
> do_garbage_collect+0x41c/0x271c
> f2fs_gc+0x27c/0x828
> gc_thread_func+0x290/0x88c
> kthread+0x11c/0x164
> ret_from_fork+0x10/0x20
>
> issue scenario
> segs_per_sec=2
> - seg#0 and seg#1 are all dirty
> - all valid blocks are removed in seg#1
> - gc select this sec and next_victim_seg=seg#0
> - migrate seg#0, next_victim_seg=seg#1
> - checkpoint -> sec(seg#0, seg#1) becomes free
> - allocator assigns sec(seg#0, seg#1) to curseg
> - gc tries to migrate seg#1
>
> Signed-off-by: yohan.joung <yohan.joung@sk.com>
> Signed-off-by: Chao Yu <chao@kernel.org>
> ---
> fs/f2fs/segment.h | 9 ++++++++-
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 0465dc00b349..0773283babfa 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
> next = find_next_bit(free_i->free_segmap,
> start_segno + SEGS_PER_SEC(sbi), start_segno);
> if (next >= start_segno + usable_segs) {
> - if (test_and_clear_bit(secno, free_i->free_secmap))
> + if (test_and_clear_bit(secno, free_i->free_secmap)) {
> free_i->free_sections++;
> +
> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno)
> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO;
> +
> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno)
> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO;
> + }
> }
> }
> skip_free:
> --
> 2.33.0
On 4/5/25 03:55, Jaegeuk Kim wrote: > Hi Yohan, > > I modified this patch after applying the clean up by > > https://lore.kernel.org/linux-f2fs-devel/20250404195442.413945-1-jaegeuk@kernel.org/T/#u > > --- a/fs/f2fs/segment.h > +++ b/fs/f2fs/segment.h > @@ -486,6 +486,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, > > free_i->free_sections++; > > + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) > + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; > + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) > + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; Reviewed-by: Chao Yu <chao@kernel.org> Thanks, > + > unlock_out: > spin_unlock(&free_i->segmap_lock); > } > > On 04/04, yohan.joung wrote: >> When selecting a victim using next_victim_seg in a large section, the >> selected section might already have been cleared and designated as the >> new current section, making it actively in use. >> This behavior causes inconsistency between the SIT and SSA. >> >> F2FS-fs (dm-54): Inconsistent segment (70961) type [0, 1] in SSA and SIT >> Call trace: >> dump_backtrace+0xe8/0x10c >> show_stack+0x18/0x28 >> dump_stack_lvl+0x50/0x6c >> dump_stack+0x18/0x28 >> f2fs_stop_checkpoint+0x1c/0x3c >> do_garbage_collect+0x41c/0x271c >> f2fs_gc+0x27c/0x828 >> gc_thread_func+0x290/0x88c >> kthread+0x11c/0x164 >> ret_from_fork+0x10/0x20 >> >> issue scenario >> segs_per_sec=2 >> - seg#0 and seg#1 are all dirty >> - all valid blocks are removed in seg#1 >> - gc select this sec and next_victim_seg=seg#0 >> - migrate seg#0, next_victim_seg=seg#1 >> - checkpoint -> sec(seg#0, seg#1) becomes free >> - allocator assigns sec(seg#0, seg#1) to curseg >> - gc tries to migrate seg#1 >> >> Signed-off-by: yohan.joung <yohan.joung@sk.com> >> Signed-off-by: Chao Yu <chao@kernel.org> >> --- >> fs/f2fs/segment.h | 9 ++++++++- >> 1 file changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h >> index 0465dc00b349..0773283babfa 100644 >> --- a/fs/f2fs/segment.h >> +++ b/fs/f2fs/segment.h >> @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, >> next = find_next_bit(free_i->free_segmap, >> start_segno + SEGS_PER_SEC(sbi), start_segno); >> if (next >= start_segno + usable_segs) { >> - if (test_and_clear_bit(secno, free_i->free_secmap)) >> + if (test_and_clear_bit(secno, free_i->free_secmap)) { >> free_i->free_sections++; >> + >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) >> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; >> + >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) >> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; >> + } >> } >> } >> skip_free: >> -- >> 2.33.0
On 4/7/25 10:08, Chao Yu wrote: > On 4/5/25 03:55, Jaegeuk Kim wrote: >> Hi Yohan, >> >> I modified this patch after applying the clean up by >> >> https://lore.kernel.org/linux-f2fs-devel/20250404195442.413945-1-jaegeuk@kernel.org/T/#u >> >> --- a/fs/f2fs/segment.h >> +++ b/fs/f2fs/segment.h >> @@ -486,6 +486,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, >> >> free_i->free_sections++; >> >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) >> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) >> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; > > Reviewed-by: Chao Yu <chao@kernel.org> Oh, can we add Fixes line to make it to be merged into stable kernel? Thanks, > > Thanks, > >> + >> unlock_out: >> spin_unlock(&free_i->segmap_lock); >> } >> >> On 04/04, yohan.joung wrote: >>> When selecting a victim using next_victim_seg in a large section, the >>> selected section might already have been cleared and designated as the >>> new current section, making it actively in use. >>> This behavior causes inconsistency between the SIT and SSA. >>> >>> F2FS-fs (dm-54): Inconsistent segment (70961) type [0, 1] in SSA and SIT >>> Call trace: >>> dump_backtrace+0xe8/0x10c >>> show_stack+0x18/0x28 >>> dump_stack_lvl+0x50/0x6c >>> dump_stack+0x18/0x28 >>> f2fs_stop_checkpoint+0x1c/0x3c >>> do_garbage_collect+0x41c/0x271c >>> f2fs_gc+0x27c/0x828 >>> gc_thread_func+0x290/0x88c >>> kthread+0x11c/0x164 >>> ret_from_fork+0x10/0x20 >>> >>> issue scenario >>> segs_per_sec=2 >>> - seg#0 and seg#1 are all dirty >>> - all valid blocks are removed in seg#1 >>> - gc select this sec and next_victim_seg=seg#0 >>> - migrate seg#0, next_victim_seg=seg#1 >>> - checkpoint -> sec(seg#0, seg#1) becomes free >>> - allocator assigns sec(seg#0, seg#1) to curseg >>> - gc tries to migrate seg#1 >>> >>> Signed-off-by: yohan.joung <yohan.joung@sk.com> >>> Signed-off-by: Chao Yu <chao@kernel.org> >>> --- >>> fs/f2fs/segment.h | 9 ++++++++- >>> 1 file changed, 8 insertions(+), 1 deletion(-) >>> >>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h >>> index 0465dc00b349..0773283babfa 100644 >>> --- a/fs/f2fs/segment.h >>> +++ b/fs/f2fs/segment.h >>> @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, >>> next = find_next_bit(free_i->free_segmap, >>> start_segno + SEGS_PER_SEC(sbi), start_segno); >>> if (next >= start_segno + usable_segs) { >>> - if (test_and_clear_bit(secno, free_i->free_secmap)) >>> + if (test_and_clear_bit(secno, free_i->free_secmap)) { >>> free_i->free_sections++; >>> + >>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) >>> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; >>> + >>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) >>> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; >>> + } >>> } >>> } >>> skip_free: >>> -- >>> 2.33.0 >
On 04/07, Chao Yu wrote: > On 4/7/25 10:08, Chao Yu wrote: > > On 4/5/25 03:55, Jaegeuk Kim wrote: > >> Hi Yohan, > >> > >> I modified this patch after applying the clean up by > >> > >> https://lore.kernel.org/linux-f2fs-devel/20250404195442.413945-1-jaegeuk@kernel.org/T/#u > >> > >> --- a/fs/f2fs/segment.h > >> +++ b/fs/f2fs/segment.h > >> @@ -486,6 +486,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, > >> > >> free_i->free_sections++; > >> > >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) > >> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; > >> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) > >> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; > > > > Reviewed-by: Chao Yu <chao@kernel.org> > > Oh, can we add Fixes line to make it to be merged into stable kernel? Which one would be good to add? > > Thanks, > > > > > Thanks, > > > >> + > >> unlock_out: > >> spin_unlock(&free_i->segmap_lock); > >> } > >> > >> On 04/04, yohan.joung wrote: > >>> When selecting a victim using next_victim_seg in a large section, the > >>> selected section might already have been cleared and designated as the > >>> new current section, making it actively in use. > >>> This behavior causes inconsistency between the SIT and SSA. > >>> > >>> F2FS-fs (dm-54): Inconsistent segment (70961) type [0, 1] in SSA and SIT > >>> Call trace: > >>> dump_backtrace+0xe8/0x10c > >>> show_stack+0x18/0x28 > >>> dump_stack_lvl+0x50/0x6c > >>> dump_stack+0x18/0x28 > >>> f2fs_stop_checkpoint+0x1c/0x3c > >>> do_garbage_collect+0x41c/0x271c > >>> f2fs_gc+0x27c/0x828 > >>> gc_thread_func+0x290/0x88c > >>> kthread+0x11c/0x164 > >>> ret_from_fork+0x10/0x20 > >>> > >>> issue scenario > >>> segs_per_sec=2 > >>> - seg#0 and seg#1 are all dirty > >>> - all valid blocks are removed in seg#1 > >>> - gc select this sec and next_victim_seg=seg#0 > >>> - migrate seg#0, next_victim_seg=seg#1 > >>> - checkpoint -> sec(seg#0, seg#1) becomes free > >>> - allocator assigns sec(seg#0, seg#1) to curseg > >>> - gc tries to migrate seg#1 > >>> > >>> Signed-off-by: yohan.joung <yohan.joung@sk.com> > >>> Signed-off-by: Chao Yu <chao@kernel.org> > >>> --- > >>> fs/f2fs/segment.h | 9 ++++++++- > >>> 1 file changed, 8 insertions(+), 1 deletion(-) > >>> > >>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h > >>> index 0465dc00b349..0773283babfa 100644 > >>> --- a/fs/f2fs/segment.h > >>> +++ b/fs/f2fs/segment.h > >>> @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, > >>> next = find_next_bit(free_i->free_segmap, > >>> start_segno + SEGS_PER_SEC(sbi), start_segno); > >>> if (next >= start_segno + usable_segs) { > >>> - if (test_and_clear_bit(secno, free_i->free_secmap)) > >>> + if (test_and_clear_bit(secno, free_i->free_secmap)) { > >>> free_i->free_sections++; > >>> + > >>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) > >>> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; > >>> + > >>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) > >>> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; > >>> + } > >>> } > >>> } > >>> skip_free: > >>> -- > >>> 2.33.0 > >
On 4/10/25 11:53, Jaegeuk Kim wrote: > On 04/07, Chao Yu wrote: >> On 4/7/25 10:08, Chao Yu wrote: >>> On 4/5/25 03:55, Jaegeuk Kim wrote: >>>> Hi Yohan, >>>> >>>> I modified this patch after applying the clean up by >>>> >>>> https://lore.kernel.org/linux-f2fs-devel/20250404195442.413945-1-jaegeuk@kernel.org/T/#u >>>> >>>> --- a/fs/f2fs/segment.h >>>> +++ b/fs/f2fs/segment.h >>>> @@ -486,6 +486,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, >>>> >>>> free_i->free_sections++; >>>> >>>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) >>>> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; >>>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) >>>> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; >>> >>> Reviewed-by: Chao Yu <chao@kernel.org> >> >> Oh, can we add Fixes line to make it to be merged into stable kernel? > > Which one would be good to add? I guess this one: Fixes: e3080b0120a1 ("f2fs: support subsectional garbage collection") Thanks, > >> >> Thanks, >> >>> >>> Thanks, >>> >>>> + >>>> unlock_out: >>>> spin_unlock(&free_i->segmap_lock); >>>> } >>>> >>>> On 04/04, yohan.joung wrote: >>>>> When selecting a victim using next_victim_seg in a large section, the >>>>> selected section might already have been cleared and designated as the >>>>> new current section, making it actively in use. >>>>> This behavior causes inconsistency between the SIT and SSA. >>>>> >>>>> F2FS-fs (dm-54): Inconsistent segment (70961) type [0, 1] in SSA and SIT >>>>> Call trace: >>>>> dump_backtrace+0xe8/0x10c >>>>> show_stack+0x18/0x28 >>>>> dump_stack_lvl+0x50/0x6c >>>>> dump_stack+0x18/0x28 >>>>> f2fs_stop_checkpoint+0x1c/0x3c >>>>> do_garbage_collect+0x41c/0x271c >>>>> f2fs_gc+0x27c/0x828 >>>>> gc_thread_func+0x290/0x88c >>>>> kthread+0x11c/0x164 >>>>> ret_from_fork+0x10/0x20 >>>>> >>>>> issue scenario >>>>> segs_per_sec=2 >>>>> - seg#0 and seg#1 are all dirty >>>>> - all valid blocks are removed in seg#1 >>>>> - gc select this sec and next_victim_seg=seg#0 >>>>> - migrate seg#0, next_victim_seg=seg#1 >>>>> - checkpoint -> sec(seg#0, seg#1) becomes free >>>>> - allocator assigns sec(seg#0, seg#1) to curseg >>>>> - gc tries to migrate seg#1 >>>>> >>>>> Signed-off-by: yohan.joung <yohan.joung@sk.com> >>>>> Signed-off-by: Chao Yu <chao@kernel.org> >>>>> --- >>>>> fs/f2fs/segment.h | 9 ++++++++- >>>>> 1 file changed, 8 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h >>>>> index 0465dc00b349..0773283babfa 100644 >>>>> --- a/fs/f2fs/segment.h >>>>> +++ b/fs/f2fs/segment.h >>>>> @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, >>>>> next = find_next_bit(free_i->free_segmap, >>>>> start_segno + SEGS_PER_SEC(sbi), start_segno); >>>>> if (next >= start_segno + usable_segs) { >>>>> - if (test_and_clear_bit(secno, free_i->free_secmap)) >>>>> + if (test_and_clear_bit(secno, free_i->free_secmap)) { >>>>> free_i->free_sections++; >>>>> + >>>>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) >>>>> + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; >>>>> + >>>>> + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) >>>>> + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; >>>>> + } >>>>> } >>>>> } >>>>> skip_free: >>>>> -- >>>>> 2.33.0 >>>
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h index 0465dc00b349..0773283babfa 100644 --- a/fs/f2fs/segment.h +++ b/fs/f2fs/segment.h @@ -474,8 +474,15 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi, next = find_next_bit(free_i->free_segmap, start_segno + SEGS_PER_SEC(sbi), start_segno); if (next >= start_segno + usable_segs) { - if (test_and_clear_bit(secno, free_i->free_secmap)) + if (test_and_clear_bit(secno, free_i->free_secmap)) { free_i->free_sections++; + + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[BG_GC]) == secno) + sbi->next_victim_seg[BG_GC] = NULL_SEGNO; + + if (GET_SEC_FROM_SEG(sbi, sbi->next_victim_seg[FG_GC]) == secno) + sbi->next_victim_seg[FG_GC] = NULL_SEGNO; + } } } skip_free: