Re: [PATCH 1/2] f2fs: fix missing discard candidates in fstrim

From: Chao Yu
Date: Mon Jan 13 2025 - 08:32:52 EST

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@xxxxxxxx>
---
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];

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;
}