From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id F20071BD007 for ; Mon, 20 Jan 2025 11:45:57 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737373558; cv=none; b=TPlR1cVP6KgyGsTx+7Yyy3HrhOzhMB3xYayDZTJ/knHwQVSaj5ffmF+sNmEfvw3FMMkStIsKJe7rQ8K+YGm03aq8/ke0LGb1KywkaSV+/TIpIy93cgfwD+zJo6qYlIPxsJg8kqs4l0TekWJMNAQ94UyhOtI02fCQ5ymuxJ/789Q= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1737373558; c=relaxed/simple; bh=v5EFNyYmGybl5D6TEXBjTvplmgaf3jTdVmghx0iQKn8=; h=Message-ID:Date:MIME-Version:Cc:Subject:To:References:From: In-Reply-To:Content-Type; b=Me98ro0zZqQvLYbUZbxTaSHCSvJ6j3NNQVq+62dhoTkvy0fqtEf00MXN41imTUurEPy0pS5cI7Rt0L05SPuNo8HSTLenARoYNyCdbmqhyk5hB8SVAvF61pS4p7V+JXae3RddPBl/Wa4Y4cJa8fXHpzCbuSxZ+DW15YwdP/Xw8XI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=E0oxIIQI; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="E0oxIIQI" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 89972C4CEDD; Mon, 20 Jan 2025 11:45:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1737373557; bh=v5EFNyYmGybl5D6TEXBjTvplmgaf3jTdVmghx0iQKn8=; h=Date:Cc:Subject:To:References:From:In-Reply-To:From; b=E0oxIIQIdzn3PfKwmCFtewHpsdJpi2edxPMv0ffGO1INQksQ2q9GSP6YBcGywAT4o JxmgdeJFDcOTJZa8ZK5QGsEtMix2G8qC5tu3OZ6apPb2OgoPwzCbMTg7AAaU9Seanr UaFLJpMr6hawCj7XoE7kxKZcfLsHHhOIDXVMlWnyGhWUSRs1PP/apmVlWA+zLRe2Aj f+ErxkY88mUnBLkOf14hVhTKdSBQ53ABOTb4h05LwenVnUCCg2t7voFqJX+jiDAHEn 86Us8y8DmXzXco/3OFN3yhbfW7RCHRu00AAOSWvfvLSwq4S4zQdYC+3GvfzmHW80Qx wPqWZup+PqqiA== Message-ID: <77fa4633-f7db-4daa-a9e1-3fdb5bf9dd1d@kernel.org> Date: Mon, 20 Jan 2025 19:45:54 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Cc: chao@kernel.org, linux-f2fs-devel@lists.sourceforge.net, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] f2fs: fix missing discard candidates in fstrim To: Chunhai Guo , jaegeuk@kernel.org References: <20250119140834.1061145-1-guochunhai@vivo.com> Content-Language: en-US From: Chao Yu In-Reply-To: <20250119140834.1061145-1-guochunhai@vivo.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 1/19/25 22:08, Chunhai Guo wrote: > 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 > --- > 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]; discard is critical, we need more sanity check here, maybe: /* never issue discard to valid data's block address */ f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]); Can you please check this? Thanks, > > while (force || SM_I(sbi)->dcc_info->nr_discards <= > SM_I(sbi)->dcc_info->max_discards) {