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

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

[Chunhai Guo]

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(-)

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

[PATCH 2/2] f2fs: fix missing small discard in fstrim

[Chunhai Guo]

If userspace issues an fstrim with a range that does not include all
segments with small discards, these segments will be reused without being
discarded. This patch fixes this issue.
This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
discard prefree segments").

Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
---
 fs/f2fs/segment.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 8fe9f794b581..af9a62591c49 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	struct list_head *head = &SM_I(sbi)->sit_entry_set;
 	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
 	struct seg_entry *se;
+	bool force = (cpc->reason & CP_DISCARD);
+	__u64 trim_start = cpc->trim_start;
 
 	down_write(&sit_i->sentry_lock);
 
@@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 #endif
 
 			/* add discard candidates */
-			if (!(cpc->reason & CP_DISCARD)) {
+			if (!force || (force &&
+					(segno < trim_start ||
+					 segno > cpc->trim_end))) {
 				cpc->trim_start = segno;
 				add_discard_addrs(sbi, cpc, false, false);
 			}
@@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	f2fs_bug_on(sbi, !list_empty(head));
 	f2fs_bug_on(sbi, sit_i->dirty_sentries);
 out:
-	if (cpc->reason & CP_DISCARD) {
-		__u64 trim_start = cpc->trim_start;
+	if (force) {
+		cpc->trim_start = trim_start;
 
 		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
 			add_discard_addrs(sbi, cpc, true, false);
-- 
2.34.1

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

[Chao Yu]

On 2025/1/2 18:13, Chunhai Guo wrote:
> If userspace issues an fstrim with a range that does not include all
> segments with small discards, these segments will be reused without being

I didn't get it, if fstrim didn't cover those segments, why do we need to
issue small discard for out-of-range segments?

Thanks,

> discarded. This patch fixes this issue.
> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
> discard prefree segments").
> 
> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
> ---
>   fs/f2fs/segment.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 8fe9f794b581..af9a62591c49 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>   	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>   	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>   	struct seg_entry *se;
> +	bool force = (cpc->reason & CP_DISCARD);
> +	__u64 trim_start = cpc->trim_start;
>   
>   	down_write(&sit_i->sentry_lock);
>   
> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>   #endif
>   
>   			/* add discard candidates */
> -			if (!(cpc->reason & CP_DISCARD)) {
> +			if (!force || (force &&
> +					(segno < trim_start ||
> +					 segno > cpc->trim_end))) {
>   				cpc->trim_start = segno;
>   				add_discard_addrs(sbi, cpc, false, false);
>   			}
> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>   	f2fs_bug_on(sbi, !list_empty(head));
>   	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>   out:
> -	if (cpc->reason & CP_DISCARD) {
> -		__u64 trim_start = cpc->trim_start;
> +	if (force) {
> +		cpc->trim_start = trim_start;
>   
>   		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>   			add_discard_addrs(sbi, cpc, true, false);

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

[Chunhai Guo]

在 1/3/2025 11:36 AM, Chao Yu 写道:
> On 2025/1/2 18:13, Chunhai Guo wrote:
>> If userspace issues an fstrim with a range that does not include all
>> segments with small discards, these segments will be reused without being
> I didn't get it, if fstrim didn't cover those segments, why do we need to
> issue small discard for out-of-range segments?
Currently, all the dirty sentries in the dirty_sentries_bitmap are 
handled in the fstrim process regardless of whether they are within the 
fstrim range or not. Therefore, this patch is necessary to address the 
issue.

f2fs_flush_sit_entries()
     list_for_each_entry_safe(ses, tmp, head, set_list) {
         for_each_set_bit_from(segno, bitmap, end) {
             ...
             __clear_bit(segno, bitmap); // segno is cleared regardless 
of whether or not it is within the fstrim range
             ...
         }
     }


Thanks,

> Thanks,
>
>> discarded. This patch fixes this issue.
>> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
>> discard prefree segments").
>>
>> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>> ---
>>    fs/f2fs/segment.c | 10 +++++++---
>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 8fe9f794b581..af9a62591c49 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>    	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>>    	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>>    	struct seg_entry *se;
>> +	bool force = (cpc->reason & CP_DISCARD);
>> +	__u64 trim_start = cpc->trim_start;
>>    
>>    	down_write(&sit_i->sentry_lock);
>>    
>> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>    #endif
>>    
>>    			/* add discard candidates */
>> -			if (!(cpc->reason & CP_DISCARD)) {
>> +			if (!force || (force &&
>> +					(segno < trim_start ||
>> +					 segno > cpc->trim_end))) {
>>    				cpc->trim_start = segno;
>>    				add_discard_addrs(sbi, cpc, false, false);
>>    			}
>> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>    	f2fs_bug_on(sbi, !list_empty(head));
>>    	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>>    out:
>> -	if (cpc->reason & CP_DISCARD) {
>> -		__u64 trim_start = cpc->trim_start;
>> +	if (force) {
>> +		cpc->trim_start = trim_start;
>>    
>>    		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>>    			add_discard_addrs(sbi, cpc, true, false);

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

[Chunhai Guo]

在 1/3/2025 3:00 PM, Chunhai Guo 写道:
> 在 1/3/2025 11:36 AM, Chao Yu 写道:
>> On 2025/1/2 18:13, Chunhai Guo wrote:
>>> If userspace issues an fstrim with a range that does not include all
>>> segments with small discards, these segments will be reused without being
>> I didn't get it, if fstrim didn't cover those segments, why do we need to
>> issue small discard for out-of-range segments?
> Currently, all the dirty sentries in the dirty_sentries_bitmap are
> handled in the fstrim process regardless of whether they are within the
> fstrim range or not. Therefore, this patch is necessary to address the
> issue.
>
> f2fs_flush_sit_entries()
>       list_for_each_entry_safe(ses, tmp, head, set_list) {
>           for_each_set_bit_from(segno, bitmap, end) {
>               ...
>               __clear_bit(segno, bitmap); // segno is cleared regardless
> of whether or not it is within the fstrim range
>               ...
>           }
>       }


Hi Chao,


Could you please provide some advice on this issue when you have a moment?
Or do you have any questions about my explanation above?

Thanks,

>
> Thanks,
>
>> Thanks,
>>
>>> discarded. This patch fixes this issue.
>>> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
>>> discard prefree segments").
>>>
>>> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
>>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>>> ---
>>>     fs/f2fs/segment.c | 10 +++++++---
>>>     1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 8fe9f794b581..af9a62591c49 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>     	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>>>     	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>>>     	struct seg_entry *se;
>>> +	bool force = (cpc->reason & CP_DISCARD);
>>> +	__u64 trim_start = cpc->trim_start;
>>>     
>>>     	down_write(&sit_i->sentry_lock);
>>>     
>>> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>     #endif
>>>     
>>>     			/* add discard candidates */
>>> -			if (!(cpc->reason & CP_DISCARD)) {
>>> +			if (!force || (force &&
>>> +					(segno < trim_start ||
>>> +					 segno > cpc->trim_end))) {
>>>     				cpc->trim_start = segno;
>>>     				add_discard_addrs(sbi, cpc, false, false);
>>>     			}
>>> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>     	f2fs_bug_on(sbi, !list_empty(head));
>>>     	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>>>     out:
>>> -	if (cpc->reason & CP_DISCARD) {
>>> -		__u64 trim_start = cpc->trim_start;
>>> +	if (force) {
>>> +		cpc->trim_start = trim_start;
>>>     
>>>     		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>>>     			add_discard_addrs(sbi, cpc, true, false);
>

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

[Chao Yu]

On 1/3/25 15:00, Chunhai Guo wrote:
> 在 1/3/2025 11:36 AM, Chao Yu 写道:
>> On 2025/1/2 18:13, Chunhai Guo wrote:
>>> If userspace issues an fstrim with a range that does not include all
>>> segments with small discards, these segments will be reused without being
>> I didn't get it, if fstrim didn't cover those segments, why do we need to
>> issue small discard for out-of-range segments?
> Currently, all the dirty sentries in the dirty_sentries_bitmap are 
> handled in the fstrim process regardless of whether they are within the 
> fstrim range or not. Therefore, this patch is necessary to address the 
> issue.

fstrim flow doesn't depend on dirty status of segments, right?

It will add discard range w/ below code?

	if (cpc->reason & CP_DISCARD) {
		__u64 trim_start = cpc->trim_start;

		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
			add_discard_addrs(sbi, cpc, false);

		cpc->trim_start = trim_start;
	}

Thanks,

> 
> f2fs_flush_sit_entries()
>      list_for_each_entry_safe(ses, tmp, head, set_list) {
>          for_each_set_bit_from(segno, bitmap, end) {
>              ...
>              __clear_bit(segno, bitmap); // segno is cleared regardless 
> of whether or not it is within the fstrim range
>              ...
>          }
>      }
> 
> 
> Thanks,
> 
>> Thanks,
>>
>>> discarded. This patch fixes this issue.
>>> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
>>> discard prefree segments").
>>>
>>> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
>>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>>> ---
>>>    fs/f2fs/segment.c | 10 +++++++---
>>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 8fe9f794b581..af9a62591c49 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>    	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>>>    	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>>>    	struct seg_entry *se;
>>> +	bool force = (cpc->reason & CP_DISCARD);
>>> +	__u64 trim_start = cpc->trim_start;
>>>    
>>>    	down_write(&sit_i->sentry_lock);
>>>    
>>> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>    #endif
>>>    
>>>    			/* add discard candidates */
>>> -			if (!(cpc->reason & CP_DISCARD)) {
>>> +			if (!force || (force &&
>>> +					(segno < trim_start ||
>>> +					 segno > cpc->trim_end))) {
>>>    				cpc->trim_start = segno;
>>>    				add_discard_addrs(sbi, cpc, false, false);
>>>    			}
>>> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>>    	f2fs_bug_on(sbi, !list_empty(head));
>>>    	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>>>    out:
>>> -	if (cpc->reason & CP_DISCARD) {
>>> -		__u64 trim_start = cpc->trim_start;
>>> +	if (force) {
>>> +		cpc->trim_start = trim_start;
>>>    
>>>    		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>>>    			add_discard_addrs(sbi, cpc, true, false);
> 
> 

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

[Chao Yu]

On 1/2/25 18:13, Chunhai Guo wrote:
> If userspace issues an fstrim with a range that does not include all
> segments with small discards, these segments will be reused without being
> discarded. This patch fixes this issue.
> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
> discard prefree segments").

I guess it's better to update commit message as we discussed?

> 
> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
> ---
>  fs/f2fs/segment.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 8fe9f794b581..af9a62591c49 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>  	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>  	struct seg_entry *se;
> +	bool force = (cpc->reason & CP_DISCARD);
> +	__u64 trim_start = cpc->trim_start;
>  
>  	down_write(&sit_i->sentry_lock);
>  
> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  #endif
>  
>  			/* add discard candidates */
> -			if (!(cpc->reason & CP_DISCARD)) {
> +			if (!force || (force &&

if (!force || (f2fs_realtime_discard_enable() &&
		(segno < trim_start || segno > trim_end)))

Thanks,

> +					(segno < trim_start ||
> +					 segno > cpc->trim_end))) {
>  				cpc->trim_start = segno;
>  				add_discard_addrs(sbi, cpc, false, false);
>  			}
> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>  	f2fs_bug_on(sbi, !list_empty(head));
>  	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>  out:
> -	if (cpc->reason & CP_DISCARD) {
> -		__u64 trim_start = cpc->trim_start;
> +	if (force) {
> +		cpc->trim_start = trim_start;
>  
>  		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>  			add_discard_addrs(sbi, cpc, true, false);

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

[Chunhai Guo]

在 5/22/2025 7:11 PM, Chao Yu 写道:
> On 1/2/25 18:13, Chunhai Guo wrote:
>> If userspace issues an fstrim with a range that does not include all
>> segments with small discards, these segments will be reused without being
>> discarded. This patch fixes this issue.
>> This patch is somewhat similar to commit 650d3c4e56e1 ("f2fs: fix a missing
>> discard prefree segments").
> I guess it's better to update commit message as we discussed?

OK. I will update this message.

>
>> Fixes: d7bc2484b8d4 ("f2fs: fix small discards not to issue redundantly")
>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>> ---
>>   fs/f2fs/segment.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 8fe9f794b581..af9a62591c49 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -4552,6 +4552,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>   	struct list_head *head = &SM_I(sbi)->sit_entry_set;
>>   	bool to_journal = !is_sbi_flag_set(sbi, SBI_IS_RESIZEFS);
>>   	struct seg_entry *se;
>> +	bool force = (cpc->reason & CP_DISCARD);
>> +	__u64 trim_start = cpc->trim_start;
>>   
>>   	down_write(&sit_i->sentry_lock);
>>   
>> @@ -4609,7 +4611,9 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>   #endif
>>   
>>   			/* add discard candidates */
>> -			if (!(cpc->reason & CP_DISCARD)) {
>> +			if (!force || (force &&
> if (!force || (f2fs_realtime_discard_enable() &&
> 		(segno < trim_start || segno > trim_end)))


Thank you for pointing this out. It indeed lacks the handling of 
realtime discard. I will submit a V2 patch to address it.

Thanks,

>
> Thanks,
>
>> +					(segno < trim_start ||
>> +					 segno > cpc->trim_end))) {
>>   				cpc->trim_start = segno;
>>   				add_discard_addrs(sbi, cpc, false, false);
>>   			}
>> @@ -4649,8 +4653,8 @@ void f2fs_flush_sit_entries(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>   	f2fs_bug_on(sbi, !list_empty(head));
>>   	f2fs_bug_on(sbi, sit_i->dirty_sentries);
>>   out:
>> -	if (cpc->reason & CP_DISCARD) {
>> -		__u64 trim_start = cpc->trim_start;
>> +	if (force) {
>> +		cpc->trim_start = trim_start;
>>   
>>   		for (; cpc->trim_start <= cpc->trim_end; cpc->trim_start++)
>>   			add_discard_addrs(sbi, cpc, true, false);

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

[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")

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

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

[Chunhai Guo]

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

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

[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?

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

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

[Chunhai Guo]

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

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

[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];

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

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

[Chunhai Guo]

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