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

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

Posted by yohan.joung 8 months, 3 weeks ago

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 | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 0465dc00b349..14d18bcf3559 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -460,6 +460,7 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
 		unsigned int segno, bool inmem)
 {
 	struct free_segmap_info *free_i = FREE_I(sbi);
+	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
 	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
 	unsigned int start_segno = GET_SEG_FROM_SEC(sbi, secno);
 	unsigned int next;
@@ -476,6 +477,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
 		if (next >= start_segno + usable_segs) {
 			if (test_and_clear_bit(secno, free_i->free_secmap))
 				free_i->free_sections++;
+
+			if (test_and_clear_bit(secno, dirty_i->victim_secmap)) {
+				sbi->next_victim_seg[BG_GC] = NULL_SEGNO;
+				sbi->next_victim_seg[FG_GC] = NULL_SEGNO;
+			}
 		}
 	}
 skip_free:
-- 
2.33.0

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

Posted by Chao Yu 8 months, 3 weeks ago

On 4/2/25 08:52, 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 | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
> index 0465dc00b349..14d18bcf3559 100644
> --- a/fs/f2fs/segment.h
> +++ b/fs/f2fs/segment.h
> @@ -460,6 +460,7 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
>  		unsigned int segno, bool inmem)
>  {
>  	struct free_segmap_info *free_i = FREE_I(sbi);
> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>  	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>  	unsigned int start_segno = GET_SEG_FROM_SEC(sbi, secno);
>  	unsigned int next;
> @@ -476,6 +477,11 @@ static inline void __set_test_and_free(struct f2fs_sb_info *sbi,
>  		if (next >= start_segno + usable_segs) {
>  			if (test_and_clear_bit(secno, free_i->free_secmap))
>  				free_i->free_sections++;
> +
> +			if (test_and_clear_bit(secno, dirty_i->victim_secmap)) {
> +				sbi->next_victim_seg[BG_GC] = NULL_SEGNO;
> +				sbi->next_victim_seg[FG_GC] = NULL_SEGNO;

sbi->next_victim_seg[FG_GC] relies on sbi->cur_victim_sec?

If sbi->next_victim_seg[BG_GC] is not equal to secno, will we still need to
nullify sbi->next_victim_seg[BG_GC]?

We have cleared bit in victim_secmap after we tag a section as prefree, right?

- locate_dirty_segment
 - __locate_dirty_segment
 - __remove_dirty_segment
  - clear_bit(GET_SEC_FROM_SEG(sbi, segno), dirty_i->victim_secmap);

Thanks,

> +			}
>  		}
>  	}
>  skip_free:

f2fs: prevent the current section from being selected as a victim during GC

Posted by yohan.joung 8 months, 3 weeks ago

>On 4/2/25 08:52, 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 | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h index
>> 0465dc00b349..14d18bcf3559 100644
>> --- a/fs/f2fs/segment.h
>> +++ b/fs/f2fs/segment.h
>> @@ -460,6 +460,7 @@ static inline void __set_test_and_free(struct
>f2fs_sb_info *sbi,
>>  		unsigned int segno, bool inmem)
>>  {
>>  	struct free_segmap_info *free_i = FREE_I(sbi);
>> +	struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
>>  	unsigned int secno = GET_SEC_FROM_SEG(sbi, segno);
>>  	unsigned int start_segno = GET_SEG_FROM_SEC(sbi, secno);
>>  	unsigned int next;
>> @@ -476,6 +477,11 @@ static inline void __set_test_and_free(struct
>f2fs_sb_info *sbi,
>>  		if (next >= start_segno + usable_segs) {
>>  			if (test_and_clear_bit(secno, free_i->free_secmap))
>>  				free_i->free_sections++;
>> +
>> +			if (test_and_clear_bit(secno, dirty_i->victim_secmap))
>{
>> +				sbi->next_victim_seg[BG_GC] = NULL_SEGNO;
>> +				sbi->next_victim_seg[FG_GC] = NULL_SEGNO;
>
>sbi->next_victim_seg[FG_GC] relies on sbi->cur_victim_sec?
right  
>
>If sbi->next_victim_seg[BG_GC] is not equal to secno, will we still need to
>nullify sbi->next_victim_seg[BG_GC]?
Changed to remove only when equal
>
>We have cleared bit in victim_secmap after we tag a section as prefree,
>right?
You are right 
change victim_secmap to not be used
In the end, I think I'll have to do this to directly compare segno, 
as you suggested.
I tested this and it worked fine.

Thanks
>
>- locate_dirty_segment
> - __locate_dirty_segment
> - __remove_dirty_segment
>  - clear_bit(GET_SEC_FROM_SEG(sbi, segno), dirty_i->victim_secmap);
>
>Thanks,
>
>> +			}
>>  		}
>>  	}
>>  skip_free: