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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <47f3cf96-62fc-4bb2-936f-dd4564e4afe5@vivo.com>
Date: Mon, 17 Mar 2025 08:27:04 +0000
From: Chunhai Guo <guochunhai@...o.com>
To: Chao Yu <chao@...nel.org>, Chunhai Guo <guochunhai@...o.com>,
	"jaegeuk@...nel.org" <jaegeuk@...nel.org>
CC: "linux-f2fs-devel@...ts.sourceforge.net"
	<linux-f2fs-devel@...ts.sourceforge.net>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] f2fs: fix missing discard for active segments

在 3/17/2025 3:11 PM, Chao Yu 写道:
> 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
> Chunhai,
>
> Thank you for providing testcase, are you interest in upstream this
> as a f2fs testcase to xfstests?


Yes, but I may not be able to start this work for a few days since I
will be busy in the next few days.

Thanks,


>
> Thanks,
>
>> 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@...o.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;
>>