| Message ID | 20250102101310.580277-1-guochunhai@vivo.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [f2fs-dev,1/2] f2fs: fix missing discard candidates in fstrim | expand |
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") > > 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@vivo.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 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; > }
在 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@vivo.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. 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; >> }
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@vivo.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? 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; >>> } > >
在 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@vivo.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]; } 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; >>>> } >>
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@vivo.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]; 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; >>>>> } >>> >
在 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@vivo.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; >>>>>> }
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] : (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; }
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. 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@vivo.com> --- fs/f2fs/segment.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)