Re: [PATCH 6/8] btrfs: do not double complete bio on errors during compressed reads

[Boris Burkov <boris@bur.io>]

On Thu, Feb 10, 2022 at 05:44:24PM -0500, Josef Bacik wrote:
> I hit some weird panics while fixing up the error handling from
> btrfs_lookup_bio_sums().  Turns out the compression path will complete
> the bio we use if we set up any of the compression bios and then return
> an error, and then btrfs_submit_data_bio() will also call bio_endio() on
> the bio.
> 
> Fix this by making btrfs_submit_compressed_read() responsible for
> calling bio_endio() on the bio if there are any errors.  Currently it
> was only doing it if we created the compression bios, otherwise it was
> depending on btrfs_submit_data_bio() to do the right thing.  This
> creates the above problem, so fix up btrfs_submit_compressed_read() to
> always call bio_endio() in case of an error, and then simply return from
> btrfs_submit_data_bio() if we had to call
> btrfs_submit_compressed_read().
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/compression.c | 14 +++++++++-----
>  fs/btrfs/inode.c       | 12 ++++++++----
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index ee1c6f870a03..9551658ac3a1 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -808,7 +808,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	u64 em_len;
>  	u64 em_start;
>  	struct extent_map *em;
> -	blk_status_t ret = BLK_STS_RESOURCE;
> +	blk_status_t ret;
>  	int faili = 0;
>  	u8 *sums;
>  
> @@ -821,9 +821,12 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	read_lock(&em_tree->lock);
>  	em = lookup_extent_mapping(em_tree, file_offset, fs_info->sectorsize);
>  	read_unlock(&em_tree->lock);
> -	if (!em)
> -		return BLK_STS_IOERR;
> +	if (!em) {
> +		ret = BLK_STS_IOERR;
> +		goto out;
> +	}
>  
> +	ret = BLK_STS_RESOURCE;

I think the error handling logic with all the special exit paths makes
it worthwhile to set ret at each individual 'goto failX'.

>  	ASSERT(em->compress_type != BTRFS_COMPRESS_NONE);
>  	compressed_len = em->block_len;
>  	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
> @@ -858,7 +861,6 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  		cb->compressed_pages[pg_index] = alloc_page(GFP_NOFS);
>  		if (!cb->compressed_pages[pg_index]) {
>  			faili = pg_index - 1;
> -			ret = BLK_STS_RESOURCE;
>  			goto fail2;
>  		}
>  	}
> @@ -938,7 +940,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  			comp_bio = NULL;
>  		}
>  	}
> -	return 0;
> +	return BLK_STS_OK;
>  
>  fail2:
>  	while (faili >= 0) {
> @@ -951,6 +953,8 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
>  	kfree(cb);
>  out:
>  	free_extent_map(em);
> +	bio->bi_status = ret;
> +	bio_endio(bio);
>  	return ret;
>  finish_cb:
>  	if (comp_bio) {
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 24099fe9e120..69fa71186e72 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -2542,10 +2542,14 @@ blk_status_t btrfs_submit_data_bio(struct inode *inode, struct bio *bio,
>  			goto out;
>  
>  		if (bio_flags & EXTENT_BIO_COMPRESSED) {
> -			ret = btrfs_submit_compressed_read(inode, bio,
> -							   mirror_num,
> -							   bio_flags);
> -			goto out;
> +			/*
> +			 * btrfs_submit_compressed_read will handle completing
> +			 * the bio if there were any errors, so just return
> +			 * here.
> +			 */
> +			return btrfs_submit_compressed_read(inode, bio,
> +							    mirror_num,
> +							    bio_flags);
>  		} else {
>  			/*
>  			 * Lookup bio sums does extra checks around whether we
> -- 
> 2.26.3
>