[PATCH] f2fs: skip further SIT updates for already invalidated blocks

[PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Seongjae Jeong]

When update_sit_entry_for_release() encounters a block whose bit is
already cleared in cur_valid_map, it reports the inconsistency and
corrects se->valid_blocks and del. However, it still proceeds with
the discard bitmap and checkpoint accounting updates.

This can clear the discard bit unnecessarily and incorrectly decrement
the segment and section ckpt_valid_blocks counters when the block is
not marked in ckpt_valid_map.

Continue to the next block after correcting se->valid_blocks and del,
so that an already invalidated block does not trigger further discard
bitmap or checkpoint counter updates.

Fixes: 81ffbd224e5f ("f2fs: update_sit_entry_for_release() supports consecutive blocks.")
Signed-off-by: Seongjae Jeong <jsjlee1020@gmail.com>
---
 fs/f2fs/segment.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 51858f64e43e..fc2f0fd8e318 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2560,7 +2560,10 @@ static int update_sit_entry_for_release(struct f2fs_sb_info *sbi, struct seg_ent
 			f2fs_bug_on(sbi, 1);
 			se->valid_blocks++;
 			del += 1;
-		} else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
+			continue;
+		}
+
+		if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
 			/*
 			 * If checkpoints are off, we must not reuse data that
 			 * was used in the previous checkpoint. If it was used
-- 
2.53.0

Re: [PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Chao Yu]

On 10/4/26 14:10, Seongjae Jeong wrote:
> When update_sit_entry_for_release() encounters a block whose bit is
> already cleared in cur_valid_map, it reports the inconsistency and
> corrects se->valid_blocks and del. However, it still proceeds with
> the discard bitmap and checkpoint accounting updates.
> 
> This can clear the discard bit unnecessarily and incorrectly decrement
> the segment and section ckpt_valid_blocks counters when the block is
> not marked in ckpt_valid_map.
> 
> Continue to the next block after correcting se->valid_blocks and del,
> so that an already invalidated block does not trigger further discard
> bitmap or checkpoint counter updates.
> 

Cc stable.

> Fixes: 81ffbd224e5f ("f2fs: update_sit_entry_for_release() supports consecutive blocks.")

This commit didn't change previous logic? IIUC.

Otherwise it looks good to me.

Thanks,

> Signed-off-by: Seongjae Jeong <jsjlee1020@gmail.com>
> ---
>   fs/f2fs/segment.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 51858f64e43e..fc2f0fd8e318 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2560,7 +2560,10 @@ static int update_sit_entry_for_release(struct f2fs_sb_info *sbi, struct seg_ent
>   			f2fs_bug_on(sbi, 1);
>   			se->valid_blocks++;
>   			del += 1;
> -		} else if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
> +			continue;
> +		}
> +
> +		if (unlikely(is_sbi_flag_set(sbi, SBI_CP_DISABLED))) {
>   			/*
>   			 * If checkpoints are off, we must not reuse data that
>   			 * was used in the previous checkpoint. If it was used

Re: [PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Seongjae Jeong]

Hi Chao,

> Cc stable.
>
> > Fixes: 81ffbd224e5f ("f2fs: update_sit_entry_for_release() supports consecutive blocks.")
>
> This commit didn't change previous logic? IIUC.

The discard bitmap handling already existed before this commit. It was
introduced
by a66cdd985532 ("f2fs: introduce discard_map for f2fs_trim_fs").

However, 81ffbd224e5f did change the checkpoint accounting.
Previously, del was set to 0 when the cur_valid_map bit was already cleared,
so se->ckpt_valid_blocks += del left the counter unchanged.
That commit replaced this logic with se->ckpt_valid_blocks -= 1,
regardless of the del value.
Thus, the counter is incorrectly decremented.

Would it make sense to add a second Fixes tag for a66cdd985532 to cover
the discard bitmap handling?

Re: [PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Chao Yu]

On 10/11/26 18:39, Seongjae Jeong wrote:
> Hi Chao,
> 
>> Cc stable.
>>
>>> Fixes: 81ffbd224e5f ("f2fs: update_sit_entry_for_release() supports consecutive blocks.")

Hi Seongjae,

Oh, sorry, it seems original Fixes line is correct, my bad.

81ffbd224e5f ("f2fs: update_sit_entry_for_release() supports consecutive blocks."):

-       if (unlikely(!exist)) {
-               f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u", blkaddr);
-               f2fs_bug_on(sbi, 1);
-               se->valid_blocks++;
-               del = 0;


+               if (unlikely(!exist)) {
+                       f2fs_err(sbi, "Bitmap was wrongly cleared, blk:%u", blkaddr + i);
+                       f2fs_bug_on(sbi, 1);
+                       se->valid_blocks++;
+                       del += 1;

The patch changed from "del = 0" to "del += 1" incorrectly.

-       if (!f2fs_test_bit(offset, se->ckpt_valid_map))
-               se->ckpt_valid_blocks += del;
+               if (!f2fs_test_bit(offset + i, se->ckpt_valid_map))
+                       se->ckpt_valid_blocks -= 1;
+       }

And from "se->ckpt_valid_blocks += del" to "se->ckpt_valid_blocks -= 1".

Let me know if I missed anything. :)

Thanks,

>>
>> This commit didn't change previous logic? IIUC.
> 
> The discard bitmap handling already existed before this commit. It was
> introduced
> by a66cdd985532 ("f2fs: introduce discard_map for f2fs_trim_fs").
> 
> However, 81ffbd224e5f did change the checkpoint accounting.
> Previously, del was set to 0 when the cur_valid_map bit was already cleared,
> so se->ckpt_valid_blocks += del left the counter unchanged.
> That commit replaced this logic with se->ckpt_valid_blocks -= 1,
> regardless of the del value.
> Thus, the counter is incorrectly decremented.
> 
> Would it make sense to add a second Fixes tag for a66cdd985532 to cover
> the discard bitmap handling?

Re: [PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Seongjae Jeong]

Hi Chao,

Thanks for checking.
I agree that 81ffbd224e5f is the right Fixes tag.

I’ve sent v2 with stable on Cc and an additional Fixes tag for
a66cdd985532 to cover the discard bitmap handling. Does the second
Fixes tag look appropriate to you?

Thanks,
Seongjae

Re: [PATCH] f2fs: skip further SIT updates for already invalidated blocks

[Chao Yu]

On 11/12/26 11:48, Seongjae Jeong wrote:
> Hi Chao,
> 
> Thanks for checking.
> I agree that 81ffbd224e5f is the right Fixes tag.
> 
> I’ve sent v2 with stable on Cc and an additional Fixes tag for
> a66cdd985532 to cover the discard bitmap handling. Does the second
> Fixes tag look appropriate to you?

Hi SeongJae,

I don't see how commit a66cdd985532 ("f2fs: introduce discard_map for f2fs_trim_fs")
introduce the bug? from the timeline, at that point there is no error handling in
update_sit_entries() if sit bitmap is inconsistent, right?

Thanks,

> 
> Thanks,
> Seongjae