Re: [PATCH] btrfs: fix extent map leak in NOCOW direct I/O write

在 2026/7/5 14:11, Shuangpeng Bai 写道:
> btrfs_dio_iomap_begin() calls btrfs_get_extent(), which returns an
> extent map reference that must be dropped on all exit paths.
> 
> For direct writes into a NOCOW range, btrfs_get_blocks_direct_write()
> keeps using that extent map and asks btrfs_create_dio_extent() to
> allocate the ordered extent. If that fails, for example because
> btrfs_alloc_ordered_extent() fails, the function returns the error
> without dropping the input extent map. The PREALLOC path avoided this by
> dropping the input extent map before replacing it with the newly
> created one.
> 
> Check the error from btrfs_create_dio_extent() before replacing the
> map and drop the input extent map on failure.
> 
> Fixes: 5f9a8a51d8b9 ("Btrfs: add semaphore to synchronize direct IO writes with fsync")
> Cc: [email protected]
> Signed-off-by: Shuangpeng Bai <[email protected]>
> ---
>   fs/btrfs/direct-io.c | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/btrfs/direct-io.c b/fs/btrfs/direct-io.c
> index 460326d34143..2b1a55769ec6 100644
> --- a/fs/btrfs/direct-io.c
> +++ b/fs/btrfs/direct-io.c
> @@ -281,17 +281,19 @@ static int btrfs_get_blocks_direct_write(struct extent_map **map,
>   		em2 = btrfs_create_dio_extent(BTRFS_I(inode), dio_data, start,
>   					      &file_extent, type);
>   		btrfs_dec_nocow_writers(bg);
> +		if (IS_ERR(em2)) {
> +			ret = PTR_ERR(em2);
> +			btrfs_free_extent_map(em);
> +			*map = NULL;
> +			goto out;
> +		}
> +

Overall looks good to me.


The function btrfs_create_dio_extent() is a pretty bad design for the 
lifespan management of the @em2.

The idea behind it is, for true NOCOW writes we do not need to update 
the extent map. But for PREALLOC writes we have to update the extent map.

So btrfs_create_dio_extent() can return a new em for preallocated 
writes, or return NULL for true NOCOW.

Thus I'd prefer to have a new comment, explaining the returned @em2 
situation.

>   		if (type == BTRFS_ORDERED_PREALLOC) {

And change the above "(type == PREALLOC)" condition to just "(em2)".
And maybe a new "ASSERT(type == BTRFS_ORDERED_PREALLOC);" to be extra sure.

Thanks,
Qu

>   			btrfs_free_extent_map(em);
>   			*map = em2;
>   			em = em2;
>   		}
>   
> -		if (IS_ERR(em2)) {
> -			ret = PTR_ERR(em2);
> -			goto out;
> -		}
> -
>   		dio_data->nocow_done = true;
>   	} else {
>   		/* Our caller expects us to free the input extent map. */