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 <w...@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;
> }
>
