Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Nikolay Borisov <nborisov@suse.com>]

On 2.08.21 г. 9:54, Qu Wenruo wrote:
> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
> while we don't have ordered extent for it at all.
> 
> Normally this won't happen and is indeed a code logical error.
> 
> But previous fix has already shown another possibility that, some call
> sites don't handle error properly and submit the write bio after its
> ordered extent has already been cleaned up.
> 
> This patch will add an extra safe net by replacing the BUG_ON() to
> proper error handling.
> 
> And even if some day we hit a regression that we're submitting bio
> without an ordered extent, we will return error and the pages will be
> marked Error, and being caught properly.

Would this hamper debugability? I.e it will result in some writes
failing with an error, right?

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file-item.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 2673c6ba7a4e..25205b9dad69 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -665,7 +665,19 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
>  
>  		if (!ordered) {
>  			ordered = btrfs_lookup_ordered_extent(inode, offset);
> -			BUG_ON(!ordered); /* Logic error */
> +			/*
> +			 * No ordered extent mostly means the OE has been
> +			 * removed (mostly for error handling). Normally for
> +			 * such case we should not flush_write_bio(), but
> +			 * end_write_bio().
> +			 *
> +			 * But an extra safe net will never hurt. Just error
> +			 * out.
> +			 */
> +			if (unlikely(!ordered)) {
> +				kvfree(sums);
> +				return BLK_STS_IOERR;
> +			}
>  		}
>  
>  		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
>