Re: [PATCH 6/7] btrfs: hold a ref on the root->reloc_root

[Josef Bacik <josef@toxicpanda.com>]

On 3/2/20 8:12 PM, Qu Wenruo wrote:
> 
> 
> On 2020/3/3 上午2:47, Josef Bacik wrote:
>> We previously were relying on root->reloc_root to be cleaned up by the
>> drop snapshot, or the error handling.  However if btrfs_drop_snapshot()
>> failed it wouldn't drop the ref for the root.  Also we sort of depend on
>> the right thing to happen with moving reloc roots between lists and the
>> fs root they belong to, which makes it hard to figure out who owns the
>> reference.
>>
>> Fix this by explicitly holding a reference on the reloc root for
>> roo->reloc_root.  This means that we hold two references on reloc roots,
>> one for whichever reloc_roots list it's attached to, and the
>> root->reloc_root we're on.
>>
>> This makes it easier to reason out who owns a reference on the root, and
>> when it needs to be dropped.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> 
> A small question inlined below, despite that,
> 
> Reviewed-by: Qu Wenruo <wqu@suse.com>
> 
>> ---
>>   fs/btrfs/relocation.c | 44 ++++++++++++++++++++++++++++++++-----------
>>   1 file changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index acd21c156378..c8ff28930677 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1384,6 +1384,7 @@ static void __del_reloc_root(struct btrfs_root *root)
>>   	struct rb_node *rb_node;
>>   	struct mapping_node *node = NULL;
>>   	struct reloc_control *rc = fs_info->reloc_ctl;
>> +	bool put_ref = false;
>>   
>>   	if (rc && root->node) {
>>   		spin_lock(&rc->reloc_root_tree.lock);
>> @@ -1400,8 +1401,13 @@ static void __del_reloc_root(struct btrfs_root *root)
>>   	}
>>   
>>   	spin_lock(&fs_info->trans_lock);
>> -	list_del_init(&root->root_list);
>> +	if (!list_empty(&root->root_list)) {
> 
> Can we make the ref of reloc root completely free from the list operation?
> It still looks like a compromise between fully ref counted reloc root
> and original non-ref counted one.
> 

No because we have things like prepare_to_merge() which does

list_del_init(&reloc_root->list);
btrfs_update_reloc_root() <- which can call __del_reloc_root
list_add_tail(&reloc_root->list, &rc->reloc_roots);

so we don't want to drop the ref in __del_reloc_root there, as we're still 
technically going to be holding the ref for the list to be cleared later.  Thanks,

Josef