linux-kernel - [PATCH v2] f2fs: fix missing discard candidates in fstrim

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-next>] [day] [month] [year] [list]

Message-Id: <20250119140834.1061145-1-guochunhai@vivo.com>
Date: Sun, 19 Jan 2025 07:08:34 -0700
From: Chunhai Guo <guochunhai@...o.com>
To: chao@...nel.org,
	jaegeuk@...nel.org
Cc: linux-f2fs-devel@...ts.sourceforge.net,
	linux-kernel@...r.kernel.org,
	Chunhai Guo <guochunhai@...o.com>
Subject: [PATCH v2] f2fs: fix missing discard candidates in fstrim

fstrim may miss candidates that need to be discarded, 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 it 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(), resulting in the failure.

The code logic can be simplified for all cases by finding all the
discard blocks based only on discard_map. 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.

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

Relevant code process of the root cause:
f2fs_trim_fs()
    f2fs_write_checkpoint()
        ...
        if (cpc->reason & CP_DISCARD) {
                if (!f2fs_exist_trim_candidates(sbi, cpc)) {
                    unblock_operations(sbi);
                    goto out; // No candidates are found here, and it exits.
                }
            ...
        }

[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>
---
v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/
v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs().
---
 fs/f2fs/segment.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 13ee73a3c481..25ea892a42dd 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2074,8 +2074,6 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
 {
 	int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
 	struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
-	unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
-	unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
 	unsigned long *discard_map = (unsigned long *)se->discard_map;
 	unsigned long *dmap = SIT_I(sbi)->tmp_map;
 	unsigned int start = 0, end = -1;
@@ -2100,8 +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] :
-				(cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
+		dmap[i] = ~discard_map[i];
 
 	while (force || SM_I(sbi)->dcc_info->nr_discards <=
 				SM_I(sbi)->dcc_info->max_discards) {
-- 
2.34.1