Re: [PATCH 5/5] btrfs: extent_io: Unify the return value of btrfs_clone_extent_buffer() with alloc_extent_buffer()

[Nikolay Borisov <n.borisov.lkml@gmail.com>]

On 22.02.19 г. 12:16 ч., Qu Wenruo wrote:
> The direct callers are:
> 1) qgroup_rescan_leaf()
> 2) btrfs_compare_trees()
>    These two call sites are stubs.
> 
> 3) tree_mod_log_rewind()
> 4) get_old_root()
>    These two doesn't follow the name pattern *_extent_buffer(), thus we
>    don't need to unify their return value.
>    However for get_old_root(), all its eb assignment can no longer be
>    NULL, change its IS_ERR_OR_NULL() to IS_ERR().
> 
> 5) btrfs_search_slot_get_root()
>    This is a stub. The only caller has already handled the return value.
> 
> This patch should cover all the call sites of
> btrfs_clone_extent_buffer().
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/backref.c   |  8 ++++----
>  fs/btrfs/ctree.c     | 14 ++++++++------
>  fs/btrfs/extent_io.c |  4 ++--
>  fs/btrfs/qgroup.c    |  5 +++--
>  4 files changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
> index 78556447e1d5..b6e469a12011 100644
> --- a/fs/btrfs/backref.c
> +++ b/fs/btrfs/backref.c
> @@ -2016,8 +2016,8 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
>  		parent = found_key.offset;
>  		slot = path->slots[0];
>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
> -		if (!eb) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(eb)) {
> +			ret = PTR_ERR(eb);
>  			break;
>  		}
>  		btrfs_release_path(path);
> @@ -2075,8 +2075,8 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
>  
>  		slot = path->slots[0];
>  		eb = btrfs_clone_extent_buffer(path->nodes[0]);
> -		if (!eb) {
> -			ret = -ENOMEM;
> +		if (IS_ERR(eb)) {
> +			ret = PTR_ERR(eb);
>  			break;
>  		}
>  		btrfs_release_path(path);
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3276b743f40a..ea7ff0012007 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -1384,7 +1384,7 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
>  		free_extent_buffer(eb_root);
>  	}
>  
> -	if (IS_ERR_OR_NULL(eb))
> +	if (IS_ERR(eb))
>  		return NULL;
>  	btrfs_tree_read_lock(eb);
>  	if (old_root) {
> @@ -2624,8 +2624,8 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>  			down_read(&fs_info->commit_root_sem);
>  			b = btrfs_clone_extent_buffer(root->commit_root);
>  			up_read(&fs_info->commit_root_sem);
> -			if (!b)
> -				return ERR_PTR(-ENOMEM);
> +			if (IS_ERR(b))
> +				return b;
>  
>  		} else {
>  			b = root->commit_root;
> @@ -5431,7 +5431,8 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>  			btrfs_clone_extent_buffer(left_root->commit_root);
>  	if (!left_path->nodes[left_level]) {
>  		up_read(&fs_info->commit_root_sem);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(left_path->nodes[left_level]);
> +		left_path->nodes[left_level] = NULL;

If we go into this branch this means left_path->nodes[left_level] was
NULL. PTR_ERR of NULL is, well, NULL and then you also set the
left_level to NULL. This is pointless...

>  		goto out;
>  	}
>  
> @@ -5439,9 +5440,10 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
>  	right_root_level = right_level;
>  	right_path->nodes[right_level] =
>  			btrfs_clone_extent_buffer(right_root->commit_root);
> -	if (!right_path->nodes[right_level]) {
> +	if (IS_ERR(right_path->nodes[right_level])) {

And here you do change the check to IS_ERR but only for the right_Path
and not left path this introduces asymmetry which makes code harder to
comprehend.

>  		up_read(&fs_info->commit_root_sem);
> -		ret = -ENOMEM;
> +		ret = PTR_ERR(right_path->nodes[right_level]);
> +		right_path->nodes[right_level] = NULL;
>  		goto out;
>  	}
>  	up_read(&fs_info->commit_root_sem);
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index a19bd8bc3b32..dbdb649f1957 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4713,13 +4713,13 @@ struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src)
>  
>  	new = __alloc_extent_buffer(src->fs_info, src->start, src->len);
>  	if (IS_ERR(new))
> -		return NULL;
> +		return new;
>  
>  	for (i = 0; i < num_pages; i++) {
>  		p = alloc_page(GFP_NOFS);
>  		if (!p) {
>  			btrfs_release_extent_buffer(new);
> -			return NULL;
> +			return ERR_PTR(-ENOMEM);
>  		}
>  		attach_extent_buffer_page(new, p);
>  		WARN_ON(PageDirty(p));
> diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
> index 4e473a998219..ccfa992a10bf 100644
> --- a/fs/btrfs/qgroup.c
> +++ b/fs/btrfs/qgroup.c
> @@ -3105,8 +3105,9 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
>  	fs_info->qgroup_rescan_progress.objectid = found.objectid + 1;
>  
>  	scratch_leaf = btrfs_clone_extent_buffer(path->nodes[0]);
> -	if (!scratch_leaf) {
> -		ret = -ENOMEM;
> +	if (IS_ERR(scratch_leaf)) {
> +		ret = PTR_ERR(scratch_leaf);
> +		scratch_leaf = NULL;
>  		mutex_unlock(&fs_info->qgroup_rescan_lock);
>  		goto out;
>  	}
>