| Message ID | 20250312102005.2893698-1-guochunhai@vivo.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [f2fs-dev,v3] f2fs: fix missing discard candidates in fstrim | expand |
On 3/12/25 18:20, 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 <guochunhai@vivo.com> Reviewed-by: Chao Yu <chao@kernel.org> To Jaegeuk, I'm fine w/ this change, but discard is critical, could you please double check it? Thanks,
On 03/12, 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. I think we need to fix the above logic. > > 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@vivo.com> > --- > v2->v3: Add f2fs_bug_on() to make sure it never issues discard to valid data's block address. > v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs(). > v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/ > --- > fs/f2fs/segment.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c > index 86e547f008f9..c8ad8e3bfebb 100644 > --- a/fs/f2fs/segment.c > +++ b/fs/f2fs/segment.c > @@ -2075,7 +2075,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; > @@ -2097,9 +2096,10 @@ 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]; > + for (i = 0; i < entries; i++) { > + dmap[i] = ~discard_map[i]; > + f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]); > + } > > while (force || SM_I(sbi)->dcc_info->nr_discards <= > SM_I(sbi)->dcc_info->max_discards) { > -- > 2.34.1
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 86e547f008f9..c8ad8e3bfebb 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -2075,7 +2075,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; @@ -2097,9 +2096,10 @@ 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]; + for (i = 0; i < entries; i++) { + dmap[i] = ~discard_map[i]; + f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]); + } while (force || SM_I(sbi)->dcc_info->nr_discards <= SM_I(sbi)->dcc_info->max_discards) {
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@vivo.com> --- v2->v3: Add f2fs_bug_on() to make sure it never issues discard to valid data's block address. v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs(). v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/ --- fs/f2fs/segment.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)