Re: [f2fs-dev] [PATCH 2/7] f2fs: read summary blocks with the correct amount for migration_granularity

[Chao Yu <chao@kernel.org>]

On 2024/8/30 5:52, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> Now we do readahead for a full section by not considering
> migration_granularity and it triggers unnecessary read. So, make it read
> with the correct amount.
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
>   fs/f2fs/gc.c | 33 ++++++++++++++++++++-------------
>   1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
> index 46e3bc26b78a..b5d3fd40b17a 100644
> --- a/fs/f2fs/gc.c
> +++ b/fs/f2fs/gc.c
> @@ -1708,24 +1708,33 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   	struct blk_plug plug;
>   	unsigned int segno = start_segno;
>   	unsigned int end_segno = start_segno + SEGS_PER_SEC(sbi);
> +	unsigned int sec_end_segno;
>   	int seg_freed = 0, migrated = 0;
>   	unsigned char type = IS_DATASEG(get_seg_entry(sbi, segno)->type) ?
>   						SUM_TYPE_DATA : SUM_TYPE_NODE;
>   	unsigned char data_type = (type == SUM_TYPE_DATA) ? DATA : NODE;
>   	int submitted = 0;
>   
> -	if (__is_large_section(sbi))
> -		end_segno = rounddown(end_segno, SEGS_PER_SEC(sbi));
> +	if (__is_large_section(sbi)) {
> +		sec_end_segno = rounddown(end_segno, SEGS_PER_SEC(sbi));
>   
> -	/*
> -	 * zone-capacity can be less than zone-size in zoned devices,
> -	 * resulting in less than expected usable segments in the zone,
> -	 * calculate the end segno in the zone which can be garbage collected
> -	 */
> -	if (f2fs_sb_has_blkzoned(sbi))
> -		end_segno -= SEGS_PER_SEC(sbi) -
> +		/*
> +		 * zone-capacity can be less than zone-size in zoned devices,
> +		 * resulting in less than expected usable segments in the zone,
> +		 * calculate the end segno in the zone which can be garbage
> +		 * collected
> +		 */
> +		if (f2fs_sb_has_blkzoned(sbi))
> +			sec_end_segno -= SEGS_PER_SEC(sbi) -
>   					f2fs_usable_segs_in_sec(sbi, segno);
>   
> +		if (gc_type == BG_GC)
> +			end_segno = start_segno + sbi->migration_granularity;
> +
> +		if (end_segno > sec_end_segno)
> +			end_segno = sec_end_segno;
> +	}
> +
>   	sanity_check_seg_type(sbi, get_seg_entry(sbi, segno)->type);
>   
>   	/* readahead multi ssa blocks those have contiguous address */
> @@ -1762,9 +1771,6 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   
>   		if (get_valid_blocks(sbi, segno, false) == 0)
>   			goto freed;
> -		if (gc_type == BG_GC && __is_large_section(sbi) &&
> -				migrated >= sbi->migration_granularity)

It seems we change the logic from migrating "migration_granularity" segments which
has valid blocks to scanning "migration_granularity" segments and try migrating
valid blocks in those segments.

IIUC, when background GC recycle sparse zone, it will take gc thread more round,
it seems low efficient. How do you think of keeping previous implementation?

Thanks,

> -			goto skip;
>   		if (!PageUptodate(sum_page) || unlikely(f2fs_cp_error(sbi)))
>   			goto skip;
>   
> @@ -1803,7 +1809,8 @@ static int do_garbage_collect(struct f2fs_sb_info *sbi,
>   
>   		if (__is_large_section(sbi))
>   			sbi->next_victim_seg[gc_type] =
> -				(segno + 1 < end_segno) ? segno + 1 : NULL_SEGNO;
> +				(segno + 1 < sec_end_segno) ?
> +					segno + 1 : NULL_SEGNO;
>   skip:
>   		f2fs_put_page(sum_page, 0);
>   	}