Re: [PATCH 1/2] btrfs: fix error handling in btrfs_del_csums

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

On 2021/5/19 下午10:52, Josef Bacik wrote:
> Error injection stress would sometimes fail with checksums on disk that
> did not have a corresponding extent.  This occurred because the pattern
> in btrfs_del_csums was
>
> 	while (1) {
> 		ret = btrfs_search_slot();
> 		if (ret < 0)
> 			break;
> 	}
> 	ret = 0;
> out:

Such "ret = 0;" is definitely causing problem when break is used.

> 	btrfs_free_path(path);
> 	return ret;
>
> If we got an error from btrfs_search_slot we'd clear the error because
> we were breaking instead of goto out.  Instead of using goto out, simply
> handle the cases where we may leave a random value in ret, and get rid
> of the
>
> 	ret = 0;
> out:
>
> pattern and simply allow break to have the proper error reporting.  With
> this fix we properly abort the transaction and do not commit thinking we
> successfully deleted the csum.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>   fs/btrfs/file-item.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 294602f139ef..a5a8dac334e8 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -788,7 +788,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>   	u64 end_byte = bytenr + len;
>   	u64 csum_end;
>   	struct extent_buffer *leaf;
> -	int ret;
> +	int ret = 0;
>   	const u32 csum_size = fs_info->csum_size;
>   	u32 blocksize_bits = fs_info->sectorsize_bits;
>
> @@ -806,6 +806,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>
>   		ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
>   		if (ret > 0) {
> +			ret = 0;
>   			if (path->slots[0] == 0)
>   				break;
>   			path->slots[0]--;
> @@ -862,7 +863,7 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>   			ret = btrfs_del_items(trans, root, path,
>   					      path->slots[0], del_nr);
>   			if (ret)
> -				goto out;
> +				break;
>   			if (key.offset == bytenr)
>   				break;
>   		} else if (key.offset < bytenr && csum_end > end_byte) {
> @@ -906,8 +907,9 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>   			ret = btrfs_split_item(trans, root, path, &key, offset);
>   			if (ret && ret != -EAGAIN) {
>   				btrfs_abort_transaction(trans, ret);
> -				goto out;
> +				break;
>   			}
> +			ret = 0;
>
>   			key.offset = end_byte - 1;
>   		} else {
> @@ -917,8 +919,6 @@ int btrfs_del_csums(struct btrfs_trans_handle *trans,
>   		}
>   		btrfs_release_path(path);
>   	}
> -	ret = 0;
> -out:
>   	btrfs_free_path(path);
>   	return ret;
>   }
>