Re: [PATCH 2/8] btrfs: handle csum lookup errors properly on reads

[Boris Burkov <boris@bur.io>]

On Thu, Feb 10, 2022 at 05:44:20PM -0500, Josef Bacik wrote:
> Currently any error we get while trying to lookup csums during reads
> shows up as a missing csum, and then on the read completion side we spit
> out an error saying there was a csum mismatch and we increase the device
> corruption count.
> 
> However we could have gotten an EIO from the lookup.  We could also be
> inside of a memory constrained container and gotten a ENOMEM while
> trying to do the read.  In either case we don't want to make this look
> like a file system corruption problem, we want to make it look like the
> actual error it is.  Capture any negative value, convert it to the
> appropriate blk_sts_t, free the csum array if we have one and bail.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/file-item.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index efb24cc0b083..e68be492109d 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -368,6 +368,7 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, u8 *dst
>  {
>  	struct btrfs_fs_info *fs_info = btrfs_sb(inode->i_sb);
>  	struct extent_io_tree *io_tree = &BTRFS_I(inode)->io_tree;
> +	struct btrfs_bio *bbio = NULL;
>  	struct btrfs_path *path;
>  	const u32 sectorsize = fs_info->sectorsize;
>  	const u32 csum_size = fs_info->csum_size;
> @@ -377,6 +378,7 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, u8 *dst
>  	u8 *csum;
>  	const unsigned int nblocks = orig_len >> fs_info->sectorsize_bits;
>  	int count = 0;
> +	blk_status_t ret = BLK_STS_OK;
>  
>  	if ((BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM) ||
>  	    test_bit(BTRFS_FS_STATE_NO_CSUMS, &fs_info->fs_state))
> @@ -400,7 +402,7 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, u8 *dst
>  		return BLK_STS_RESOURCE;
>  
>  	if (!dst) {
> -		struct btrfs_bio *bbio = btrfs_bio(bio);
> +		bbio = btrfs_bio(bio);
>  
>  		if (nblocks * csum_size > BTRFS_BIO_INLINE_CSUM_SIZE) {
>  			bbio->csum = kmalloc_array(nblocks, csum_size, GFP_NOFS);
> @@ -456,6 +458,13 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, u8 *dst
>  
>  		count = search_csum_tree(fs_info, path, cur_disk_bytenr,
>  					 search_len, csum_dst);
> +		if (count < 0) {
> +			ret = errno_to_blk_status(count);
> +			if (bbio)
> +				btrfs_bio_free_csum(bbio);
> +			break;
> +		}
> +
>  		if (count <= 0) {

This new error handling doesn't quite mesh with the existing logic, IMO.

1) count <= 0 is redundant with count < 0 above and it's confusing to
think about whether this code runs for count < 0.

2) it really is not running the nodatacow thing for the reloc inodes for
negative return values anymore. Is that desirable? I found the original
commit about it:
'Btrfs: fix nodatasum handling in balancing code'
and it looks like that one cared about the 0 case, not the negative
case, but that's not what the logic or comment suggest as they are.

I think if you switch it to explicitly:
< 0 -> error; run the new bail code.
== 0 -> not found; deal with reloc, warn otherwise.
and make a single comment covering it, then it would make sense to me.

>  			/*
>  			 * Either we hit a critical error or we didn't find
> @@ -491,7 +500,7 @@ blk_status_t btrfs_lookup_bio_sums(struct inode *inode, struct bio *bio, u8 *dst
>  	}
>  
>  	btrfs_free_path(path);
> -	return BLK_STS_OK;
> +	return ret;
>  }
>  
>  int btrfs_lookup_csums_range(struct btrfs_root *root, u64 start, u64 end,
> -- 
> 2.26.3
>