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

[Chao Yu <chao@kernel.org>]

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