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?