[PATCH 0/7] relocation error handling fixes

[PATCH 0/7] relocation error handling fixes

[Josef Bacik]

My root ref patches have uncovered weird failures in some of our xfstests,
particularly those that do balance while having errors.

I ran relocation through my eio-stress bpf script and loads of things fell out,
these are the fixes required to make the stress test run to completion.

Dave this is just based on my master, I assume it'll apply cleanly to what you
have, but if not let me know which branch you want me to rebase onto to get it
to work right.

Most of these are straightforward, the only tricky/subtle one is 7/7, and I've
added a lot of explanation there around my reasoning.  6/7 is also a little bit
more complicated as it changes the rules slightly for reference holding for
roots.  Before we just sort of hoped and prayed we go the right reference
dropped when we dropped root->reloc_root.  Now we hold one ref for the list of
reloc roots and one ref for root->reloc_root, so it's more clear when we need to
be dropping references.  Everything else is relatively straightforward.  Thanks,

Josef 

[PATCH 1/7] btrfs: drop block from cache on error in relocation

[Josef Bacik]

If we have an error while building the backref tree in relocation we'll
process all the pending edges and then free the node.  This isn't quite
right however as the node could be integrated into the existing cache
partially, linking children within itself into the cache, but not
properly linked into the cache itself.  The fix for this is simple, use
remove_backref_node() instead of free_backref_node(), which will clean
up the cache related to this node completely.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 4fb7e3cc2aca..507361e99316 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
 			free_backref_node(cache, lower);
 		}
 
-		free_backref_node(cache, node);
+		remove_backref_node(cache, node);
 		return ERR_PTR(err);
 	}
 	ASSERT(!node || !node->detached);
-- 
2.24.1

Re: [PATCH 1/7] btrfs: drop block from cache on error in relocation

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> If we have an error while building the backref tree in relocation we'll
> process all the pending edges and then free the node.  This isn't quite
> right however as the node could be integrated into the existing cache
> partially, linking children within itself into the cache, but not
> properly linked into the cache itself.  The fix for this is simple, use
> remove_backref_node() instead of free_backref_node(), which will clean
> up the cache related to this node completely.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  fs/btrfs/relocation.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 4fb7e3cc2aca..507361e99316 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>  			free_backref_node(cache, lower);
>  		}
>  
> -		free_backref_node(cache, node);
> +		remove_backref_node(cache, node);
>  		return ERR_PTR(err);
>  	}
>  	ASSERT(!node || !node->detached);
> 


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

Re: [PATCH 1/7] btrfs: drop block from cache on error in relocation

[David Sterba]

On Mon, Mar 02, 2020 at 01:47:51PM -0500, Josef Bacik wrote:
> If we have an error while building the backref tree in relocation we'll
> process all the pending edges and then free the node.  This isn't quite
> right however as the node could be integrated into the existing cache
> partially, linking children within itself into the cache, but not
> properly linked into the cache itself.

I'm missing description of what's the problem. Something is linked and
then freed, followed by 'fixed by'.

> The fix for this is simple, use
> remove_backref_node() instead of free_backref_node(), which will clean
> up the cache related to this node completely.

So this means that some entries are left in the cache? Leaked memory or
something else?

> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 4fb7e3cc2aca..507361e99316 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>  			free_backref_node(cache, lower);
>  		}
>  
> -		free_backref_node(cache, node);
> +		remove_backref_node(cache, node);
>  		return ERR_PTR(err);
>  	}
>  	ASSERT(!node || !node->detached);

There's a similar pattern in clone_backref_node

1317 fail:
1318         while (!list_empty(&new_node->lower)) {
1319                 new_edge = list_entry(new_node->lower.next,
1320                                       struct backref_edge, list[UPPER]);
1321                 list_del(&new_edge->list[UPPER]);
1322                 free_backref_edge(cache, new_edge);
1323         }
1324         free_backref_node(cache, new_node);

Does this also need to be fixed?

Re: [PATCH 1/7] btrfs: drop block from cache on error in relocation

[Josef Bacik]

On 3/3/20 9:59 AM, David Sterba wrote:
> On Mon, Mar 02, 2020 at 01:47:51PM -0500, Josef Bacik wrote:
>> If we have an error while building the backref tree in relocation we'll
>> process all the pending edges and then free the node.  This isn't quite
>> right however as the node could be integrated into the existing cache
>> partially, linking children within itself into the cache, but not
>> properly linked into the cache itself.
> 
> I'm missing description of what's the problem. Something is linked and
> then freed, followed by 'fixed by'.
> 
>> The fix for this is simple, use
>> remove_backref_node() instead of free_backref_node(), which will clean
>> up the cache related to this node completely.
> 
> So this means that some entries are left in the cache? Leaked memory or
> something else?

Yeah leaked memory and root references, I'll update the changelog to be more clear.

> 
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/relocation.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 4fb7e3cc2aca..507361e99316 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>>   			free_backref_node(cache, lower);
>>   		}
>>   
>> -		free_backref_node(cache, node);
>> +		remove_backref_node(cache, node);
>>   		return ERR_PTR(err);
>>   	}
>>   	ASSERT(!node || !node->detached);
> 
> There's a similar pattern in clone_backref_node
> 
> 1317 fail:
> 1318         while (!list_empty(&new_node->lower)) {
> 1319                 new_edge = list_entry(new_node->lower.next,
> 1320                                       struct backref_edge, list[UPPER]);
> 1321                 list_del(&new_edge->list[UPPER]);
> 1322                 free_backref_edge(cache, new_edge);
> 1323         }
> 1324         free_backref_node(cache, new_node);
> 
> Does this also need to be fixed?
> 

No this is fine, this essentially does what remove_backref_node() does.  The 
build_backref_tree() cleanup just handles the local lists, not the edges 
attached to the node.  clone_backref_node does the cleanup properly.  Thanks,

Josef

[PATCH 2/7] btrfs: unset reloc control if we fail to recover

[Josef Bacik]

If we fail to load an fs root, or fail to start a transaction we can
bail without unsetting the reloc control, which leads to problems later
when we free the reloc control but still have it attached to the file
system.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 507361e99316..173fc7628235 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 		fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
 		if (IS_ERR(fs_root)) {
 			err = PTR_ERR(fs_root);
+			unset_reloc_control(rc);
 			list_add_tail(&reloc_root->root_list, &reloc_roots);
 			goto out_free;
 		}
@@ -4689,8 +4690,10 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 	}
 
 	err = btrfs_commit_transaction(trans);
-	if (err)
+	if (err) {
+		unset_reloc_control(rc);
 		goto out_free;
+	}
 
 	merge_reloc_roots(rc);
 
-- 
2.24.1

Re: [PATCH 2/7] btrfs: unset reloc control if we fail to recover

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> If we fail to load an fs root, or fail to start a transaction we can
> bail without unsetting the reloc control, which leads to problems later
> when we free the reloc control but still have it attached to the file
> system.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 507361e99316..173fc7628235 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  		fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
>  		if (IS_ERR(fs_root)) {
>  			err = PTR_ERR(fs_root);
> +			unset_reloc_control(rc);
>  			list_add_tail(&reloc_root->root_list, &reloc_roots);
>  			goto out_free;
>  		}


Shouldn't the unset_reloc_control() also get called for all related
errors after set_reloc_control()?

That includes btrfs_join_transaction() (the one after
set_reloc_contrl(), which looks missing), the read_fs_root() and the
commit transaction error you're addressing.

Thanks,
Qu

> @@ -4689,8 +4690,10 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  	}
>  
>  	err = btrfs_commit_transaction(trans);
> -	if (err)
> +	if (err) {
> +		unset_reloc_control(rc);
>  		goto out_free;
> +	}
>  
>  	merge_reloc_roots(rc);
>  
> 


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

Re: [PATCH 2/7] btrfs: unset reloc control if we fail to recover

[Josef Bacik]

On 3/2/20 7:58 PM, Qu Wenruo wrote:
> 
> 
> On 2020/3/3 上午2:47, Josef Bacik wrote:
>> If we fail to load an fs root, or fail to start a transaction we can
>> bail without unsetting the reloc control, which leads to problems later
>> when we free the reloc control but still have it attached to the file
>> system.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/relocation.c | 5 ++++-
>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 507361e99316..173fc7628235 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>>   		fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
>>   		if (IS_ERR(fs_root)) {
>>   			err = PTR_ERR(fs_root);
>> +			unset_reloc_control(rc);
>>   			list_add_tail(&reloc_root->root_list, &reloc_roots);
>>   			goto out_free;
>>   		}
> 
> 
> Shouldn't the unset_reloc_control() also get called for all related
> errors after set_reloc_control()?
> 
> That includes btrfs_join_transaction() (the one after
> set_reloc_contrl(), which looks missing), the read_fs_root() and the
> commit transaction error you're addressing.
>

It's already doing unset in the join_transaction right after calling set.  These 
are the only two missing paths.  Thanks,

Josef

Re: [PATCH 2/7] btrfs: unset reloc control if we fail to recover

[Qu Wenruo]

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



On 2020/3/3 上午9:03, Josef Bacik wrote:
> On 3/2/20 7:58 PM, Qu Wenruo wrote:
>>
>>
>> On 2020/3/3 上午2:47, Josef Bacik wrote:
>>> If we fail to load an fs root, or fail to start a transaction we can
>>> bail without unsetting the reloc control, which leads to problems later
>>> when we free the reloc control but still have it attached to the file
>>> system.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>   fs/btrfs/relocation.c | 5 ++++-
>>>   1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>>> index 507361e99316..173fc7628235 100644
>>> --- a/fs/btrfs/relocation.c
>>> +++ b/fs/btrfs/relocation.c
>>> @@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root
>>> *root)
>>>           fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
>>>           if (IS_ERR(fs_root)) {
>>>               err = PTR_ERR(fs_root);
>>> +            unset_reloc_control(rc);
>>>               list_add_tail(&reloc_root->root_list, &reloc_roots);
>>>               goto out_free;
>>>           }
>>
>>
>> Shouldn't the unset_reloc_control() also get called for all related
>> errors after set_reloc_control()?
>>
>> That includes btrfs_join_transaction() (the one after
>> set_reloc_contrl(), which looks missing), the read_fs_root() and the
>> commit transaction error you're addressing.
>>
> 
> It's already doing unset in the join_transaction right after calling
> set.  These are the only two missing paths.  Thanks,
> 
> Josef

My bad.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu


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

Re: [PATCH 2/7] btrfs: unset reloc control if we fail to recover

[David Sterba]

On Tue, Mar 03, 2020 at 08:58:22AM +0800, Qu Wenruo wrote:
> On 2020/3/3 上午2:47, Josef Bacik wrote:
> > If we fail to load an fs root, or fail to start a transaction we can
> > bail without unsetting the reloc control, which leads to problems later
> > when we free the reloc control but still have it attached to the file
> > system.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/relocation.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> > index 507361e99316..173fc7628235 100644
> > --- a/fs/btrfs/relocation.c
> > +++ b/fs/btrfs/relocation.c
> > @@ -4678,6 +4678,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
> >  		fs_root = read_fs_root(fs_info, reloc_root->root_key.offset);
> >  		if (IS_ERR(fs_root)) {
> >  			err = PTR_ERR(fs_root);
> > +			unset_reloc_control(rc);
> >  			list_add_tail(&reloc_root->root_list, &reloc_roots);
> >  			goto out_free;
> >  		}
> 
> 
> Shouldn't the unset_reloc_control() also get called for all related
> errors after set_reloc_control()?

It should and the patch does that but I think it could be merged into
one label so it follows the nesting. The only problem is that the reloc
control is unset between 2 calls of transaction join/commit, so the
unset would have to be called twice:

	set_reloc_control()
	...
	join_transaction()
	if (error)
		goto out_unset;
	...
		read_fs_root()
		if (error)
			goto out_unset;		// added by patch
	...
	commit_transaction()
	if (error)
		goto out_unset;			// added by patch
	...
	merge_reloc_roots();
	unset_reloc_control();			// unconditional
	join_transaction();
	if (error)
		goto out_free;
	commit_transaction();
	clean_dirty_subvols()
out_free_unset:
	unset_reloc_control();			// would be new, duplicated
out_free:
	kfree(rc);


It's fine to call the function twice as it only resets the reloc_ctl
pointer, no other relocation can run at this point, but it does not look
all great. I still think that having the label-based cleanup is better
than to add the cleanup before each goto.

[PATCH 3/7] btrfs: splice rc->reloc_roots onto reloc roots in recover

[Josef Bacik]

If we have an error while processing the reloc roots we could leak roots
that were added to rc->reloc_roots before we hit the error.  Handle this
by splicing rc->reloc_roots onto our local reloc_roots list so they are
properly cleaned up.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index 173fc7628235..f42589cb351c 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4710,6 +4710,9 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 	if (ret < 0 && !err)
 		err = ret;
 out_free:
+	mutex_lock(&fs_info->reloc_mutex);
+	list_splice_init(&rc->reloc_roots, &reloc_roots);
+	mutex_unlock(&fs_info->reloc_mutex);
 	kfree(rc);
 out:
 	if (!list_empty(&reloc_roots))
-- 
2.24.1

Re: [PATCH 3/7] btrfs: splice rc->reloc_roots onto reloc roots in recover

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> If we have an error while processing the reloc roots we could leak roots
> that were added to rc->reloc_roots before we hit the error.  Handle this
> by splicing rc->reloc_roots onto our local reloc_roots list so they are
> properly cleaned up.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/relocation.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index 173fc7628235..f42589cb351c 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4710,6 +4710,9 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  	if (ret < 0 && !err)
>  		err = ret;
>  out_free:
> +	mutex_lock(&fs_info->reloc_mutex);
> +	list_splice_init(&rc->reloc_roots, &reloc_roots);
> +	mutex_unlock(&fs_info->reloc_mutex);
>  	kfree(rc);
>  out:
>  	if (!list_empty(&reloc_roots))
> 


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

[PATCH 4/7] btrfs: run clean_dirty_subvols if we fail to start a trans

[Josef Bacik]

If we do merge_reloc_roots() we could insert a few roots onto the dirty
subvol roots list, where we hold a ref on them.  If we fail to start the
transaction we need to run clean_dirty_subvols() in order to cleanup the
refs.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index f42589cb351c..e60450c44406 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -4275,6 +4275,7 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
 	/* get rid of pinned extents */
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
+		clean_dirty_subvols(rc);
 		err = PTR_ERR(trans);
 		goto out_free;
 	}
@@ -4701,6 +4702,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 
 	trans = btrfs_join_transaction(rc->extent_root);
 	if (IS_ERR(trans)) {
+		clean_dirty_subvols(rc);
 		err = PTR_ERR(trans);
 		goto out_free;
 	}
-- 
2.24.1

Re: [PATCH 4/7] btrfs: run clean_dirty_subvols if we fail to start a trans

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> If we do merge_reloc_roots() we could insert a few roots onto the dirty
> subvol roots list, where we hold a ref on them.  If we fail to start the
> transaction we need to run clean_dirty_subvols() in order to cleanup the
> refs.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu

> ---
>  fs/btrfs/relocation.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index f42589cb351c..e60450c44406 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4275,6 +4275,7 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
>  	/* get rid of pinned extents */
>  	trans = btrfs_join_transaction(rc->extent_root);
>  	if (IS_ERR(trans)) {
> +		clean_dirty_subvols(rc);
>  		err = PTR_ERR(trans);
>  		goto out_free;
>  	}
> @@ -4701,6 +4702,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  
>  	trans = btrfs_join_transaction(rc->extent_root);
>  	if (IS_ERR(trans)) {
> +		clean_dirty_subvols(rc);
>  		err = PTR_ERR(trans);
>  		goto out_free;
>  	}
> 


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

Re: [PATCH 4/7] btrfs: run clean_dirty_subvols if we fail to start a trans

[David Sterba]

On Mon, Mar 02, 2020 at 01:47:54PM -0500, Josef Bacik wrote:
> If we do merge_reloc_roots() we could insert a few roots onto the dirty
> subvol roots list, where we hold a ref on them.  If we fail to start the
> transaction we need to run clean_dirty_subvols() in order to cleanup the
> refs.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index f42589cb351c..e60450c44406 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -4275,6 +4275,7 @@ static noinline_for_stack int relocate_block_group(struct reloc_control *rc)
>  	/* get rid of pinned extents */
>  	trans = btrfs_join_transaction(rc->extent_root);
>  	if (IS_ERR(trans)) {
> +		clean_dirty_subvols(rc);
>  		err = PTR_ERR(trans);
>  		goto out_free;
>  	}
> @@ -4701,6 +4702,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  
>  	trans = btrfs_join_transaction(rc->extent_root);
>  	if (IS_ERR(trans)) {
> +		clean_dirty_subvols(rc);
>  		err = PTR_ERR(trans);
>  		goto out_free;
>  	}

Call to clean_dirty subvolumes is just before the out_free label and
also handles errors so the cleanup should be done there and not before
the gotos.

[PATCH 5/7] btrfs: clear BTRFS_ROOT_DEAD_RELOC_TREE before dropping the reloc root

[Josef Bacik]

We were doing the clear dance for the reloc root after doing the drop of
the reloc root, which means we have a giant window where we could miss
having BTRFS_ROOT_DEAD_RELOC_TREE unset and the reloc_root == NULL.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index e60450c44406..acd21c156378 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2291,18 +2291,19 @@ static int clean_dirty_subvols(struct reloc_control *rc)
 
 			list_del_init(&root->reloc_dirty_list);
 			root->reloc_root = NULL;
-			if (reloc_root) {
-
-				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
-				if (ret2 < 0 && !ret)
-					ret = ret2;
-			}
 			/*
 			 * Need barrier to ensure clear_bit() only happens after
 			 * root->reloc_root = NULL. Pairs with have_reloc_root.
 			 */
 			smp_wmb();
 			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
+
+			if (reloc_root) {
+
+				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
+				if (ret2 < 0 && !ret)
+					ret = ret2;
+			}
 			btrfs_put_root(root);
 		} else {
 			/* Orphan reloc tree, just clean it up */
-- 
2.24.1

Re: [PATCH 5/7] btrfs: clear BTRFS_ROOT_DEAD_RELOC_TREE before dropping the reloc root

[David Sterba]

On Mon, Mar 02, 2020 at 01:47:55PM -0500, Josef Bacik wrote:
> We were doing the clear dance for the reloc root after doing the drop of
> the reloc root, which means we have a giant window where we could miss
> having BTRFS_ROOT_DEAD_RELOC_TREE unset and the reloc_root == NULL.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index e60450c44406..acd21c156378 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2291,18 +2291,19 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>  
>  			list_del_init(&root->reloc_dirty_list);
>  			root->reloc_root = NULL;
> -			if (reloc_root) {
> -
> -				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> -				if (ret2 < 0 && !ret)
> -					ret = ret2;
> -			}
>  			/*
>  			 * Need barrier to ensure clear_bit() only happens after
>  			 * root->reloc_root = NULL. Pairs with have_reloc_root.
>  			 */
>  			smp_wmb();
>  			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> +
> +			if (reloc_root) {
> +
> +				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> +				if (ret2 < 0 && !ret)
> +					ret = ret2;
> +			}

This reverts fix 1fac4a54374f7ef385938f3c6cf7649c0fe4f6cd that moved if
(reloc_root) before the clear_bit.

Re: [PATCH 5/7] btrfs: clear BTRFS_ROOT_DEAD_RELOC_TREE before dropping the reloc root

[Josef Bacik]

On 3/2/20 2:31 PM, David Sterba wrote:
> On Mon, Mar 02, 2020 at 01:47:55PM -0500, Josef Bacik wrote:
>> We were doing the clear dance for the reloc root after doing the drop of
>> the reloc root, which means we have a giant window where we could miss
>> having BTRFS_ROOT_DEAD_RELOC_TREE unset and the reloc_root == NULL.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/relocation.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index e60450c44406..acd21c156378 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -2291,18 +2291,19 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>>   
>>   			list_del_init(&root->reloc_dirty_list);
>>   			root->reloc_root = NULL;
>> -			if (reloc_root) {
>> -
>> -				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
>> -				if (ret2 < 0 && !ret)
>> -					ret = ret2;
>> -			}
>>   			/*
>>   			 * Need barrier to ensure clear_bit() only happens after
>>   			 * root->reloc_root = NULL. Pairs with have_reloc_root.
>>   			 */
>>   			smp_wmb();
>>   			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
>> +
>> +			if (reloc_root) {
>> +
>> +				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
>> +				if (ret2 < 0 && !ret)
>> +					ret = ret2;
>> +			}
> 
> This reverts fix 1fac4a54374f7ef385938f3c6cf7649c0fe4f6cd that moved if
> (reloc_root) before the clear_bit.
> 

Hmm we should probably keep this and move the

if (root->reloc_root)

thing after the

         if (!rc || !rc->create_reloc_tree ||
             root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID)
                 return 0;

to properly fix this.  I'll add this and send an updated series.  Thanks,

Josef

Re: [PATCH 5/7] btrfs: clear BTRFS_ROOT_DEAD_RELOC_TREE before dropping the reloc root

[David Sterba]

On Mon, Mar 02, 2020 at 02:51:11PM -0500, Josef Bacik wrote:
> On 3/2/20 2:31 PM, David Sterba wrote:
> > On Mon, Mar 02, 2020 at 01:47:55PM -0500, Josef Bacik wrote:
> >> We were doing the clear dance for the reloc root after doing the drop of
> >> the reloc root, which means we have a giant window where we could miss
> >> having BTRFS_ROOT_DEAD_RELOC_TREE unset and the reloc_root == NULL.
> >>
> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >> ---
> >>   fs/btrfs/relocation.c | 13 +++++++------
> >>   1 file changed, 7 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> >> index e60450c44406..acd21c156378 100644
> >> --- a/fs/btrfs/relocation.c
> >> +++ b/fs/btrfs/relocation.c
> >> @@ -2291,18 +2291,19 @@ static int clean_dirty_subvols(struct reloc_control *rc)
> >>   
> >>   			list_del_init(&root->reloc_dirty_list);
> >>   			root->reloc_root = NULL;
> >> -			if (reloc_root) {
> >> -
> >> -				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> >> -				if (ret2 < 0 && !ret)
> >> -					ret = ret2;
> >> -			}
> >>   			/*
> >>   			 * Need barrier to ensure clear_bit() only happens after
> >>   			 * root->reloc_root = NULL. Pairs with have_reloc_root.
> >>   			 */
> >>   			smp_wmb();
> >>   			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> >> +
> >> +			if (reloc_root) {
> >> +
> >> +				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> >> +				if (ret2 < 0 && !ret)
> >> +					ret = ret2;
> >> +			}
> > 
> > This reverts fix 1fac4a54374f7ef385938f3c6cf7649c0fe4f6cd that moved if
> > (reloc_root) before the clear_bit.
> > 
> 
> Hmm we should probably keep this and move the
> 
> if (root->reloc_root)
> 
> thing after the
> 
>          if (!rc || !rc->create_reloc_tree ||
>              root->root_key.objectid == BTRFS_TREE_RELOC_OBJECTID)
>                  return 0;
> 
> to properly fix this.  I'll add this and send an updated series.  Thanks,

Also please update the changelog, it's too vague for a code that had
several bugs regarding the reloc_root lifetime.

Re: [PATCH 5/7] btrfs: clear BTRFS_ROOT_DEAD_RELOC_TREE before dropping the reloc root

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> We were doing the clear dance for the reloc root after doing the drop of
> the reloc root, which means we have a giant window where we could miss
> having BTRFS_ROOT_DEAD_RELOC_TREE unset and the reloc_root == NULL.

Sorry I didn't see the problem of having BTRF_ROOT_DEAD_RELOC_TREE and
reloc_root == NULL.

The whole idea of BTRFS_ROOT_DEAD_RELOC_TREE is to avoid accessing
root->reloc_root, no matter if reloc_root is NULL or not.

Or did I miss anything?

Thanks,
Qu

> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/relocation.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index e60450c44406..acd21c156378 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2291,18 +2291,19 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>  
>  			list_del_init(&root->reloc_dirty_list);
>  			root->reloc_root = NULL;
> -			if (reloc_root) {
> -
> -				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> -				if (ret2 < 0 && !ret)
> -					ret = ret2;
> -			}
>  			/*
>  			 * Need barrier to ensure clear_bit() only happens after
>  			 * root->reloc_root = NULL. Pairs with have_reloc_root.
>  			 */
>  			smp_wmb();
>  			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> +
> +			if (reloc_root) {
> +
> +				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> +				if (ret2 < 0 && !ret)
> +					ret = ret2;
> +			}
>  			btrfs_put_root(root);
>  		} else {
>  			/* Orphan reloc tree, just clean it up */
> 


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

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

[Josef Bacik]

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>
---
 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)) {
+		put_ref = true;
+		list_del_init(&root->root_list);
+	}
 	spin_unlock(&fs_info->trans_lock);
+	if (put_ref)
+		btrfs_put_root(root);
 	kfree(node);
 }
 
@@ -1555,7 +1561,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
 
 	ret = __add_reloc_root(reloc_root);
 	BUG_ON(ret < 0);
-	root->reloc_root = reloc_root;
+	root->reloc_root = btrfs_grab_root(reloc_root);
 	return 0;
 }
 
@@ -1576,6 +1582,13 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
 	reloc_root = root->reloc_root;
 	root_item = &reloc_root->root_item;
 
+	/*
+	 * We are probably ok here, but __del_reloc_root() will drop its ref of
+	 * the root.  We have the ref fro root->reloc_root, but just in case
+	 * hold it while we update the reloc root.
+	 */
+	btrfs_grab_root(reloc_root);
+
 	/* root->reloc_root will stay until current relocation finished */
 	if (fs_info->reloc_ctl->merge_reloc_tree &&
 	    btrfs_root_refs(root_item) == 0) {
@@ -1597,7 +1610,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
 	ret = btrfs_update_root(trans, fs_info->tree_root,
 				&reloc_root->root_key, root_item);
 	BUG_ON(ret);
-
+	btrfs_put_root(reloc_root);
 out:
 	return 0;
 }
@@ -2297,19 +2310,28 @@ static int clean_dirty_subvols(struct reloc_control *rc)
 			 */
 			smp_wmb();
 			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
-
 			if (reloc_root) {
-
+				/*
+				 * btrfs_drop_snapshot drops our ref we hold for
+				 * ->reloc_root.  If it fails however we must
+				 * drop the ref ourselves.
+				 */
 				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
-				if (ret2 < 0 && !ret)
-					ret = ret2;
+				if (ret2 < 0) {
+					btrfs_put_root(reloc_root);
+					if (!ret)
+						ret = ret2;
+				}
 			}
 			btrfs_put_root(root);
 		} else {
 			/* Orphan reloc tree, just clean it up */
 			ret2 = btrfs_drop_snapshot(root, NULL, 0, 1);
-			if (ret2 < 0 && !ret)
-				ret = ret2;
+			if (ret2 < 0) {
+				btrfs_put_root(root);
+				if (!ret)
+					ret = ret2;
+			}
 		}
 	}
 	return ret;
@@ -4687,7 +4709,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
 
 		err = __add_reloc_root(reloc_root);
 		BUG_ON(err < 0); /* -ENOMEM or logic error */
-		fs_root->reloc_root = reloc_root;
+		fs_root->reloc_root = btrfs_grab_root(reloc_root);
 		btrfs_put_root(fs_root);
 	}
 
@@ -4912,7 +4934,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
 
 	ret = __add_reloc_root(reloc_root);
 	BUG_ON(ret < 0);
-	new_root->reloc_root = reloc_root;
+	new_root->reloc_root = btrfs_grab_root(reloc_root);
 
 	if (rc->create_reloc_tree)
 		ret = clone_backref_node(trans, rc, root, reloc_root);
-- 
2.24.1

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

[Qu Wenruo]

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



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.

Thanks,
Qu

> +		put_ref = true;
> +		list_del_init(&root->root_list);
> +	}
>  	spin_unlock(&fs_info->trans_lock);
> +	if (put_ref)
> +		btrfs_put_root(root);
>  	kfree(node);
>  }
>  
> @@ -1555,7 +1561,7 @@ int btrfs_init_reloc_root(struct btrfs_trans_handle *trans,
>  
>  	ret = __add_reloc_root(reloc_root);
>  	BUG_ON(ret < 0);
> -	root->reloc_root = reloc_root;
> +	root->reloc_root = btrfs_grab_root(reloc_root);
>  	return 0;
>  }
>  
> @@ -1576,6 +1582,13 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>  	reloc_root = root->reloc_root;
>  	root_item = &reloc_root->root_item;
>  
> +	/*
> +	 * We are probably ok here, but __del_reloc_root() will drop its ref of
> +	 * the root.  We have the ref fro root->reloc_root, but just in case

s/fro/for/

> +	 * hold it while we update the reloc root.
> +	 */
> +	btrfs_grab_root(reloc_root);
> +
>  	/* root->reloc_root will stay until current relocation finished */
>  	if (fs_info->reloc_ctl->merge_reloc_tree &&
>  	    btrfs_root_refs(root_item) == 0) {
> @@ -1597,7 +1610,7 @@ int btrfs_update_reloc_root(struct btrfs_trans_handle *trans,
>  	ret = btrfs_update_root(trans, fs_info->tree_root,
>  				&reloc_root->root_key, root_item);
>  	BUG_ON(ret);
> -
> +	btrfs_put_root(reloc_root);
>  out:
>  	return 0;
>  }
> @@ -2297,19 +2310,28 @@ static int clean_dirty_subvols(struct reloc_control *rc)
>  			 */
>  			smp_wmb();
>  			clear_bit(BTRFS_ROOT_DEAD_RELOC_TREE, &root->state);
> -
>  			if (reloc_root) {
> -
> +				/*
> +				 * btrfs_drop_snapshot drops our ref we hold for
> +				 * ->reloc_root.  If it fails however we must
> +				 * drop the ref ourselves.
> +				 */
>  				ret2 = btrfs_drop_snapshot(reloc_root, NULL, 0, 1);
> -				if (ret2 < 0 && !ret)
> -					ret = ret2;
> +				if (ret2 < 0) {
> +					btrfs_put_root(reloc_root);
> +					if (!ret)
> +						ret = ret2;
> +				}
>  			}
>  			btrfs_put_root(root);
>  		} else {
>  			/* Orphan reloc tree, just clean it up */
>  			ret2 = btrfs_drop_snapshot(root, NULL, 0, 1);
> -			if (ret2 < 0 && !ret)
> -				ret = ret2;
> +			if (ret2 < 0) {
> +				btrfs_put_root(root);
> +				if (!ret)
> +					ret = ret2;
> +			}
>  		}
>  	}
>  	return ret;
> @@ -4687,7 +4709,7 @@ int btrfs_recover_relocation(struct btrfs_root *root)
>  
>  		err = __add_reloc_root(reloc_root);
>  		BUG_ON(err < 0); /* -ENOMEM or logic error */
> -		fs_root->reloc_root = reloc_root;
> +		fs_root->reloc_root = btrfs_grab_root(reloc_root);
>  		btrfs_put_root(fs_root);
>  	}
>  
> @@ -4912,7 +4934,7 @@ int btrfs_reloc_post_snapshot(struct btrfs_trans_handle *trans,
>  
>  	ret = __add_reloc_root(reloc_root);
>  	BUG_ON(ret < 0);
> -	new_root->reloc_root = reloc_root;
> +	new_root->reloc_root = btrfs_grab_root(reloc_root);
>  
>  	if (rc->create_reloc_tree)
>  		ret = clone_backref_node(trans, rc, root, reloc_root);
> 


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

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

[Josef Bacik]

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

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

[David Sterba]

On Mon, Mar 02, 2020 at 01:47:56PM -0500, 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.

I think the functions that add a reference should say that in it's
comment, so it can be easily pairred with btrfs_put_root.

Otherwise yes the explicit references make it easier to see the owner.

[PATCH 7/7] btrfs: remove a BUG_ON() from merge_reloc_roots()

[Josef Bacik]

This was pretty subtle, we default to reloc roots having 0 root refs, so
if we crash in the middle of the relocation they can just be deleted.
If we successfully complete the relocation operations we'll set our root
refs to 1 in prepare_to_merge() and then go on to merge_reloc_roots().

At prepare_to_merge() time if any of the reloc roots have a 0 reference
still, we will remove that reloc root from our reloc root rb tree, and
then clean it up later.

However this only happens if we successfully start a transaction.  If
we've aborted previously we will skip this step completely, and only
have reloc roots with a reference count of 0, but were never properly
removed from the reloc control's rb tree.

This isn't a problem per-se, our references are held by the list the
reloc roots are on, and by the original root the reloc root belongs to.
If we end up in this situation all the reloc roots will be added to the
dirty_reloc_list, and then properly dropped at that point.  The reloc
control will be free'd and the rb tree is no longer used.

There were two options when fixing this, one was to remove the BUG_ON(),
the other was to make prepare_to_merge() handle the case where we
couldn't start a trans handle.

IMO this is the cleaner solution.  I started with handling the error in
prepare_to_merge(), but it turned out super ugly.  And in the end this
BUG_ON() simply doesn't matter, the cleanup was happening properly, we
were just panicing because this BUG_ON() only matters in the success
case.  So I've opted to just remove it and add a comment where it was.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/relocation.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
index c8ff28930677..387b0e7f1372 100644
--- a/fs/btrfs/relocation.c
+++ b/fs/btrfs/relocation.c
@@ -2642,7 +2642,19 @@ void merge_reloc_roots(struct reloc_control *rc)
 			free_reloc_roots(&reloc_roots);
 	}
 
-	BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
+	/*
+	 * We used to have
+	 *
+	 * BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
+	 *
+	 * here, but it's wrong.  If we fail to start the transaction in
+	 * prepare_to_merge() we will have only 0 ref reloc roots, none of which
+	 * have actually been removed from the reloc_root_tree rb tree.  This is
+	 * fine because we're bailing here, and we hold a reference on the root
+	 * for the list that holds it, so these roots will be cleaned up when we
+	 * do the reloc_dirty_list afterwards.  Meanwhile the root->reloc_root
+	 * will be cleaned up on unmount.
+	 */
 }
 
 static void free_block_list(struct rb_root *blocks)
-- 
2.24.1

Re: [PATCH 7/7] btrfs: remove a BUG_ON() from merge_reloc_roots()

[Qu Wenruo]

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



On 2020/3/3 上午2:47, Josef Bacik wrote:
> This was pretty subtle, we default to reloc roots having 0 root refs, so
> if we crash in the middle of the relocation they can just be deleted.
> If we successfully complete the relocation operations we'll set our root
> refs to 1 in prepare_to_merge() and then go on to merge_reloc_roots().
> 
> At prepare_to_merge() time if any of the reloc roots have a 0 reference
> still, we will remove that reloc root from our reloc root rb tree, and
> then clean it up later.
> 
> However this only happens if we successfully start a transaction.  If
> we've aborted previously we will skip this step completely, and only
> have reloc roots with a reference count of 0, but were never properly
> removed from the reloc control's rb tree.

Great, this explains the reason why we're seeing one internal report of
the BUG_ON() get triggered.

> 
> This isn't a problem per-se, our references are held by the list the
> reloc roots are on, and by the original root the reloc root belongs to.
> If we end up in this situation all the reloc roots will be added to the
> dirty_reloc_list, and then properly dropped at that point.  The reloc
> control will be free'd and the rb tree is no longer used.
> 
> There were two options when fixing this, one was to remove the BUG_ON(),
> the other was to make prepare_to_merge() handle the case where we
> couldn't start a trans handle.
> 
> IMO this is the cleaner solution.  I started with handling the error in
> prepare_to_merge(), but it turned out super ugly.  And in the end this
> BUG_ON() simply doesn't matter, the cleanup was happening properly, we
> were just panicing because this BUG_ON() only matters in the success
> case.  So I've opted to just remove it and add a comment where it was.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Since there is a comment added, it looks pretty OK to me.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Thanks,
Qu
> ---
>  fs/btrfs/relocation.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
> index c8ff28930677..387b0e7f1372 100644
> --- a/fs/btrfs/relocation.c
> +++ b/fs/btrfs/relocation.c
> @@ -2642,7 +2642,19 @@ void merge_reloc_roots(struct reloc_control *rc)
>  			free_reloc_roots(&reloc_roots);
>  	}
>  
> -	BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
> +	/*
> +	 * We used to have
> +	 *
> +	 * BUG_ON(!RB_EMPTY_ROOT(&rc->reloc_root_tree.rb_root));
> +	 *
> +	 * here, but it's wrong.  If we fail to start the transaction in
> +	 * prepare_to_merge() we will have only 0 ref reloc roots, none of which
> +	 * have actually been removed from the reloc_root_tree rb tree.  This is
> +	 * fine because we're bailing here, and we hold a reference on the root
> +	 * for the list that holds it, so these roots will be cleaned up when we
> +	 * do the reloc_dirty_list afterwards.  Meanwhile the root->reloc_root
> +	 * will be cleaned up on unmount.
> +	 */
>  }
>  
>  static void free_block_list(struct rb_root *blocks)
> 


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