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

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5415 bytes --]



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;
>>  	}
>>


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]