Re: [PATCH] btrfs: free exchange changeset on failures

[Filipe Manana <fdmanana@kernel.org>]

On Fri, Dec 03, 2021 at 02:55:33AM -0800, Johannes Thumshirn wrote:
> Fstests runs on my VMs have show several kmemleak reports like the following.
> 
>   unreferenced object 0xffff88811ae59080 (size 64):
>     comm "xfs_io", pid 12124, jiffies 4294987392 (age 6.368s)
>     hex dump (first 32 bytes):
>       00 c0 1c 00 00 00 00 00 ff cf 1c 00 00 00 00 00  ................
>       90 97 e5 1a 81 88 ff ff 90 97 e5 1a 81 88 ff ff  ................
>     backtrace:
>       [<00000000ac0176d2>] ulist_add_merge+0x60/0x150 [btrfs]
>       [<0000000076e9f312>] set_state_bits+0x86/0xc0 [btrfs]
>       [<0000000014fe73d6>] set_extent_bit+0x270/0x690 [btrfs]
>       [<000000004f675208>] set_record_extent_bits+0x19/0x20 [btrfs]
>       [<00000000b96137b1>] qgroup_reserve_data+0x274/0x310 [btrfs]
>       [<0000000057e9dcbb>] btrfs_check_data_free_space+0x5c/0xa0 [btrfs]
>       [<0000000019c4511d>] btrfs_delalloc_reserve_space+0x1b/0xa0 [btrfs]
>       [<000000006d37e007>] btrfs_dio_iomap_begin+0x415/0x970 [btrfs]
>       [<00000000fb8a74b8>] iomap_iter+0x161/0x1e0
>       [<0000000071dff6ff>] __iomap_dio_rw+0x1df/0x700
>       [<000000002567ba53>] iomap_dio_rw+0x5/0x20
>       [<0000000072e555f8>] btrfs_file_write_iter+0x290/0x530 [btrfs]
>       [<000000005eb3d845>] new_sync_write+0x106/0x180
>       [<000000003fb505bf>] vfs_write+0x24d/0x2f0
>       [<000000009bb57d37>] __x64_sys_pwrite64+0x69/0xa0
>       [<000000003eba3fdf>] do_syscall_64+0x43/0x90
> 
> In case brtfs_qgroup_reserve_data() or btrfs_delalloc_reserve_metadata()
> fail the allocated extent_changeset will not be freed.
> 
> So in btrfs_check_data_free_space() and btrfs_delalloc_reserve_space()
> free the allocated extent_changeset to get rid of the allocated memory.
> 
> Cc: Filipe Manana <fdmanana@suse.com>
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>


Looks good, and it ran successfully on my fstests run with kmemleak as well.

Just a note worth adding, is that the issue currently only happens in the
direct IO write path, but only after my change "btrfs: fix ENOSPC failure
when attempting direct IO write into NOCOW range", and also at
defrag_one_locked_target() (haven't checked since when). Every other place
is always calling extent_changeset_free() even if its call to
btrfs_delalloc_reserve_space() or btrfs_check_data_free_space() has failed.

With that, which probably David can add, it looks good:

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> ---
>  fs/btrfs/delalloc-space.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/delalloc-space.c b/fs/btrfs/delalloc-space.c
> index bca438c7c972..fb46a28f5065 100644
> --- a/fs/btrfs/delalloc-space.c
> +++ b/fs/btrfs/delalloc-space.c
> @@ -143,10 +143,13 @@ int btrfs_check_data_free_space(struct btrfs_inode *inode,
>  
>  	/* Use new btrfs_qgroup_reserve_data to reserve precious data space. */
>  	ret = btrfs_qgroup_reserve_data(inode, reserved, start, len);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		btrfs_free_reserved_data_space_noquota(fs_info, len);
> -	else
> +		extent_changeset_free(*reserved);
> +		*reserved = NULL;
> +	} else {
>  		ret = 0;
> +	}
>  	return ret;
>  }
>  
> @@ -452,8 +455,11 @@ int btrfs_delalloc_reserve_space(struct btrfs_inode *inode,
>  	if (ret < 0)
>  		return ret;
>  	ret = btrfs_delalloc_reserve_metadata(inode, len);
> -	if (ret < 0)
> +	if (ret < 0) {
>  		btrfs_free_reserved_data_space(inode, *reserved, start, len);
> +		extent_changeset_free(*reserved);
> +		*reserved = NULL;
> +	}
>  	return ret;
>  }
>  
> -- 
> 2.31.1
>