Re: [PATCH v2] f2fs: fix missing discard for active segments

[Chao Yu <chao@kernel.org>]

On 1/9/25 20:27, Chunhai Guo wrote:
> During a checkpoint, the current active segment X may not be handled
> properly. This occurs when segment X has 0 valid blocks and a non-zero

How does this happen? Allocator selects a dirty segment w/ SSR? and the
left valid data blocks were deleted later before following checkpoint?

If so, pending discard count in that segment should be in range of (0, 512)?

Thanks,

> number of discard blocks, for the following reasons:
> 
> locate_dirty_segment() does not mark any active segment as a prefree
> segment. As a result, segment X is not included in dirty_segmap[PRE], and
> f2fs_clear_prefree_segments() skips it when handling prefree segments.
> 
> add_discard_addrs() skips any segment with 0 valid blocks, so segment X is
> also skipped.
> 
> Consequently, no `struct discard_cmd` is actually created for segment X.
> However, the ckpt_valid_map and cur_valid_map of segment X are synced by
> seg_info_to_raw_sit() during the current checkpoint process. As a result,
> it cannot find the missing discard bits even in subsequent checkpoints.
> Consequently, the value of sbi->discard_blks remains non-zero. Thus, when
> f2fs is umounted, CP_TRIMMED_FLAG will not be set due to the non-zero
> sbi->discard_blks.
> 
> Relevant code process:
> 
> f2fs_write_checkpoint()
>      f2fs_flush_sit_entries()
>           list_for_each_entry_safe(ses, tmp, head, set_list) {
>               for_each_set_bit_from(segno, bitmap, end) {
>                   ...
>                   add_discard_addrs(sbi, cpc, false); // skip segment X due to its 0 valid blocks
>                   ...
>                   seg_info_to_raw_sit(); // sync ckpt_valid_map with cur_valid_map for segment X
>                   ...
>               }
>           }
>      f2fs_clear_prefree_segments(); // segment X is not included in dirty_segmap[PRE] and is skipped
> 
> Since add_discard_addrs() can handle active segments with non-zero valid
> blocks, it is reasonable to fix this issue by allowing it to also handle
> active segments with 0 valid blocks.
> 
> Fixes: b29555505d81 ("f2fs: add key functions for small discards")
> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
> ---
> v1: https://lore.kernel.org/linux-f2fs-devel/20241203065108.2763436-1-guochunhai@vivo.com/
> v1->v2:
>   - Modify the commit message to make it easier to understand.
>   - Add fixes to the commit.
> ---
>   fs/f2fs/segment.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 86e547f008f9..13ee73a3c481 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2090,7 +2090,9 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>   		return false;
>   
>   	if (!force) {
> -		if (!f2fs_realtime_discard_enable(sbi) || !se->valid_blocks ||
> +		if (!f2fs_realtime_discard_enable(sbi) ||
> +			(!se->valid_blocks &&
> +				!IS_CURSEG(sbi, cpc->trim_start)) ||
>   			SM_I(sbi)->dcc_info->nr_discards >=
>   				SM_I(sbi)->dcc_info->max_discards)
>   			return false;