Re: [f2fs-dev] [PATCH v5] f2fs: prevent the current section from being selected as a victim during GC

[yohan.joung]

On Thu, 10 Apr 2025 14:12:39 +0900	[thread overview] "yohan.joung" <yohan.joung@xxxxxx> wrote:
> On Thu, 10 Apr 2025 03:53:07 +0000	[thread overview] Jaegeuk Kim via Linux-f2fs-devel <linux-f2fs-devel@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx/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@xxxxxxxxxx>
> > > 
> > > Oh, can we add Fixes line to make it to be merged into stable kernel?
> > 
> > Which one would be good to add?
> jaegeuk, please apply the patch above
> Thanks
sorry it is correct to apply the one below.
> > > 
> > > 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@xxxxxx>
> > > >>> Signed-off-by: Chao Yu <chao@xxxxxxxxxx>
> > > >>> ---
> > > >>>  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