Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Nikolay Borisov <nborisov@suse.com>, Qu Wenruo <wqu@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 3/8] btrfs: delayed-ref: Use btrfs_ref to refactor btrfs_add_delayed_tree_ref()
Date: Wed, 12 Dec 2018 13:50:54 +0800	[thread overview]
Message-ID: <f2d5a621-1321-5ddf-1e3d-76b3e2617d93@gmx.com> (raw)
In-Reply-To: <167bf109-41cf-f10b-4364-9af5701f5ee0@suse.com>

On 2018/12/10 下午5:21, Nikolay Borisov wrote:
> 
> 
> On 6.12.18 г. 8:58 ч., Qu Wenruo wrote:
>> btrfs_add_delayed_tree_ref() has a longer and longer parameter list, and
>> some caller like btrfs_inc_extent_ref() are using @owner as level for
>> delayed tree ref.
>>
>> Instead of making the parameter list longer and longer, use btrfs_ref to
>> refactor it, so each parameter assignment should be self-explaining
>> without dirty level/owner trick, and provides the basis for later refactor.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  fs/btrfs/delayed-ref.c | 24 ++++++++++++++-------
>>  fs/btrfs/delayed-ref.h |  4 +---
>>  fs/btrfs/extent-tree.c | 48 ++++++++++++++++++++++++------------------
>>  3 files changed, 44 insertions(+), 32 deletions(-)
>>
>> diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
>> index 11dd46be4017..c42b8ade7b07 100644
>> --- a/fs/btrfs/delayed-ref.c
>> +++ b/fs/btrfs/delayed-ref.c
>> @@ -710,9 +710,7 @@ static void init_delayed_ref_common(struct btrfs_fs_info *fs_info,
>>   * transaction commits.
>>   */
>>  int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
>> -			       u64 bytenr, u64 num_bytes, u64 parent,
>> -			       u64 ref_root,  int level, bool for_reloc,
>> -			       int action,
>> +			       struct btrfs_ref *generic_ref,
>>  			       struct btrfs_delayed_extent_op *extent_op,
>>  			       int *old_ref_mod, int *new_ref_mod)
>>  {
>> @@ -722,10 +720,17 @@ int btrfs_add_delayed_tree_ref(struct btrfs_trans_handle *trans,
>>  	struct btrfs_delayed_ref_root *delayed_refs;
>>  	struct btrfs_qgroup_extent_record *record = NULL;
>>  	int qrecord_inserted;
>> -	bool is_system = (ref_root == BTRFS_CHUNK_TREE_OBJECTID);
>> +	bool is_system = (generic_ref->real_root == BTRFS_CHUNK_TREE_OBJECTID);
>> +	int action = generic_ref->action;
>> +	int level = generic_ref->tree_ref.level;
>>  	int ret;
>> +	u64 bytenr = generic_ref->bytenr;
>> +	u64 num_bytes = generic_ref->len;
>> +	u64 parent = generic_ref->parent;
>>  	u8 ref_type;
>>  
>> +	ASSERT(generic_ref && generic_ref->type == BTRFS_REF_METADATA &&
>> +		generic_ref->action);
> 
> You have already dereferenced generic_ref by the time you ASSERT and
> would have crashed. I guess the first condition in the assert can be
> removed.
> 
>>  	BUG_ON(extent_op && extent_op->is_data);
>>  	ref = kmem_cache_alloc(btrfs_delayed_tree_ref_cachep, GFP_NOFS);
>>  	if (!ref)
> 
> <snip>
> 
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -2031,6 +2031,7 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>>  			 bool for_reloc)
>>  {
>>  	struct btrfs_fs_info *fs_info = root->fs_info;
>> +	struct btrfs_ref generic_ref = { 0 };
>>  	int old_ref_mod, new_ref_mod;
>>  	int ret;
>>  
>> @@ -2040,13 +2041,13 @@ int btrfs_inc_extent_ref(struct btrfs_trans_handle *trans,
>>  	btrfs_ref_tree_mod(root, bytenr, num_bytes, parent, root_objectid,
>>  			   owner, offset, BTRFS_ADD_DELAYED_REF);
>>  
>> +	btrfs_init_generic_ref(&generic_ref, BTRFS_ADD_DELAYED_REF, bytenr,
>> +			       num_bytes, root->root_key.objectid, parent);
>> +	generic_ref.skip_qgroup = for_reloc;
>>  	if (owner < BTRFS_FIRST_FREE_OBJECTID) {
>> -		ret = btrfs_add_delayed_tree_ref(trans, bytenr,
>> -						 num_bytes, parent,
>> -						 root_objectid, (int)owner,
>> -						 for_reloc,
>> -						 BTRFS_ADD_DELAYED_REF, NULL,
>> -						 &old_ref_mod, &new_ref_mod);
>> +		btrfs_init_tree_ref(&generic_ref, (int)owner, root_objectid);
> 
> The cast is now redundant since the parameter definition of
> btrfs_init_tree_ref is an int.

It will be redundant when btrfs_free_extent() is converted to use btrfs_ref.
But not now.

As @owner is still u64, while parameter of btrfs_init_tree_ref() is
still using int for @level.

Thanks,
Qu

>> +		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref,
>> +				NULL, &old_ref_mod, &new_ref_mod);
>>  	} else {
>>  		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
>>  						 num_bytes, parent,
> 
> <snip>
> 
>> @@ -7102,11 +7110,8 @@ int btrfs_free_extent(struct btrfs_trans_handle *trans,
>>  		old_ref_mod = new_ref_mod = 0;
>>  		ret = 0;
>>  	} else if (owner < BTRFS_FIRST_FREE_OBJECTID) {
>> -		ret = btrfs_add_delayed_tree_ref(trans, bytenr,
>> -						 num_bytes, parent,
>> -						 root_objectid, (int)owner,
>> -						 for_reloc,
>> -						 BTRFS_DROP_DELAYED_REF, NULL,
>> +		btrfs_init_tree_ref(&generic_ref, (int)owner, root_objectid);
> 
> ditto about the cast
>> +		ret = btrfs_add_delayed_tree_ref(trans, &generic_ref, NULL,
>>  						 &old_ref_mod, &new_ref_mod);
>>  	} else {
>>  		ret = btrfs_add_delayed_data_ref(trans, root, bytenr,
> 
> <sniop>
> 