Re: [PATCH] btrfs: fix error handling in commit_fs_roots

[Nikolay Borisov <nborisov@suse.com>]

On 1.12.20 г. 16:53 ч., Josef Bacik wrote:
> While doing error injection I would sometimes get a corrupt file system.
> This is because I was injecting errors at btrfs_search_slot, but would
> only do it one time per stack.  This uncovered a problem in
> commit_fs_roots, where if we get an error we would just break.  However
> we're in a second loop, the first loop being a loop to find all the

nit: s/second/nested as initially I go confused about us being in a 2nd
loop iteration. Using nested makes it a bit  clearer

> dirty fs roots, and then subsequent root updates would succeed clearing
> the error value.
> 
> This isn't likely to happen in real scenarios, however we could
> potentially get a random ENOMEM once and then not again, and we'd end up
> with a corrupted file system.  Fix this by moving the error checking
> around a bit to the main loop, as this is the only place where something
> will fail, and return the error as soon as it occurs.
> 
> With this patch my reproducer no longer corrupts the file system.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/transaction.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
> index 1dac76b7ea96..b05f75654b16 100644
> --- a/fs/btrfs/transaction.c
> +++ b/fs/btrfs/transaction.c
> @@ -1328,7 +1328,6 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>  	struct btrfs_root *gang[8];
>  	int i;
>  	int ret;
> -	int err = 0;
>  
>  	spin_lock(&fs_info->fs_roots_radix_lock);
>  	while (1) {
> @@ -1340,6 +1339,8 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>  			break;
>  		for (i = 0; i < ret; i++) {
>  			struct btrfs_root *root = gang[i];
> +			int err;
> +
>  			radix_tree_tag_clear(&fs_info->fs_roots_radix,
>  					(unsigned long)root->root_key.objectid,
>  					BTRFS_ROOT_TRANS_TAG);
> @@ -1366,14 +1367,14 @@ static noinline int commit_fs_roots(struct btrfs_trans_handle *trans)
>  			err = btrfs_update_root(trans, fs_info->tree_root,
>  						&root->root_key,
>  						&root->root_item);
> -			spin_lock(&fs_info->fs_roots_radix_lock);
>  			if (err)
> -				break;
> +				return err;
> +			spin_lock(&fs_info->fs_roots_radix_lock);
>  			btrfs_qgroup_free_meta_all_pertrans(root);
>  		}
>  	}
>  	spin_unlock(&fs_info->fs_roots_radix_lock);
> -	return err;
> +	return 0;
>  }
>  
>  /*
>