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

[Chunhai Guo]

在 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).

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@xxxxxxxx>
>> ---
>> v1: https://lore.kernel.org/linux-f2fs-devel/20241203065108.2763436-1-guochunhai@xxxxxxxx/
>> 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;