On Thu, Feb 28, 2019 at 10:08:57AM +0800, Qu Wenruo wrote:
> [snip]
> >> ret = radix_tree_preload(GFP_NOFS);
> >> - if (ret)
> >> - goto free_eb;
> >> + if (ret) {
> >> + btrfs_release_extent_buffer(eb);
> >> + return ERR_PTR(ret);
> >> + }
> >> spin_lock(&fs_info->buffer_lock);
> >> ret = radix_tree_insert(&fs_info->buffer_radix,
> >> start >> PAGE_SHIFT, eb);
> >> @@ -4875,21 +4892,26 @@ struct extent_buffer
> >> *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
> >> radix_tree_preload_end();
> >> if (ret == -EEXIST) {
> >> exists = find_extent_buffer(fs_info, start);
> >> - if (exists)
> >> - goto free_eb;
> >> - else
> >> - goto again;
> >> + if (exists) {
> >> + btrfs_release_extent_buffer(eb);
> >> + return exists;
> >> + }
> >> + goto again;
> >> }
> >> check_buffer_tree_ref(eb);
> >> set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags);
> >>
> >> return eb;
> >> -free_eb:
> >> - btrfs_release_extent_buffer(eb);
> >> - return exists;
> >
> > Changes in this functions are more than just the conversion, namely
> > undoing the common exit block and adding the inline returns. These are
> > different logical things so I'd rather see that in a separate patch.
> > Thanks.
>
> This part is not just undoing the goto branch.
>
> For the error just after radix_tree_preload(GFP_NOFS), we're doing error
> out.
>
> For the return after find_extent_buffer(), we're returning valid found eb.
>
> The old code is abusing the exit for both error out and found case.
> Due to the change to ERR_PTR(), we can on longer follow that abuse any
> more, thus I have to remove the old free_eb label.
If you set exists to the ERR_PTR then the same code path is taken, this
is the exit block pattern. But the function is short and understandable
with the returns, so I'm ok with your version.
