Re: [PATCH] f2fs: compress: fix inconsistent update of i_blocks in release_compress_blocks and reserve_compress_blocks

[Chao Yu <chao@kernel.org>]

On 2024/9/29 10:43, Qi Han wrote:
> After release a file and subsequently reserve it, the FSCK flag is set
> when the file is deleted, as shown in the following backtrace:
> 
> F2FS-fs (dm-48): Inconsistent i_blocks, ino:401231, iblocks:1448, sectors:1472
> fs_rec_info_write_type+0x58/0x274
> f2fs_rec_info_write+0x1c/0x2c
> set_sbi_flag+0x74/0x98
> dec_valid_block_count+0x150/0x190
> f2fs_truncate_data_blocks_range+0x2d4/0x3cc
> f2fs_do_truncate_blocks+0x2fc/0x5f0
> f2fs_truncate_blocks+0x68/0x100
> f2fs_truncate+0x80/0x128
> f2fs_evict_inode+0x1a4/0x794
> evict+0xd4/0x280
> iput+0x238/0x284
> do_unlinkat+0x1ac/0x298
> __arm64_sys_unlinkat+0x48/0x68
> invoke_syscall+0x58/0x11c
> 
> For clusters of the following type, i_blocks are decremented by 1 and
> i_compr_blocks are incremented by 7 in release_compress_blocks, while
> updates to i_blocks and i_compr_blocks are skipped in reserve_compress_blocks.
> 
> raw node:
> D D D D D D D D
> after compress:
> C D D D D D D D
> after reserve:
> C D D D D D D D
> 
> Let's update i_blocks and i_compr_blocks properly in reserve_compress_blocks.

Hi, Qi,

Thank you for catching this.

> 
> Fixes: eb8fbaa53374 ("f2fs: compress: fix to check unreleased compressed cluster")
> Signed-off-by: Qi Han <hanqi@vivo.com>
> ---
>   fs/f2fs/file.c | 6 ------
>   1 file changed, 6 deletions(-)
> 
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 9ae54c4c72fe..7500b4067996 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -3791,12 +3791,6 @@ static int reserve_compress_blocks(struct dnode_of_data *dn, pgoff_t count,
>   
>   		to_reserved = cluster_size - compr_blocks - reserved;
>   
> -		/* for the case all blocks in cluster were reserved */
> -		if (to_reserved == 1) {

I think this case is trying to catch abnormal condition and try to
handle it correctly, e.g. compressed cluster was not released due
to it fails in release_compress_blocks(), so status of compress
cluster may be: C D N N N N N N.

So the right check condition should be?

if (reserved && to_reserved == 1)

Thoughts?

And I think we'd better add a testcase in fstests to cover all these
cases, let me figure out a patch soon.

Thanks,

> -			dn->ofs_in_node += cluster_size;
> -			goto next;
> -		}
> -
>   		ret = inc_valid_block_count(sbi, dn->inode,
>   						&to_reserved, false);
>   		if (unlikely(ret))