[PATCH 0/8] Fix error handling on data bio submission

[PATCH 0/8] Fix error handling on data bio submission

[Josef Bacik]

Internally we tried to enable a remediation to automatically re-provision
machines that had checksum errors.  This was based on dmesg scanning, however we
discovered that we were getting transienty csum error messages.  This came down
to getting a transient ENOMEM while trying to lookup checksums while doing a
data read (this was on memory constrained containers).

What we were doing was simply acting like there wasn't a checksum there, which
would print a scary message about missing checksums.  And then we'd do the read,
but because we didn't have a checksum we'd complain about a checksum mismatch.
Neither of these things were actually what was happening, we simply got an EIO
while looking up the checksums.

Fix this by properly returning an error and erroring out the BIO with the
correct error.  This is actually correct, it allows us to skip the IO and also
not erroneously tell the user that their checksums are invalid.

While testing this fix however I uncovered a variety of problems with our error
handling when we submit.  So the first two patches are to fix the main problem I
wanted to fix, and the next 6 are to fix problems that happen when injecting
errors into the checksum lookup path.

With these patches I'm no longer getting csum mismatch errors when I fail to
lookup csums, and I'm also able to survive a xfstests run while randomly
injecting errors into this path.  Thanks,

Josef

Josef Bacik (8):
  btrfs: make search_csum_tree return 0 if we get -EFBIG
  btrfs: handle csum lookup errors properly on reads
  btrfs: check correct bio in finish_compressed_bio_read
  btrfs: remove the bio argument from finish_compressed_bio_read
  btrfs: track compressed bio errors as blk_status_t
  btrfs: do not double complete bio on errors during compressed reads
  btrfs: do not try to repair bio that has no mirror set
  btrfs: do not clean up repair bio if submit fails

 fs/btrfs/compression.c | 50 +++++++++++++++++++++++-------------------
 fs/btrfs/compression.h |  2 +-
 fs/btrfs/extent_io.c   | 25 ++++++++++++++-------
 fs/btrfs/file-item.c   | 15 ++++++++++---
 fs/btrfs/inode.c       | 12 ++++++----
 5 files changed, 65 insertions(+), 39 deletions(-)

-- 
2.26.3

[PATCH 1/8] btrfs: make search_csum_tree return 0 if we get -EFBIG

[Josef Bacik]

We can either fail to find a csum entry at all and return -ENOENT, or we
can find a range that is close, but return -EFBIG.  In essence these
both mean the same thing when we are doing a lookup for a csum in an
existing range, we didn't find a csum.  We want to treat both of these
errors the same way, complain loudly that there wasn't a csum.  This
currently happens anyway because we do

	count = search_csum_tree();
	if (count <= 0) {
		// reloc and error handling
	}

however it forces us to incorrectly treat EIO or ENOMEM errors as on
disk corruption.  Fix this by returning 0 if we get either -ENOENT or
-EFBIG from btrfs_lookup_csum() so we can do proper error handling.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/file-item.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 9a3de652ada8..efb24cc0b083 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -305,7 +305,7 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
 	read_extent_buffer(path->nodes[0], dst, (unsigned long)item,
 			ret * csum_size);
 out:
-	if (ret == -ENOENT)
+	if (ret == -ENOENT || ret == -EFBIG)
 		ret = 0;
 	return ret;
 }
-- 
2.26.3

Re: [PATCH 1/8] btrfs: make search_csum_tree return 0 if we get -EFBIG

[Boris Burkov]

On Thu, Feb 10, 2022 at 05:44:19PM -0500, Josef Bacik wrote:
> We can either fail to find a csum entry at all and return -ENOENT, or we
> can find a range that is close, but return -EFBIG.  In essence these
> both mean the same thing when we are doing a lookup for a csum in an
> existing range, we didn't find a csum.  We want to treat both of these
> errors the same way, complain loudly that there wasn't a csum.  This
> currently happens anyway because we do
> 
> 	count = search_csum_tree();
> 	if (count <= 0) {
> 		// reloc and error handling
> 	}
> 
> however it forces us to incorrectly treat EIO or ENOMEM errors as on
> disk corruption.  Fix this by returning 0 if we get either -ENOENT or
> -EFBIG from btrfs_lookup_csum() so we can do proper error handling.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/file-item.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 9a3de652ada8..efb24cc0b083 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -305,7 +305,7 @@ static int search_csum_tree(struct btrfs_fs_info *fs_info,
>  	read_extent_buffer(path->nodes[0], dst, (unsigned long)item,
>  			ret * csum_size);
>  out:
> -	if (ret == -ENOENT)
> +	if (ret == -ENOENT || ret == -EFBIG)
>  		ret = 0;
>  	return ret;
>  }
> -- 
> 2.26.3
> 

Re: [PATCH 1/8] btrfs: make search_csum_tree return 0 if we get -EFBIG

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

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

[Josef Bacik]

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) {
 			/*
 			 * 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

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

[Boris Burkov]

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
> 

[PATCH 3/8] btrfs: check correct bio in finish_compressed_bio_read

[Josef Bacik]

Commit c09abff87f90 ("btrfs: cloned bios must not be iterated by
bio_for_each_segment_all") added ASSERT()'s to make sure we weren't
calling bio_for_each_segment_all() on a RAID5/6 bio.  However it was
checking the bio that the compression code passed in, not the
cb->orig_bio that we actually iterate over, so adjust this ASSERT() to
check the correct bio.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/compression.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 71e5b2e9a1ba..a9d458305a08 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -259,7 +259,7 @@ static void finish_compressed_bio_read(struct compressed_bio *cb, struct bio *bi
 		 * We have verified the checksum already, set page checked so
 		 * the end_io handlers know about it
 		 */
-		ASSERT(!bio_flagged(bio, BIO_CLONED));
+		ASSERT(!bio_flagged(cb->orig_bio, BIO_CLONED));
 		bio_for_each_segment_all(bvec, cb->orig_bio, iter_all) {
 			u64 bvec_start = page_offset(bvec->bv_page) +
 					 bvec->bv_offset;
-- 
2.26.3

Re: [PATCH 3/8] btrfs: check correct bio in finish_compressed_bio_read

[Boris Burkov]

On Thu, Feb 10, 2022 at 05:44:21PM -0500, Josef Bacik wrote:
> Commit c09abff87f90 ("btrfs: cloned bios must not be iterated by
> bio_for_each_segment_all") added ASSERT()'s to make sure we weren't
> calling bio_for_each_segment_all() on a RAID5/6 bio.  However it was
> checking the bio that the compression code passed in, not the
> cb->orig_bio that we actually iterate over, so adjust this ASSERT() to
> check the correct bio.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/compression.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 71e5b2e9a1ba..a9d458305a08 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -259,7 +259,7 @@ static void finish_compressed_bio_read(struct compressed_bio *cb, struct bio *bi
>  		 * We have verified the checksum already, set page checked so
>  		 * the end_io handlers know about it
>  		 */
> -		ASSERT(!bio_flagged(bio, BIO_CLONED));
> +		ASSERT(!bio_flagged(cb->orig_bio, BIO_CLONED));
>  		bio_for_each_segment_all(bvec, cb->orig_bio, iter_all) {
>  			u64 bvec_start = page_offset(bvec->bv_page) +
>  					 bvec->bv_offset;
> -- 
> 2.26.3
> 

Re: [PATCH 3/8] btrfs: check correct bio in finish_compressed_bio_read

[Johannes Thumshirn]

On 10/02/2022 23:45, Josef Bacik wrote:
> Commit c09abff87f90 ("btrfs: cloned bios must not be iterated by
> bio_for_each_segment_all") added ASSERT()'s to make sure we weren't
> calling bio_for_each_segment_all() on a RAID5/6 bio.  However it was
> checking the bio that the compression code passed in, not the
> cb->orig_bio that we actually iterate over, so adjust this ASSERT() to
> check the correct bio.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/compression.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
> index 71e5b2e9a1ba..a9d458305a08 100644
> --- a/fs/btrfs/compression.c
> +++ b/fs/btrfs/compression.c
> @@ -259,7 +259,7 @@ static void finish_compressed_bio_read(struct compressed_bio *cb, struct bio *bi
>  		 * We have verified the checksum already, set page checked so
>  		 * the end_io handlers know about it
>  		 */
> -		ASSERT(!bio_flagged(bio, BIO_CLONED));
> +		ASSERT(!bio_flagged(cb->orig_bio, BIO_CLONED));
>  		bio_for_each_segment_all(bvec, cb->orig_bio, iter_all) {
>  			u64 bvec_start = page_offset(bvec->bv_page) +
>  					 bvec->bv_offset;
Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 4/8] btrfs: remove the bio argument from finish_compressed_bio_read

[Josef Bacik]

This bio is usually one of the compressed bio's, and we don't actually
need it in this function, so remove the argument and stop passing it
around.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/compression.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index a9d458305a08..0112fba345d4 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -234,7 +234,7 @@ static bool dec_and_test_compressed_bio(struct compressed_bio *cb, struct bio *b
 	return last_io;
 }
 
-static void finish_compressed_bio_read(struct compressed_bio *cb, struct bio *bio)
+static void finish_compressed_bio_read(struct compressed_bio *cb)
 {
 	unsigned int index;
 	struct page *page;
@@ -253,8 +253,6 @@ static void finish_compressed_bio_read(struct compressed_bio *cb, struct bio *bi
 		struct bio_vec *bvec;
 		struct bvec_iter_all iter_all;
 
-		ASSERT(bio);
-		ASSERT(!bio->bi_status);
 		/*
 		 * We have verified the checksum already, set page checked so
 		 * the end_io handlers know about it
@@ -325,7 +323,7 @@ static void end_compressed_bio_read(struct bio *bio)
 csum_failed:
 	if (ret)
 		cb->errors = 1;
-	finish_compressed_bio_read(cb, bio);
+	finish_compressed_bio_read(cb);
 out:
 	bio_put(bio);
 }
@@ -970,7 +968,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 	 */
 	ASSERT(refcount_read(&cb->pending_sectors));
 	/* Now we are the only one referring @cb, can finish it safely. */
-	finish_compressed_bio_read(cb, NULL);
+	finish_compressed_bio_read(cb);
 	return ret;
 }
 
-- 
2.26.3

Re: [PATCH 4/8] btrfs: remove the bio argument from finish_compressed_bio_read

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 5/8] btrfs: track compressed bio errors as blk_status_t

[Josef Bacik]

Right now we just have a binary "errors" flag, so any error we get on
the compressed bio's gets translated to EIO.  This isn't necessarily a
bad thing, but if we get an ENOMEM it may be nice to know that's what
happened instead of an EIO.  Track our errors as a blk_status_t, and do
the appropriate setting of the errors accordingly.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/compression.c | 26 ++++++++++++++------------
 fs/btrfs/compression.h |  2 +-
 2 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/compression.c b/fs/btrfs/compression.c
index 0112fba345d4..ee1c6f870a03 100644
--- a/fs/btrfs/compression.c
+++ b/fs/btrfs/compression.c
@@ -219,7 +219,7 @@ static bool dec_and_test_compressed_bio(struct compressed_bio *cb, struct bio *b
 		bi_size += bvec->bv_len;
 
 	if (bio->bi_status)
-		cb->errors = 1;
+		cb->status = bio->bi_status;
 
 	ASSERT(bi_size && bi_size <= cb->compressed_len);
 	last_io = refcount_sub_and_test(bi_size >> fs_info->sectorsize_bits,
@@ -247,8 +247,9 @@ static void finish_compressed_bio_read(struct compressed_bio *cb)
 	}
 
 	/* Do io completion on the original bio */
-	if (cb->errors) {
-		bio_io_error(cb->orig_bio);
+	if (cb->status != BLK_STS_OK) {
+		cb->orig_bio->bi_status = cb->status;
+		bio_endio(cb->orig_bio);
 	} else {
 		struct bio_vec *bvec;
 		struct bvec_iter_all iter_all;
@@ -306,7 +307,7 @@ static void end_compressed_bio_read(struct bio *bio)
 	 * Some IO in this cb have failed, just skip checksum as there
 	 * is no way it could be correct.
 	 */
-	if (cb->errors == 1)
+	if (cb->status != BLK_STS_OK)
 		goto csum_failed;
 
 	inode = cb->inode;
@@ -322,7 +323,7 @@ static void end_compressed_bio_read(struct bio *bio)
 
 csum_failed:
 	if (ret)
-		cb->errors = 1;
+		cb->status = errno_to_blk_status(ret);
 	finish_compressed_bio_read(cb);
 out:
 	bio_put(bio);
@@ -340,11 +341,12 @@ static noinline void end_compressed_writeback(struct inode *inode,
 	unsigned long end_index = (cb->start + cb->len - 1) >> PAGE_SHIFT;
 	struct page *pages[16];
 	unsigned long nr_pages = end_index - index + 1;
+	int errno = blk_status_to_errno(cb->status);
 	int i;
 	int ret;
 
-	if (cb->errors)
-		mapping_set_error(inode->i_mapping, -EIO);
+	if (errno)
+		mapping_set_error(inode->i_mapping, errno);
 
 	while (nr_pages > 0) {
 		ret = find_get_pages_contig(inode->i_mapping, index,
@@ -356,7 +358,7 @@ static noinline void end_compressed_writeback(struct inode *inode,
 			continue;
 		}
 		for (i = 0; i < ret; i++) {
-			if (cb->errors)
+			if (errno)
 				SetPageError(pages[i]);
 			btrfs_page_clamp_clear_writeback(fs_info, pages[i],
 							 cb->start, cb->len);
@@ -372,14 +374,14 @@ static void finish_compressed_bio_write(struct compressed_bio *cb)
 {
 	struct inode *inode = cb->inode;
 	unsigned int index;
+	int errno = blk_status_to_errno(cb->status);
 
 	/*
 	 * Ok, we're the last bio for this extent, step one is to call back
 	 * into the FS and do all the end_io operations.
 	 */
 	btrfs_writepage_endio_finish_ordered(BTRFS_I(inode), NULL,
-			cb->start, cb->start + cb->len - 1,
-			!cb->errors);
+			cb->start, cb->start + cb->len - 1, errno == 0);
 
 	end_compressed_writeback(inode, cb);
 	/* Note, our inode could be gone now */
@@ -522,7 +524,7 @@ blk_status_t btrfs_submit_compressed_write(struct btrfs_inode *inode, u64 start,
 	if (!cb)
 		return BLK_STS_RESOURCE;
 	refcount_set(&cb->pending_sectors, compressed_len >> fs_info->sectorsize_bits);
-	cb->errors = 0;
+	cb->status = BLK_STS_OK;
 	cb->inode = &inode->vfs_inode;
 	cb->start = start;
 	cb->len = len;
@@ -829,7 +831,7 @@ blk_status_t btrfs_submit_compressed_read(struct inode *inode, struct bio *bio,
 		goto out;
 
 	refcount_set(&cb->pending_sectors, compressed_len >> fs_info->sectorsize_bits);
-	cb->errors = 0;
+	cb->status = BLK_STS_OK;
 	cb->inode = inode;
 	cb->mirror_num = mirror_num;
 	sums = cb->sums;
diff --git a/fs/btrfs/compression.h b/fs/btrfs/compression.h
index 7dbd14caab01..c224c67f8c5e 100644
--- a/fs/btrfs/compression.h
+++ b/fs/btrfs/compression.h
@@ -55,7 +55,7 @@ struct compressed_bio {
 	u8 compress_type;
 
 	/* IO errors */
-	u8 errors;
+	blk_status_t status;
 	int mirror_num;
 
 	/* for reads, this is the bio we are copying the data into */
-- 
2.26.3

Re: [PATCH 5/8] btrfs: track compressed bio errors as blk_status_t

[Johannes Thumshirn]

On 10/02/2022 23:45, Josef Bacik wrote:
> @@ -372,14 +374,14 @@ static void finish_compressed_bio_write(struct compressed_bio *cb)
>  {
>  	struct inode *inode = cb->inode;
>  	unsigned int index;
> +	int errno = blk_status_to_errno(cb->status);
>  
>  	/*
>  	 * Ok, we're the last bio for this extent, step one is to call back
>  	 * into the FS and do all the end_io operations.
>  	 */
>  	btrfs_writepage_endio_finish_ordered(BTRFS_I(inode), NULL,
> -			cb->start, cb->start + cb->len - 1,
> -			!cb->errors);
> +			cb->start, cb->start + cb->len - 1, errno == 0);

No need for the errno here, it could just be:

	btrfs_writepage_endio_finish_ordered(BTRFS_I(inode), NULL,
			cb->start, cb->start + cb->len - 1,
			cb->status == BLK_STS_OK);

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

[Josef Bacik]

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>
---
 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;
 	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

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

[Boris Burkov]

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
> 

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

[David Sterba]

On Fri, Feb 11, 2022 at 02:54:22PM -0800, Boris Burkov wrote:
> > @@ -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'.

Yeah, this is IMHO more clear which error is returned from the error
branches than some random value set before.

[PATCH 7/8] btrfs: do not try to repair bio that has no mirror set

[Josef Bacik]

If we fail to submit a bio for whatever reason, we may not have setup a
mirror_num for that bio.  This means we shouldn't try to do the repair
workflow, if we do we'll hit an BUG_ON(!failrec->this_mirror) in
clean_io_failure.  Instead simply skip the repair workflow if we have no
mirror set, and add an assert to btrfs_check_repairable() to make it
easier to catch what is happening in the future.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index bda7fa8cf540..29ffb2814e5c 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2610,6 +2610,7 @@ static bool btrfs_check_repairable(struct inode *inode,
 	 * a good copy of the failed sector and if we succeed, we have setup
 	 * everything for repair_io_failure to do the rest for us.
 	 */
+	ASSERT(failed_mirror);
 	failrec->failed_mirror = failed_mirror;
 	failrec->this_mirror++;
 	if (failrec->this_mirror == failed_mirror)
@@ -3067,6 +3068,14 @@ static void end_bio_extent_readpage(struct bio *bio)
 			goto readpage_ok;
 
 		if (is_data_inode(inode)) {
+			/*
+			 * If we failed to submit the IO at all we'll have a
+			 * mirror_num == 0, in which case we need to just mark
+			 * the page with an error and unlock it and carry on.
+			 */
+			if (mirror == 0)
+				goto readpage_ok;
+
 			/*
 			 * btrfs_submit_read_repair() will handle all the good
 			 * and bad sectors, we just continue to the next bvec.
-- 
2.26.3

Re: [PATCH 7/8] btrfs: do not try to repair bio that has no mirror set

[Boris Burkov]

On Thu, Feb 10, 2022 at 05:44:25PM -0500, Josef Bacik wrote:
> If we fail to submit a bio for whatever reason, we may not have setup a
> mirror_num for that bio.  This means we shouldn't try to do the repair
> workflow, if we do we'll hit an BUG_ON(!failrec->this_mirror) in
> clean_io_failure.  Instead simply skip the repair workflow if we have no
> mirror set, and add an assert to btrfs_check_repairable() to make it
> easier to catch what is happening in the future.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/extent_io.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index bda7fa8cf540..29ffb2814e5c 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2610,6 +2610,7 @@ static bool btrfs_check_repairable(struct inode *inode,
>  	 * a good copy of the failed sector and if we succeed, we have setup
>  	 * everything for repair_io_failure to do the rest for us.
>  	 */
> +	ASSERT(failed_mirror);
>  	failrec->failed_mirror = failed_mirror;
>  	failrec->this_mirror++;
>  	if (failrec->this_mirror == failed_mirror)
> @@ -3067,6 +3068,14 @@ static void end_bio_extent_readpage(struct bio *bio)
>  			goto readpage_ok;
>  
>  		if (is_data_inode(inode)) {
> +			/*
> +			 * If we failed to submit the IO at all we'll have a
> +			 * mirror_num == 0, in which case we need to just mark
> +			 * the page with an error and unlock it and carry on.
> +			 */
> +			if (mirror == 0)
> +				goto readpage_ok;
> +
>  			/*
>  			 * btrfs_submit_read_repair() will handle all the good
>  			 * and bad sectors, we just continue to the next bvec.
> -- 
> 2.26.3
> 

[PATCH 8/8] btrfs: do not clean up repair bio if submit fails

[Josef Bacik]

The submit helper will always run bio_endio() on the bio if it fails to
submit, so cleaning up the bio just leads to a variety of UAF and NULL
pointer deref bugs because we race with the endio function that is
cleaning up the bio.  Instead just return STS_OK as the repair function
has to continue to process the rest of the pages, and the endio for the
repair bio will do the appropriate cleanup for the page that it was
given.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 29ffb2814e5c..16b6820c913d 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2640,7 +2640,6 @@ int btrfs_repair_one_sector(struct inode *inode,
 	const int icsum = bio_offset >> fs_info->sectorsize_bits;
 	struct bio *repair_bio;
 	struct btrfs_bio *repair_bbio;
-	blk_status_t status;
 
 	btrfs_debug(fs_info,
 		   "repair read error: read error at %llu", start);
@@ -2679,13 +2678,14 @@ int btrfs_repair_one_sector(struct inode *inode,
 		    "repair read error: submitting new read to mirror %d",
 		    failrec->this_mirror);
 
-	status = submit_bio_hook(inode, repair_bio, failrec->this_mirror,
-				 failrec->bio_flags);
-	if (status) {
-		free_io_failure(failure_tree, tree, failrec);
-		bio_put(repair_bio);
-	}
-	return blk_status_to_errno(status);
+	/*
+	 * At this point we have a bio, so any errors from submit_bio_hook()
+	 * will be handled by the endio on the repair_bio, so we can't return an
+	 * error here.
+	 */
+	submit_bio_hook(inode, repair_bio, failrec->this_mirror,
+			failrec->bio_flags);
+	return BLK_STS_OK;
 }
 
 static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
-- 
2.26.3

Re: [PATCH 8/8] btrfs: do not clean up repair bio if submit fails

[Boris Burkov]

On Thu, Feb 10, 2022 at 05:44:26PM -0500, Josef Bacik wrote:
> The submit helper will always run bio_endio() on the bio if it fails to
> submit, so cleaning up the bio just leads to a variety of UAF and NULL
> pointer deref bugs because we race with the endio function that is
> cleaning up the bio.  Instead just return STS_OK as the repair function
> has to continue to process the rest of the pages, and the endio for the
> repair bio will do the appropriate cleanup for the page that it was
> given.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
Reviewed-by: Boris Burkov <boris@bur.io>
> ---
>  fs/btrfs/extent_io.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 29ffb2814e5c..16b6820c913d 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -2640,7 +2640,6 @@ int btrfs_repair_one_sector(struct inode *inode,
>  	const int icsum = bio_offset >> fs_info->sectorsize_bits;
>  	struct bio *repair_bio;
>  	struct btrfs_bio *repair_bbio;
> -	blk_status_t status;
>  
>  	btrfs_debug(fs_info,
>  		   "repair read error: read error at %llu", start);
> @@ -2679,13 +2678,14 @@ int btrfs_repair_one_sector(struct inode *inode,
>  		    "repair read error: submitting new read to mirror %d",
>  		    failrec->this_mirror);
>  
> -	status = submit_bio_hook(inode, repair_bio, failrec->this_mirror,
> -				 failrec->bio_flags);
> -	if (status) {
> -		free_io_failure(failure_tree, tree, failrec);
> -		bio_put(repair_bio);
> -	}
> -	return blk_status_to_errno(status);
> +	/*
> +	 * At this point we have a bio, so any errors from submit_bio_hook()
> +	 * will be handled by the endio on the repair_bio, so we can't return an
> +	 * error here.
> +	 */
> +	submit_bio_hook(inode, repair_bio, failrec->this_mirror,
> +			failrec->bio_flags);
> +	return BLK_STS_OK;
>  }
>  
>  static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
> -- 
> 2.26.3
>