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

[Chao Yu <chao@kernel.org>]

On 3/13/25 10:25, Chunhai Guo wrote:
> 在 1/20/2025 8:25 PM, Chao Yu 写道:
>> 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)?
> 
> 
> This issue is found with LFS rather than SSR. Here's what happens: some
> data blocks are allocated for a file in the current active segment, and
> then the file is deleted, resulting in all valid data blocks in the
> current active segment being deleted before the following checkpoint.
> This issue is easy to reproduce with the following operations:
> 
> 
> # mkfs.f2fs -f /dev/nvme2n1
> # mount -t f2fs /dev/nvme2n1 /vtmp/mnt/f2fs
> # dd if=/dev/nvme0n1 of=/vtmp/mnt/f2fs/1.bin bs=4k count=256
> # sync
> # rm /vtmp/mnt/f2fs/1.bin
> # umount /vtmp/mnt/f2fs
> # dump.f2fs /dev/nvme2n1 | grep "checkpoint state"
> Info: checkpoint state = 45 :  crc compacted_summary unmount ----
> 'trimmed' flag is missing
> 
> The pending discard count in that segment indeed falls within the range
> of (0, 512).

Please add this testcase into commit message, otherwise it looks
good to me, feel free to add:

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,

> 
> Thanks,
>> 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;
> 
>