| Message ID | 20190222101645.4403-6-wqu@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Unify the return value of alloc/clone_extent_buffer() | expand |
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; > } >
On 2019/2/22 下午8:47, Nikolay Borisov wrote: > > > 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... Oh, previous "!left_path->nodes[left_level]" should be "IS_ERR(left_path->nodes[left_level]". That should explain all the problem. Thanks, Qu > >> 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; >> } >>
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; 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])) { 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; }
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(-)