| 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
| ||
|
Message-ID: <c3726bec-6c14-459f-b5c4-6e8c9fcefc1d@vivo.com>
Date: Sun, 19 Jan 2025 13:34:06 +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 1/2] f2fs: fix missing discard candidates in fstrim
在 1/13/2025 9:32 PM, Chao Yu 写道:
> On 2025/1/9 18:03, Chunhai Guo wrote:
>> 在 1/8/2025 8:46 PM, Chao Yu 写道:
>>> On 2025/1/3 16:07, Chunhai Guo wrote:
>>>> 在 1/3/2025 11:26 AM, Chao Yu 写道:
>>>>> On 2025/1/2 18:13, Chunhai Guo wrote:
>>>>>> fstrim may miss candidates that need to be discarded in fstrim, as shown in
>>>>>> the examples below.
>>>>>> The root cause is that when cpc->reason is set with CP_DISCARD,
>>>>>> add_discard_addrs() expects that ckpt_valid_map and cur_valid_map have been
>>>>>> synced by seg_info_to_raw_sit() [1] and tries to find the candidates based
>>>>>> on ckpt_valid_map and discard_map. However, seg_info_to_raw_sit() does not
>>>>>> actually run before f2fs_exist_trim_candidates(), which results in failure.
>>>>> Chunhai,
>>>>>
>>>>> Can you please use nodiscard option due to fstrim stopped to return
>>>>> trimmed length after below commit:
>>>>>
>>>>> 5a6154920faf ("f2fs: don't issue discard commands in online discard is on")
>>>> Thank you for your explanation, but I guess this issue is not relevant
>>>> to this commit, and I understand that '0 B (0 bytes) trimmed' is fine.
>>>>
>>>> The real problem is that there are actually many candidates that should
>>>> be handled in fstrim, but it cannot find any of them.
>>>>
>>>> f2fs_trim_fs()
>>>> f2fs_write_checkpoint()
>>>> ...
>>>> if (cpc->reason & CP_DISCARD) {
>>>> if (!f2fs_exist_trim_candidates(sbi, cpc)) {
>>>> unblock_operations(sbi);
>>>> goto out; // Not candidate is found here and exit.
>>>> }
>>>> ...
>>>> }
>>>>
>>>>>> root# cp testfile /f2fs_mountpoint
>>>>>>
>>>>>> root# f2fs_io fiemap 0 1 /f2fs_mountpoint/testfile
>>>>>> Fiemap: offset = 0 len = 1
>>>>>> logical addr. physical addr. length flags
>>>>>> 0 0000000000000000 0000000406a00000 000000003d800000 00001000
>>>>>>
>>>>>> root# rm /f2fs_mountpoint/testfile
>>>>>>
>>>>>> root# fstrim -v -o 0x406a00000 -l 1024M /f2fs_mountpoint -- no candidate is found
>>>>>> /f2fs_mountpoint: 0 B (0 bytes) trimmed
>>>>>>
>>>>>> [1] Please refer to commit d7bc2484b8d4 ("f2fs: fix small discards not to
>>>>>> issue redundantly") for the relationship between seg_info_to_raw_sit() and
>>>>>> add_discard_addrs().
>>>>>>
>>>>>> Fixes: 25290fa5591d ("f2fs: return fs_trim if there is no candidate")
>>>>>> Signed-off-by: Chunhai Guo<guochunhai@...o.com>
>>>>>> ---
>>>>>> fs/f2fs/segment.c | 10 +++++-----
>>>>>> 1 file changed, 5 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>>>> index eade36c5ef13..8fe9f794b581 100644
>>>>>> --- a/fs/f2fs/segment.c
>>>>>> +++ b/fs/f2fs/segment.c
>>>>>> @@ -2070,7 +2070,7 @@ static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
>>>>>> }
>>>>>>
>>>>>> static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>>>> - bool check_only)
>>>>>> + bool synced, bool check_only)
>>>>>> {
>>>>>> int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
>>>>>> struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
>>>>>> @@ -2098,7 +2098,7 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>>>>
>>>>>> /* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>>>>>> for (i = 0; i < entries; i++)
>>>>>> - dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
>>>>>> + dmap[i] = synced ? ~ckpt_map[i] & ~discard_map[i] :
>>>>> I guess this condition "force ? ~ckpt_map[i] & ~discard_map[i]" didn't cover
>>>>> all below cases, thoughts?
>>>>> - ckpt_map[i] == 0 // write data, and then remove data before checkpoint
>>>>> - ckpt_map[i] != 0 // remove data existed in previous checkpoint
>>>> From the handling of ckpt_valid_map in update_sit_entry(), I guess the
>>>> condition can cover both cases.
>>>> For example, when the checkpoint is enabled, all the set bits in the
>>>> ckpt_valid_map remain set before the checkpoint (even when the blocks
>>>> are deleted), which makes it find all the right bits in both cases.
>>> My point is for fstrim case, we only need to check discard_map bitmap?
>>> once bit(s) in discard_map bitmap is zero, no matter the status of
>>> bit(s) in ckpt_map bitmap, we need to trigger a checkpoit for following
>>> discard submission?
>> Oh, yes. It is reasonable to check only the discard_map bitmap. What do
>> you think about the code below?
>>
>> for (i = 0; i < entries; i++) {
>> if (check_only)
>> dmap[i] = ~discard_map[i];
>> else
>> dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
>> (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
> How about this?
>
> dmap[i] = force ? ~discard_map[i] :
> (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
I think it is OK, too. I believe it is identical for "~discard_map[i]"
and "~ckpt_map[i] & ~discard_map[i]" for fstrim here.
Moreover, I think the code logic can be simplified for all cases by
finding all the discard blocks based only on discard_map, as shown in
the code below. This might result in more discard blocks being sent for
the segment during the first checkpoint after mounting, which were
originally expected to be sent only in fstrim. Regardless, these discard
blocks should eventually be sent, and the simplified code makes sense in
this context.
dmap[i] = ~discard_map[i];
I will send you the v2 patch for review.
Thanks,
> Thanks,
>
>
>> }
>>
>> Thanks,
>>
>>> Thanks,
>>>
>>>> Thanks,
>>>>
>>>>> Thanks,
>>>>>
>>>>>> (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
>>>>>>
>>>>>> while (force || SM_I(sbi)->dcc_info->nr_discards <=
>>>>>> @@ -3275,7 +3275,7 @@ bool f2fs_exist_trim_candidates(struct f2fs_sb_info *sbi,
>>>>>>
>>>>>> down_write(&SIT_I(sbi)->sentry_lock);
>>>>>> for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++) {
>>>>>> - if (add_discard_addrs(sbi, cpc, true)) {
>>>>>> + if (add_discard_addrs(sbi, cpc, false, true)) {
>>>>>> has_candidate = true;
>>>>>> break;
>>>>>> }
>>>>>> @@ -4611,7 +4611,7 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>> /* add discard candidates */
>>>>>> if (!(cpc->reason & CP_DISCARD)) {
>>>>>> cpc->trim_start = segno;
>>>>>> - add_discard_addrs(sbi, cpc, false);
>>>>>> + add_discard_addrs(sbi, cpc, false, false);
>>>>>> }
>>>>>>
>>>>>> if (to_journal) {
>>>>>> @@ -4653,7 +4653,7 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>>>> __u64 trim_start = cpc->trim_start;
>>>>>>
>>>>>> for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>>>>>> - add_discard_addrs(sbi, cpc, false);
>>>>>> + add_discard_addrs(sbi, cpc, true, false);
>>>>>>
>>>>>> cpc->trim_start = trim_start;
>>>>>> }
Powered by blists - more mailing lists