Re: [f2fs-dev] [PATCH] f2fs: do not use granularity control for segment or section unit discard

[Chao Yu]

On 2025/2/20 23:49, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@xxxxxxxxxx>
>
> When we support segment or section unit discard, we should only focus on
> how actively we submit discard commands for only one type of size, such
> as segment or section. In this case, we don't have to manage smaller
> sized discards.
>
> Reported-by: Yohan Joung <yohan.joung@xxxxxx>
> Signed-off-by: Daeho Jeong <daehojeong@xxxxxxxxxx>
> ---
>   fs/f2fs/segment.c | 29 ++++++++++++++++++++---------
>   1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index c282e8a0a2ec..4316ff7aa0d1 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -1661,12 +1661,20 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>   				f2fs_time_over(sbi, UMOUNT_DISCARD_TIMEOUT))
>   			break;
>   
> -		if (i + 1 < dpolicy->granularity)
> -			break;
> +		/*
> +		 * Do not granularity control for segment or section
> +		 * unit discard, since we have only one type of discard length.
> +		 */
> +		if (f2fs_block_unit_discard(sbi)) {
> +			if (i + 1 < dpolicy->granularity)
> +				break;
>   
> -		if (i + 1 < dcc->max_ordered_discard && dpolicy->ordered) {
> -			__issue_discard_cmd_orderly(sbi, dpolicy, &issued);
> -			return issued;
> +			if (i + 1 < dcc->max_ordered_discard &&
> +					dpolicy->ordered) {
> +				__issue_discard_cmd_orderly(sbi, dpolicy,
> +						&issued);
> +				return issued;
> +			}
>   		}
>   
>   		pend_list = &dcc->pend_list[i];
> @@ -1701,6 +1709,13 @@ static int __issue_discard_cmd(struct f2fs_sb_info *sbi,
>   
>   		if (issued >= dpolicy->max_requests || io_interrupted)
>   			break;
> +
> +		/*
> +		 * We only use the largest discard unit for segment or
> +		 * section unit discard.
> +		 */
> +		if (!f2fs_block_unit_discard(sbi))
> +			break;
>   	}
>   
>   	if (dpolicy->type == DPOLICY_UMOUNT && issued) {
> @@ -2320,10 +2335,6 @@ static int create_discard_cmd_control(struct f2fs_sb_info *sbi)
>   	dcc->discard_granularity = DEFAULT_DISCARD_GRANULARITY;
>   	dcc->max_ordered_discard = DEFAULT_MAX_ORDERED_DISCARD_GRANULARITY;
>   	dcc->discard_io_aware = DPOLICY_IO_AWARE_ENABLE;
> -	if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SEGMENT)
> -		dcc->discard_granularity = BLKS_PER_SEG(sbi);
> -	else if (F2FS_OPTION(sbi).discard_unit == DISCARD_UNIT_SECTION)
> -		dcc->discard_granularity = BLKS_PER_SEC(sbi);

Hi Daeho,

I think this bug was introduced by commit 4f993264fe29 ("f2fs: introduce
discard_unit mount option"), since it set discard_granularity to section
size incorrectly for discard_unit=section mount option, once section size
is large than segment size, discard_granularity will be larger than 512.

However, w/ current implementation, we only support range of [1, 512] for
discard_granularity parameter, resulting in failing to submitting all
dicards.

So, what do you think of setting discard_granularity to 512 for both
discard_unit=segment and discard_unit=section mount option, as I proposed
in [1]? Then, discard_thread in DPOLICY_BG mode can submit those large-sized
discards.

[1] https://lore.kernel.org/linux-f2fs-devel/53598146-1f01-41ad-980e-9f4b989e81ab@xxxxxxxxxx/

Thanks,

>   
>   	INIT_LIST_HEAD(&dcc->entry_list);
>   	for (i = 0; i < MAX_PLIST_NUM; i++)