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