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

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

[Chunhai Guo]

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

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

Re: [PATCH v2] f2fs: fix missing discard candidates in fstrim

[Chao Yu]

On 1/19/25 22:08, Chunhai Guo wrote:
> fstrim may miss candidates that need to be discarded, as shown in the
> examples below.
> 
> The root cause is that when cpc->reason is set with CP_DISCARD,
> add_discard_addrs() expects that ckpt_valid_map and cur_valid_map have
> been synced by seg_info_to_raw_sit() [1], and it tries to find the
> candidates based on ckpt_valid_map and discard_map. However,
> seg_info_to_raw_sit() does not actually run before
> f2fs_exist_trim_candidates(), resulting in the failure.
> 
> The code logic can be simplified for all cases by finding all the
> discard blocks based only on discard_map. This might result in more
> discard blocks being sent for the segment during the first checkpoint
> after mounting, which were originally expected to be sent only in
> fstrim. Regardless, these discard blocks should eventually be sent, and
> the simplified code makes sense in this context.
> 
> root# cp testfile /f2fs_mountpoint
> 
> root# f2fs_io fiemap 0 1 /f2fs_mountpoint/testfile
> Fiemap: offset = 0 len = 1
>          logical addr.    physical addr.   length           flags
> 0       0000000000000000 0000000406a00000 000000003d800000 00001000
> 
> root# rm /f2fs_mountpoint/testfile
> 
> root# fstrim -v -o 0x406a00000 -l 1024M /f2fs_mountpoint -- no candidate is found
> /f2fs_mountpoint: 0 B (0 bytes) trimmed
> 
> Relevant code process of the root cause:
> f2fs_trim_fs()
>      f2fs_write_checkpoint()
>          ...
>          if (cpc->reason & CP_DISCARD) {
>                  if (!f2fs_exist_trim_candidates(sbi, cpc)) {
>                      unblock_operations(sbi);
>                      goto out; // No candidates are found here, and it exits.
>                  }
>              ...
>          }
> 
> [1] Please refer to commit d7bc2484b8d4 ("f2fs: fix small discards not
> to issue redundantly") for the relationship between
> seg_info_to_raw_sit() and add_discard_addrs().
> 
> Fixes: 25290fa5591d ("f2fs: return fs_trim if there is no candidate")
> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
> ---
> v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/
> v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs().
> ---
>   fs/f2fs/segment.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 13ee73a3c481..25ea892a42dd 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2074,8 +2074,6 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>   {
>   	int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
>   	struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
> -	unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
> -	unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
>   	unsigned long *discard_map = (unsigned long *)se->discard_map;
>   	unsigned long *dmap = SIT_I(sbi)->tmp_map;
>   	unsigned int start = 0, end = -1;
> @@ -2100,8 +2098,7 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>   
>   	/* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>   	for (i = 0; i < entries; i++)
> -		dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
> -				(cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
> +		dmap[i] = ~discard_map[i];

discard is critical, we need more sanity check here, maybe:

/* never issue discard to valid data's block address */
f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]);

Can you please check this?

Thanks,

>   
>   	while (force || SM_I(sbi)->dcc_info->nr_discards <=
>   				SM_I(sbi)->dcc_info->max_discards) {

Re: [PATCH v2] f2fs: fix missing discard candidates in fstrim

[Chunhai Guo]

在 1/20/2025 7:45 PM, Chao Yu 写道:
> On 1/19/25 22:08, Chunhai Guo wrote:
>> fstrim may miss candidates that need to be discarded, as shown in the
>> examples below.
>>
>> The root cause is that when cpc->reason is set with CP_DISCARD,
>> add_discard_addrs() expects that ckpt_valid_map and cur_valid_map have
>> been synced by seg_info_to_raw_sit() [1], and it tries to find the
>> candidates based on ckpt_valid_map and discard_map. However,
>> seg_info_to_raw_sit() does not actually run before
>> f2fs_exist_trim_candidates(), resulting in the failure.
>>
>> The code logic can be simplified for all cases by finding all the
>> discard blocks based only on discard_map. This might result in more
>> discard blocks being sent for the segment during the first checkpoint
>> after mounting, which were originally expected to be sent only in
>> fstrim. Regardless, these discard blocks should eventually be sent, and
>> the simplified code makes sense in this context.
>>
>> root# cp testfile /f2fs_mountpoint
>>
>> root# f2fs_io fiemap 0 1 /f2fs_mountpoint/testfile
>> Fiemap: offset = 0 len = 1
>>           logical addr.    physical addr.   length           flags
>> 0       0000000000000000 0000000406a00000 000000003d800000 00001000
>>
>> root# rm /f2fs_mountpoint/testfile
>>
>> root# fstrim -v -o 0x406a00000 -l 1024M /f2fs_mountpoint -- no candidate is found
>> /f2fs_mountpoint: 0 B (0 bytes) trimmed
>>
>> Relevant code process of the root cause:
>> f2fs_trim_fs()
>>       f2fs_write_checkpoint()
>>           ...
>>           if (cpc->reason & CP_DISCARD) {
>>                   if (!f2fs_exist_trim_candidates(sbi, cpc)) {
>>                       unblock_operations(sbi);
>>                       goto out; // No candidates are found here, and it exits.
>>                   }
>>               ...
>>           }
>>
>> [1] Please refer to commit d7bc2484b8d4 ("f2fs: fix small discards not
>> to issue redundantly") for the relationship between
>> seg_info_to_raw_sit() and add_discard_addrs().
>>
>> Fixes: 25290fa5591d ("f2fs: return fs_trim if there is no candidate")
>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>> ---
>> v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/
>> v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs().
>> ---
>>    fs/f2fs/segment.c | 5 +----
>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>> index 13ee73a3c481..25ea892a42dd 100644
>> --- a/fs/f2fs/segment.c
>> +++ b/fs/f2fs/segment.c
>> @@ -2074,8 +2074,6 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>    {
>>      int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
>>      struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
>> -    unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
>> -    unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
>>      unsigned long *discard_map = (unsigned long *)se->discard_map;
>>      unsigned long *dmap = SIT_I(sbi)->tmp_map;
>>      unsigned int start = 0, end = -1;
>> @@ -2100,8 +2098,7 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>
>>      /* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>>      for (i = 0; i < entries; i++)
>> -            dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
>> -                            (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
>> +            dmap[i] = ~discard_map[i];
> discard is critical, we need more sanity check here, maybe:
>
> /* never issue discard to valid data's block address */
> f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]);
>
> Can you please check this?

Sure. I have added the BUG_ON check and performed the following tests
without issue:
1. Ran xfstests and fsstress in the QEMU environment.

2. Ran monkey and reboot tests on ARM64 Android devices with the 6.6 kernel.

Thanks,


>
> Thanks,
>
>>
>>      while (force || SM_I(sbi)->dcc_info->nr_discards <=
>>                              SM_I(sbi)->dcc_info->max_discards) {

Re: [PATCH v2] f2fs: fix missing discard candidates in fstrim

[Chao Yu]

On 3/12/25 11:19, Chunhai Guo wrote:
> 在 1/20/2025 7:45 PM, Chao Yu 写道:
>> On 1/19/25 22:08, Chunhai Guo wrote:
>>> fstrim may miss candidates that need to be discarded, as shown in the
>>> examples below.
>>>
>>> The root cause is that when cpc->reason is set with CP_DISCARD,
>>> add_discard_addrs() expects that ckpt_valid_map and cur_valid_map have
>>> been synced by seg_info_to_raw_sit() [1], and it tries to find the
>>> candidates based on ckpt_valid_map and discard_map. However,
>>> seg_info_to_raw_sit() does not actually run before
>>> f2fs_exist_trim_candidates(), resulting in the failure.
>>>
>>> The code logic can be simplified for all cases by finding all the
>>> discard blocks based only on discard_map. This might result in more
>>> discard blocks being sent for the segment during the first checkpoint
>>> after mounting, which were originally expected to be sent only in
>>> fstrim. Regardless, these discard blocks should eventually be sent, and
>>> the simplified code makes sense in this context.
>>>
>>> root# cp testfile /f2fs_mountpoint
>>>
>>> root# f2fs_io fiemap 0 1 /f2fs_mountpoint/testfile
>>> Fiemap: offset = 0 len = 1
>>>           logical addr.    physical addr.   length           flags
>>> 0       0000000000000000 0000000406a00000 000000003d800000 00001000
>>>
>>> root# rm /f2fs_mountpoint/testfile
>>>
>>> root# fstrim -v -o 0x406a00000 -l 1024M /f2fs_mountpoint -- no candidate is found
>>> /f2fs_mountpoint: 0 B (0 bytes) trimmed
>>>
>>> Relevant code process of the root cause:
>>> f2fs_trim_fs()
>>>       f2fs_write_checkpoint()
>>>           ...
>>>           if (cpc->reason & CP_DISCARD) {
>>>                   if (!f2fs_exist_trim_candidates(sbi, cpc)) {
>>>                       unblock_operations(sbi);
>>>                       goto out; // No candidates are found here, and it exits.
>>>                   }
>>>               ...
>>>           }
>>>
>>> [1] Please refer to commit d7bc2484b8d4 ("f2fs: fix small discards not
>>> to issue redundantly") for the relationship between
>>> seg_info_to_raw_sit() and add_discard_addrs().
>>>
>>> Fixes: 25290fa5591d ("f2fs: return fs_trim if there is no candidate")
>>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>>> ---
>>> v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/
>>> v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs().
>>> ---
>>>    fs/f2fs/segment.c | 5 +----
>>>    1 file changed, 1 insertion(+), 4 deletions(-)
>>>
>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>> index 13ee73a3c481..25ea892a42dd 100644
>>> --- a/fs/f2fs/segment.c
>>> +++ b/fs/f2fs/segment.c
>>> @@ -2074,8 +2074,6 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>    {
>>>      int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
>>>      struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
>>> -    unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
>>> -    unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
>>>      unsigned long *discard_map = (unsigned long *)se->discard_map;
>>>      unsigned long *dmap = SIT_I(sbi)->tmp_map;
>>>      unsigned int start = 0, end = -1;
>>> @@ -2100,8 +2098,7 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>
>>>      /* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>>>      for (i = 0; i < entries; i++)
>>> -            dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
>>> -                            (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
>>> +            dmap[i] = ~discard_map[i];
>> discard is critical, we need more sanity check here, maybe:
>>
>> /* never issue discard to valid data's block address */
>> f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]);
>>
>> Can you please check this?
> 
> Sure. I have added the BUG_ON check and performed the following tests
> without issue:
> 1. Ran xfstests and fsstress in the QEMU environment.
> 
> 2. Ran monkey and reboot tests on ARM64 Android devices with the 6.6 kernel.

Thanks, so it looks fine now, can you please update the patch w/ above
f2fs_bug_on check?

Thanks,

> 
> Thanks,
> 
> 
>>
>> Thanks,
>>
>>>
>>>      while (force || SM_I(sbi)->dcc_info->nr_discards <=
>>>                              SM_I(sbi)->dcc_info->max_discards) {
> 
> 

Re: [PATCH v2] f2fs: fix missing discard candidates in fstrim

[Chunhai Guo]

在 3/12/2025 12:03 PM, Chao Yu 写道:
>
> On 3/12/25 11:19, Chunhai Guo wrote:
>> 在 1/20/2025 7:45 PM, Chao Yu 写道:
>>> On 1/19/25 22:08, Chunhai Guo wrote:
>>>> fstrim may miss candidates that need to be discarded, as shown in the
>>>> examples below.
>>>>
>>>> The root cause is that when cpc->reason is set with CP_DISCARD,
>>>> add_discard_addrs() expects that ckpt_valid_map and cur_valid_map have
>>>> been synced by seg_info_to_raw_sit() [1], and it tries to find the
>>>> candidates based on ckpt_valid_map and discard_map. However,
>>>> seg_info_to_raw_sit() does not actually run before
>>>> f2fs_exist_trim_candidates(), resulting in the failure.
>>>>
>>>> The code logic can be simplified for all cases by finding all the
>>>> discard blocks based only on discard_map. This might result in more
>>>> discard blocks being sent for the segment during the first checkpoint
>>>> after mounting, which were originally expected to be sent only in
>>>> fstrim. Regardless, these discard blocks should eventually be sent, and
>>>> the simplified code makes sense in this context.
>>>>
>>>> root# cp testfile /f2fs_mountpoint
>>>>
>>>> root# f2fs_io fiemap 0 1 /f2fs_mountpoint/testfile
>>>> Fiemap: offset = 0 len = 1
>>>>            logical addr.    physical addr.   length           flags
>>>> 0       0000000000000000 0000000406a00000 000000003d800000 00001000
>>>>
>>>> root# rm /f2fs_mountpoint/testfile
>>>>
>>>> root# fstrim -v -o 0x406a00000 -l 1024M /f2fs_mountpoint -- no candidate is found
>>>> /f2fs_mountpoint: 0 B (0 bytes) trimmed
>>>>
>>>> Relevant code process of the root cause:
>>>> f2fs_trim_fs()
>>>>        f2fs_write_checkpoint()
>>>>            ...
>>>>            if (cpc->reason & CP_DISCARD) {
>>>>                    if (!f2fs_exist_trim_candidates(sbi, cpc)) {
>>>>                        unblock_operations(sbi);
>>>>                        goto out; // No candidates are found here, and it exits.
>>>>                    }
>>>>                ...
>>>>            }
>>>>
>>>> [1] Please refer to commit d7bc2484b8d4 ("f2fs: fix small discards not
>>>> to issue redundantly") for the relationship between
>>>> seg_info_to_raw_sit() and add_discard_addrs().
>>>>
>>>> Fixes: 25290fa5591d ("f2fs: return fs_trim if there is no candidate")
>>>> Signed-off-by: Chunhai Guo <guochunhai@vivo.com>
>>>> ---
>>>> v1: https://lore.kernel.org/linux-f2fs-devel/20250102101310.580277-1-guochunhai@vivo.com/
>>>> v1->v2: Find all the discard blocks based only on discard_map in add_discard_addrs().
>>>> ---
>>>>     fs/f2fs/segment.c | 5 +----
>>>>     1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>>>> index 13ee73a3c481..25ea892a42dd 100644
>>>> --- a/fs/f2fs/segment.c
>>>> +++ b/fs/f2fs/segment.c
>>>> @@ -2074,8 +2074,6 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>>     {
>>>>       int entries = SIT_VBLOCK_MAP_SIZE / sizeof(unsigned long);
>>>>       struct seg_entry *se = get_seg_entry(sbi, cpc->trim_start);
>>>> -    unsigned long *cur_map = (unsigned long *)se->cur_valid_map;
>>>> -    unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
>>>>       unsigned long *discard_map = (unsigned long *)se->discard_map;
>>>>       unsigned long *dmap = SIT_I(sbi)->tmp_map;
>>>>       unsigned int start = 0, end = -1;
>>>> @@ -2100,8 +2098,7 @@ static bool add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc,
>>>>
>>>>       /* SIT_VBLOCK_MAP_SIZE should be multiple of sizeof(unsigned long) */
>>>>       for (i = 0; i < entries; i++)
>>>> -            dmap[i] = force ? ~ckpt_map[i] & ~discard_map[i] :
>>>> -                            (cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
>>>> +            dmap[i] = ~discard_map[i];
>>> discard is critical, we need more sanity check here, maybe:
>>>
>>> /* never issue discard to valid data's block address */
>>> f2fs_bug_on(sbi, (cur_map[i] ^ discard_map[i]) & cur_map[i]);
>>>
>>> Can you please check this?
>> Sure. I have added the BUG_ON check and performed the following tests
>> without issue:
>> 1. Ran xfstests and fsstress in the QEMU environment.
>>
>> 2. Ran monkey and reboot tests on ARM64 Android devices with the 6.6 kernel.
> Thanks, so it looks fine now, can you please update the patch w/ above
> f2fs_bug_on check?


OK. I will sent the v3 patch.

Thanks,


> Thanks,
>
>> Thanks,
>>
>>
>>> Thanks,
>>>
>>>>       while (force || SM_I(sbi)->dcc_info->nr_discards <=
>>>>                               SM_I(sbi)->dcc_info->max_discards) {
>>