[PATCH 0/2] btrfs_del_csums error handling fixes

[PATCH 0/2] btrfs_del_csums error handling fixes

[Josef Bacik]

Hello,

Here are two fixes related to deleting csums.  Doing error injection stress
testing I was consistently seeing cases where we had a corrupt file system with
csums that existed without the corresponding extents being written.  This was
occuring because we were losing the return value in two cases, both of which
would result in this style of corruption.  With these two patches I'm no longer
seeing these errors.  Thanks,

Josef

Josef Bacik (2):
  btrfs: fix error handling in btrfs_del_csums
  btrfs: return errors from btrfs_del_csums in cleanup_ref_head

 fs/btrfs/extent-tree.c |  4 ++--
 fs/btrfs/file-item.c   | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

-- 
2.26.3

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

[Josef Bacik]

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:
	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>
---
 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;
 }
-- 
2.26.3

[PATCH 2/2] btrfs: return errors from btrfs_del_csums in cleanup_ref_head

[Josef Bacik]

We are unconditionally returning 0 in cleanup_ref_head, despite the fact
that btrfs_del_csums could fail.  We need to return the error so the
transaction gets aborted properly, fix this by returning ret from
btrfs_del_csums in cleanup_ref_head.

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

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index b84bbc24ff57..790de24576ac 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -1826,7 +1826,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
 
 	struct btrfs_fs_info *fs_info = trans->fs_info;
 	struct btrfs_delayed_ref_root *delayed_refs;
-	int ret;
+	int ret = 0;
 
 	delayed_refs = &trans->transaction->delayed_refs;
 
@@ -1868,7 +1868,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
 	trace_run_delayed_ref_head(fs_info, head, 0);
 	btrfs_delayed_ref_unlock(head);
 	btrfs_put_delayed_ref_head(head);
-	return 0;
+	return ret;
 }
 
 static struct btrfs_delayed_ref_head *btrfs_obtain_ref_head(
-- 
2.26.3

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

[Qu Wenruo]

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

Re: [PATCH 2/2] btrfs: return errors from btrfs_del_csums in cleanup_ref_head

[Qu Wenruo]

On 2021/5/19 下午10:52, Josef Bacik wrote:
> We are unconditionally returning 0 in cleanup_ref_head, despite the fact
> that btrfs_del_csums could fail.  We need to return the error so the
> transaction gets aborted properly, fix this by returning ret from
> btrfs_del_csums in cleanup_ref_head.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

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

Thanks,
Qu
> ---
>   fs/btrfs/extent-tree.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b84bbc24ff57..790de24576ac 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1826,7 +1826,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
>
>   	struct btrfs_fs_info *fs_info = trans->fs_info;
>   	struct btrfs_delayed_ref_root *delayed_refs;
> -	int ret;
> +	int ret = 0;
>
>   	delayed_refs = &trans->transaction->delayed_refs;
>
> @@ -1868,7 +1868,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
>   	trace_run_delayed_ref_head(fs_info, head, 0);
>   	btrfs_delayed_ref_unlock(head);
>   	btrfs_put_delayed_ref_head(head);
> -	return 0;
> +	return ret;
>   }
>
>   static struct btrfs_delayed_ref_head *btrfs_obtain_ref_head(
>

Re: [PATCH 2/2] btrfs: return errors from btrfs_del_csums in cleanup_ref_head

[David Sterba]

On Wed, May 19, 2021 at 10:52:46AM -0400, Josef Bacik wrote:
> We are unconditionally returning 0 in cleanup_ref_head, despite the fact
> that btrfs_del_csums could fail.  We need to return the error so the
> transaction gets aborted properly, fix this by returning ret from
> btrfs_del_csums in cleanup_ref_head.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/extent-tree.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index b84bbc24ff57..790de24576ac 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -1826,7 +1826,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
>  
>  	struct btrfs_fs_info *fs_info = trans->fs_info;
>  	struct btrfs_delayed_ref_root *delayed_refs;
> -	int ret;
> +	int ret = 0;

>  
>  	delayed_refs = &trans->transaction->delayed_refs;

ret is used for a return just after this line

	ret = run_and_cleanup_extent_op(trans, head);

so not necessary to initialize at the declaration

>  
> @@ -1868,7 +1868,7 @@ static int cleanup_ref_head(struct btrfs_trans_handle *trans,
>  	trace_run_delayed_ref_head(fs_info, head, 0);
>  	btrfs_delayed_ref_unlock(head);
>  	btrfs_put_delayed_ref_head(head);
> -	return 0;
> +	return ret;
>  }
>  
>  static struct btrfs_delayed_ref_head *btrfs_obtain_ref_head(
> -- 
> 2.26.3

Re: [PATCH 0/2] btrfs_del_csums error handling fixes

[David Sterba]

On Wed, May 19, 2021 at 10:52:44AM -0400, Josef Bacik wrote:
> Hello,
> 
> Here are two fixes related to deleting csums.  Doing error injection stress
> testing I was consistently seeing cases where we had a corrupt file system with
> csums that existed without the corresponding extents being written.  This was
> occuring because we were losing the return value in two cases, both of which
> would result in this style of corruption.  With these two patches I'm no longer
> seeing these errors.  Thanks,
> 
> Josef
> 
> Josef Bacik (2):
>   btrfs: fix error handling in btrfs_del_csums
>   btrfs: return errors from btrfs_del_csums in cleanup_ref_head

Added to misc-next, thanks.