Re: [PATCH v3 3/3] ext4: add check pending tree when evict inode

[Eric Whitney]

`* Ye Bin <yebin@xxxxxxxxxxxxxxx>:
> From: Ye Bin <yebin10@xxxxxxxxxx>
> 
> Syzbot found the following issue:
> BUG: memory leak
> unreferenced object 0xffff8881bde17420 (size 32):
>   comm "rep", pid 2327, jiffies 4295381963 (age 32.265s)
>   hex dump (first 32 bytes):
>     01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>     00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  ................
>   backtrace:
>     [<00000000ac6d38f8>] __insert_pending+0x13c/0x2d0
>     [<00000000d717de3b>] ext4_es_insert_delayed_block+0x399/0x4e0
>     [<000000004be03913>] ext4_da_map_blocks.constprop.0+0x739/0xfa0
>     [<00000000885a832a>] ext4_da_get_block_prep+0x10c/0x440
>     [<0000000029b7f8ef>] __block_write_begin_int+0x28d/0x860
>     [<00000000e182ebc3>] ext4_da_write_inline_data_begin+0x2d1/0xf30
>     [<00000000ced0c8a2>] ext4_da_write_begin+0x612/0x860
>     [<000000008d5f27fa>] generic_perform_write+0x215/0x4d0
>     [<00000000552c1cde>] ext4_buffered_write_iter+0x101/0x3b0
>     [<0000000052177ae8>] do_iter_readv_writev+0x19f/0x340
>     [<000000004b9de834>] do_iter_write+0x13b/0x650
>     [<00000000e2401b9b>] iter_file_splice_write+0x5a5/0xab0
>     [<0000000023aa5d90>] direct_splice_actor+0x103/0x1e0
>     [<0000000089e00fc1>] splice_direct_to_actor+0x2c9/0x7b0
>     [<000000004386851e>] do_splice_direct+0x159/0x280
>     [<00000000b567e609>] do_sendfile+0x932/0x1200
> 
> Above issue fixed by 1b8f787ef547 "ext4: fix warning in 'ext4_da_release_space'"
> in this scene. To make things better add check pending tree when evit inode.
> According to Eric Whitney's suggestion, bigalloc + inline is still in development
> so we just add test for this situation, there isn't need to add code to free
> pending tree entry.
> 
> Reported-by: syzbot+05a0f0ccab4a25626e38@xxxxxxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Ye Bin <yebin10@xxxxxxxxxx>
> ---
>  fs/ext4/super.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 41413338c05b..2e2fbc4a832c 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1391,6 +1391,11 @@ static void ext4_destroy_inode(struct inode *inode)
>  			"Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
>  			inode->i_ino, EXT4_I(inode),
>  			EXT4_I(inode)->i_reserved_data_blocks);
> +
> +	if (!RB_EMPTY_ROOT(&EXT4_I(inode)->i_pending_tree.root))
> +		ext4_error(inode->i_sb,
> +			"Inode %lu (%p): i_pending_tree not empty!",
> +			inode->i_ino, EXT4_I(inode));
>  }
>

It's always a good idea to run ./scripts/checkpatch.pl on your patches before
submitting them.  It's complaining that the lines in your commit description
are too long (it wants a maximum of 75 characters per line, but Ted prefers
a maximum of 72 for ext4 patches, IIRC).  Also, it wants parentheses around
the title of the patch mentioned in the commit message:
ie: commit 1b8f787ef547 ("ext4: fix warning in 'ext4_da_release_space'")

Also, typical ext4 practice is to align arguments to a function on following
lines to the beginning of the first argument, as can be seen earlier in
ext4_destroy_inode.  Indenting as you've done here passes checkpatch, but
it's different from most ext4 code (which also passes checkpatch).

Otherwise, it looks okay.

Eric


>  static void init_once(void *foo)
> -- 
> 2.31.1
>