[PATCH 1/2] btrfs: free fs roots on failed mount

[PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

While testing a weird problem with -o degraded, I noticed I was getting
leaked root errors

BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
BTRFS error (device loop0): open_ctree failed
BTRFS error (device loop0): leaked root -9-0 refcount 1

This is the DATA_RELOC root, which gets read before the other fs roots,
but is included in the fs roots radix tree, and thus gets freed by
btrfs_free_fs_roots.  Fix this by moving the call into fail_tree_roots:
in open_ctree.  With this fix we no longer leak that root on mount
failure.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c850d7f44fbe..f1fdbdd44c02 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3421,7 +3421,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 fail_trans_kthread:
 	kthread_stop(fs_info->transaction_kthread);
 	btrfs_cleanup_transaction(fs_info);
-	btrfs_free_fs_roots(fs_info);
 fail_cleaner:
 	kthread_stop(fs_info->cleaner_kthread);
 
@@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	btrfs_put_block_group_cache(fs_info);
 
 fail_tree_roots:
+	btrfs_free_fs_roots(fs_info);
 	free_root_pointers(fs_info, true);
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 
-- 
2.24.1

[PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[Josef Bacik]

I'm a actual human being so am incapable of converting u64 to s64 in my
head, use %lld so we can see the negative number in order to identify
which of our special roots we leaked.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index f1fdbdd44c02..cc4081a1c7f9 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1508,7 +1508,7 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
 	while (!list_empty(&fs_info->allocated_roots)) {
 		root = list_first_entry(&fs_info->allocated_roots,
 					struct btrfs_root, leak_list);
-		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
+		btrfs_err(fs_info, "leaked root %lld-%llu refcount %d",
 			  root->root_key.objectid, root->root_key.offset,
 			  refcount_read(&root->refs));
 		while (refcount_read(&root->refs) > 1)
-- 
2.24.1

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[David Sterba]

On Wed, Jul 22, 2020 at 12:07:22PM -0400, Josef Bacik wrote:
> I'm a actual human being so am incapable of converting u64 to s64 in my
> head, use %lld so we can see the negative number in order to identify
> which of our special roots we leaked.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/disk-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index f1fdbdd44c02..cc4081a1c7f9 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1508,7 +1508,7 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>  	while (!list_empty(&fs_info->allocated_roots)) {
>  		root = list_first_entry(&fs_info->allocated_roots,
>  					struct btrfs_root, leak_list);
> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> +		btrfs_err(fs_info, "leaked root %lld-%llu refcount %d",

But this is wrong in another way, roots with high numbers will appear as
negative numbers.

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[Qu Wenruo]

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



On 2020/7/23 下午10:20, David Sterba wrote:
> On Wed, Jul 22, 2020 at 12:07:22PM -0400, Josef Bacik wrote:
>> I'm a actual human being so am incapable of converting u64 to s64 in my
>> head, use %lld so we can see the negative number in order to identify
>> which of our special roots we leaked.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>  fs/btrfs/disk-io.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index f1fdbdd44c02..cc4081a1c7f9 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1508,7 +1508,7 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>>  	while (!list_empty(&fs_info->allocated_roots)) {
>>  		root = list_first_entry(&fs_info->allocated_roots,
>>  					struct btrfs_root, leak_list);
>> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
>> +		btrfs_err(fs_info, "leaked root %lld-%llu refcount %d",
> 
> But this is wrong in another way, roots with high numbers will appear as
> negative numbers.
> 

Nope. We won't have that many roots.

In fact, for subvolumes, the highest id is only 2 ^ 48, an special limit
introduced for qgroup.

So we won't have high enough subvolume ids to be negative, but only
special trees.

Thanks,
Qu


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

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[David Sterba]

On Fri, Jul 24, 2020 at 08:40:17AM +0800, Qu Wenruo wrote:
> 
> 
> On 2020/7/23 下午10:20, David Sterba wrote:
> > On Wed, Jul 22, 2020 at 12:07:22PM -0400, Josef Bacik wrote:
> >> I'm a actual human being so am incapable of converting u64 to s64 in my
> >> head, use %lld so we can see the negative number in order to identify
> >> which of our special roots we leaked.
> >>
> >> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> >> ---
> >>  fs/btrfs/disk-io.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index f1fdbdd44c02..cc4081a1c7f9 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -1508,7 +1508,7 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
> >>  	while (!list_empty(&fs_info->allocated_roots)) {
> >>  		root = list_first_entry(&fs_info->allocated_roots,
> >>  					struct btrfs_root, leak_list);
> >> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> >> +		btrfs_err(fs_info, "leaked root %lld-%llu refcount %d",
> > 
> > But this is wrong in another way, roots with high numbers will appear as
> > negative numbers.
> > 
> 
> Nope. We won't have that many roots.
> 
> In fact, for subvolumes, the highest id is only 2 ^ 48, an special limit
> introduced for qgroup.

It's not a hard limit and certainly can have subvolumes with numbers
that high. That qgoups interpret the qgroup in some way is not a
limitation on subvolumes. We'll have to start reusing the subvolume ids
eventually, with qgroups we can on.

Also the negativer numbers start to appear with 2^32 so that's still
below the percieved limit of 2^48.

> So we won't have high enough subvolume ids to be negative, but only
> special trees.

For the internal trees we eg. have pretty-printer in progs so kernel can
reuse that.

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[David Sterba]

On Mon, Jul 27, 2020 at 04:03:14PM +0200, David Sterba wrote:
> On Fri, Jul 24, 2020 at 08:40:17AM +0800, Qu Wenruo wrote:
> > Nope. We won't have that many roots.
> > 
> > In fact, for subvolumes, the highest id is only 2 ^ 48, an special limit
> > introduced for qgroup.
> 
> It's not a hard limit and certainly can have subvolumes with numbers
> that high. That qgoups interpret the qgroup in some way is not a
> limitation on subvolumes. We'll have to start reusing the subvolume ids
> eventually, with qgroups we can on.

Let me rephrase without the typos:

It's not a hard limit and we certainly can have subvolumes with numbers
that high. That qgroups interpret the qgroup id in some way is not a
limitation on subvolumes. We'll have to start reusing the subvolume ids
eventually, when qgroups are turned on.

(The comment about 2^32/2^48 was incorrect.)

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[David Sterba]

On Wed, Jul 22, 2020 at 12:07:21PM -0400, Josef Bacik wrote:
> While testing a weird problem with -o degraded, I noticed I was getting
> leaked root errors
> 
> BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
> BTRFS error (device loop0): open_ctree failed
> BTRFS error (device loop0): leaked root -9-0 refcount 1
> 
> This is the DATA_RELOC root, which gets read before the other fs roots,
> but is included in the fs roots radix tree, and thus gets freed by
> btrfs_free_fs_roots.  Fix this by moving the call into fail_tree_roots:
> in open_ctree.  With this fix we no longer leak that root on mount
> failure.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/disk-io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c850d7f44fbe..f1fdbdd44c02 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3421,7 +3421,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  fail_trans_kthread:
>  	kthread_stop(fs_info->transaction_kthread);
>  	btrfs_cleanup_transaction(fs_info);
> -	btrfs_free_fs_roots(fs_info);
>  fail_cleaner:
>  	kthread_stop(fs_info->cleaner_kthread);
>  
> @@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  	btrfs_put_block_group_cache(fs_info);
>  
>  fail_tree_roots:
> +	btrfs_free_fs_roots(fs_info);
>  	free_root_pointers(fs_info, true);

The data reloc tree is freed inside free_root_pointers, that it's also
in the radix tree is for convenience so I'd rather fix it inside
free_root_pointers and not reorder btrfs_free_fs_roots.

>  	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
>  
> -- 
> 2.24.1

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

On 7/27/20 10:19 AM, David Sterba wrote:
> On Wed, Jul 22, 2020 at 12:07:21PM -0400, Josef Bacik wrote:
>> While testing a weird problem with -o degraded, I noticed I was getting
>> leaked root errors
>>
>> BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
>> BTRFS error (device loop0): open_ctree failed
>> BTRFS error (device loop0): leaked root -9-0 refcount 1
>>
>> This is the DATA_RELOC root, which gets read before the other fs roots,
>> but is included in the fs roots radix tree, and thus gets freed by
>> btrfs_free_fs_roots.  Fix this by moving the call into fail_tree_roots:
>> in open_ctree.  With this fix we no longer leak that root on mount
>> failure.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/disk-io.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index c850d7f44fbe..f1fdbdd44c02 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3421,7 +3421,6 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>   fail_trans_kthread:
>>   	kthread_stop(fs_info->transaction_kthread);
>>   	btrfs_cleanup_transaction(fs_info);
>> -	btrfs_free_fs_roots(fs_info);
>>   fail_cleaner:
>>   	kthread_stop(fs_info->cleaner_kthread);
>>   
>> @@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>   	btrfs_put_block_group_cache(fs_info);
>>   
>>   fail_tree_roots:
>> +	btrfs_free_fs_roots(fs_info);
>>   	free_root_pointers(fs_info, true);
> 
> The data reloc tree is freed inside free_root_pointers, that it's also
> in the radix tree is for convenience so I'd rather fix it inside
> free_root_pointers and not reorder btrfs_free_fs_roots.
> 

We can't do that, because free_root_pointers() is called to drop the extent 
buffers when we're looping through the backup roots.  btrfs_free_fs_roots() is 
the correct thing to do here, it goes through anything that ended up in the 
radix tree.  Thanks,

Josef

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[David Sterba]

On Mon, Jul 27, 2020 at 10:33:32AM -0400, Josef Bacik wrote:
> >> @@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
> >>   	btrfs_put_block_group_cache(fs_info);
> >>   
> >>   fail_tree_roots:
> >> +	btrfs_free_fs_roots(fs_info);
> >>   	free_root_pointers(fs_info, true);
> > 
> > The data reloc tree is freed inside free_root_pointers, that it's also
> > in the radix tree is for convenience so I'd rather fix it inside
> > free_root_pointers and not reorder btrfs_free_fs_roots.
> 
> We can't do that, because free_root_pointers() is called to drop the extent 
> buffers when we're looping through the backup roots.  btrfs_free_fs_roots() is 
> the correct thing to do here, it goes through anything that ended up in the 
> radix tree.  Thanks,

I see, free_root_pointers is used elsewhere. I'm concerned about the
reordeing because there are several functions that are now called after
the roots are freed.

(before)	btrfs_free_fs_roots(fs_info);

		kthread_stop(fs_info->cleaner_kthread);
		filemap_write_and_wait(fs_info->btree_inode->i_mapping);
		btrfs_sysfs_remove_mounted(fs_info);
		btrfs_sysfs_remove_fsid(fs_info->fs_devices);
		btrfs_put_block_group_cache(fs_info);

(after)		btrfs_free_fs_roots(fs_info);

If something called by btrfs_free_fs_roots depends on structures
removed/cleaned by the other functions, eg. btrfs_put_block_group_cache
ti could be a problem.

I haven't spotted anything so far, the functions are called after
failure still during mount, this is easier than shutdown sequence after
a full mount.

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

On 7/27/20 11:17 AM, David Sterba wrote:
> On Mon, Jul 27, 2020 at 10:33:32AM -0400, Josef Bacik wrote:
>>>> @@ -3441,6 +3440,7 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>>>    	btrfs_put_block_group_cache(fs_info);
>>>>    
>>>>    fail_tree_roots:
>>>> +	btrfs_free_fs_roots(fs_info);
>>>>    	free_root_pointers(fs_info, true);
>>>
>>> The data reloc tree is freed inside free_root_pointers, that it's also
>>> in the radix tree is for convenience so I'd rather fix it inside
>>> free_root_pointers and not reorder btrfs_free_fs_roots.
>>
>> We can't do that, because free_root_pointers() is called to drop the extent
>> buffers when we're looping through the backup roots.  btrfs_free_fs_roots() is
>> the correct thing to do here, it goes through anything that ended up in the
>> radix tree.  Thanks,
> 
> I see, free_root_pointers is used elsewhere. I'm concerned about the
> reordeing because there are several functions that are now called after
> the roots are freed.
> 
> (before)	btrfs_free_fs_roots(fs_info);
> 
> 		kthread_stop(fs_info->cleaner_kthread);
> 		filemap_write_and_wait(fs_info->btree_inode->i_mapping);
> 		btrfs_sysfs_remove_mounted(fs_info);
> 		btrfs_sysfs_remove_fsid(fs_info->fs_devices);
> 		btrfs_put_block_group_cache(fs_info);
> 
> (after)		btrfs_free_fs_roots(fs_info);
> 
> If something called by btrfs_free_fs_roots depends on structures
> removed/cleaned by the other functions, eg. btrfs_put_block_group_cache
> ti could be a problem.
> 
> I haven't spotted anything so far, the functions are called after
> failure still during mount, this is easier than shutdown sequence after
> a full mount.
> 

Yeah I'm always loathe to move this stuff around.  I actually think we can end 
up with the case described in close_ctree if we get an error during log replay 
on mount.  I'll rework this some more.  Thanks,

Josef

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[Qu Wenruo]

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



On 2020/7/27 下午10:03, David Sterba wrote:
> On Fri, Jul 24, 2020 at 08:40:17AM +0800, Qu Wenruo wrote:
>>
>>
>> On 2020/7/23 下午10:20, David Sterba wrote:
>>> On Wed, Jul 22, 2020 at 12:07:22PM -0400, Josef Bacik wrote:
>>>> I'm a actual human being so am incapable of converting u64 to s64 in my
>>>> head, use %lld so we can see the negative number in order to identify
>>>> which of our special roots we leaked.
>>>>
>>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>>> ---
>>>>  fs/btrfs/disk-io.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>>> index f1fdbdd44c02..cc4081a1c7f9 100644
>>>> --- a/fs/btrfs/disk-io.c
>>>> +++ b/fs/btrfs/disk-io.c
>>>> @@ -1508,7 +1508,7 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>>>>  	while (!list_empty(&fs_info->allocated_roots)) {
>>>>  		root = list_first_entry(&fs_info->allocated_roots,
>>>>  					struct btrfs_root, leak_list);
>>>> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
>>>> +		btrfs_err(fs_info, "leaked root %lld-%llu refcount %d",
>>>
>>> But this is wrong in another way, roots with high numbers will appear as
>>> negative numbers.
>>>
>>
>> Nope. We won't have that many roots.
>>
>> In fact, for subvolumes, the highest id is only 2 ^ 48, an special limit
>> introduced for qgroup.
> 
> It's not a hard limit and certainly can have subvolumes with numbers
> that high. That qgoups interpret the qgroup in some way is not a
> limitation on subvolumes. We'll have to start reusing the subvolume ids
> eventually, with qgroups we can on.

Strange...

I still remember that we put that limit as a hard limit for subvolume
creation.
Did we change that behavior in recent releases?

> 
> Also the negativer numbers start to appear with 2^32 so that's still
> below the percieved limit of 2^48.

Nope, For signed 64bits, it's -2^63 ~ 2^63 - 1, not 2 ^ 32.

> 
>> So we won't have high enough subvolume ids to be negative, but only
>> special trees.
> 
> For the internal trees we eg. have pretty-printer in progs so kernel can
> reuse that.
> 

That's true. Pretty tree name is much better for human to read.

Thanks,
Qu


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

Re: [PATCH 2/2] btrfs: fix root leak printk to use %lld instead of %llu

[David Sterba]

On Tue, Jul 28, 2020 at 07:47:00AM +0800, Qu Wenruo wrote:
> I still remember that we put that limit as a hard limit for subvolume
> creation.

Ok, I found it, added by your patch e09fe2d2119800e6 in 2015. It's not
documented anywhere, eg. manual page 5 states the limit is 2^64.

2^48 should be enough for everyone and we can't probably changed it
right now as it's part of API and ABI. Oh well.

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[David Sterba]

On Thu, Sep 03, 2020 at 02:29:50PM -0400, Josef Bacik wrote:
> While testing a weird problem with -o degraded, I noticed I was getting
> leaked root errors
> 
> BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
> BTRFS error (device loop0): open_ctree failed
> BTRFS error (device loop0): leaked root -9-0 refcount 1
> 
> This is the DATA_RELOC root, which gets read before the other fs roots,
> but is included in the fs roots radix tree.  Handle this by adding a
> btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
> is ok to do here if we fail further up because we will only drop the ref
> if we delete the root from the radix tree, and all other cleanup won't
> be duplicated.
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

I'll add this to misc-next as this is the fix and does not need to wait
on the root pretty printing.

[PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

While testing a weird problem with -o degraded, I noticed I was getting
leaked root errors

BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
BTRFS error (device loop0): open_ctree failed
BTRFS error (device loop0): leaked root -9-0 refcount 1

This is the DATA_RELOC root, which gets read before the other fs roots,
but is included in the fs roots radix tree.  Handle this by adding a
btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
is ok to do here if we fail further up because we will only drop the ref
if we delete the root from the radix tree, and all other cleanup won't
be duplicated.

Reviewed-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 0df589c95d86..7147237d9bf0 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3415,6 +3415,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	btrfs_put_block_group_cache(fs_info);
 
 fail_tree_roots:
+	if (fs_info->data_reloc_root)
+		btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);
 	free_root_pointers(fs_info, true);
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 
-- 
2.24.1

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Nikolay Borisov]

On 21.08.20 г. 16:59 ч., Josef Bacik wrote:
> On 8/21/20 3:31 AM, Nikolay Borisov wrote:
>>
>>
>> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
>>> While testing a weird problem with -o degraded, I noticed I was getting
>>> leaked root errors
>>>
>>> BTRFS warning (device loop0): writable mount is not allowed due to
>>> too many missing devices
>>> BTRFS error (device loop0): open_ctree failed
>>> BTRFS error (device loop0): leaked root -9-0 refcount 1
>>>
>>> This is the DATA_RELOC root, which gets read before the other fs roots,
>>> but is included in the fs roots radix tree.  Handle this by adding a
>>> btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
>>> is ok to do here if we fail further up because we will only drop the ref
>>> if we delete the root from the radix tree, and all other cleanup won't
>>> be duplicated.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>   fs/btrfs/disk-io.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index 814f8de395fe..ac6d6fddd5f4 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -3418,6 +3418,8 @@ int __cold open_ctree(struct super_block *sb,
>>> struct btrfs_fs_devices *fs_device
>>>       btrfs_put_block_group_cache(fs_info);
>>>     fail_tree_roots:
>>> +    if (fs_info->data_reloc_root)
>>> +        btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);
>>
>> But will this really free the root? So the newly allocated
>> data_reloc_root has it's ref set to 1 from
>> btrfs_get_root_ref->btrfs_read_tree_root->btrfs_alloc_root and to 2 from
>> being added to the radix tree in btrfs_insert_fs_root().
>>
>> But btrfs_drop_and_free_fs_root makes a single call to btrfs_put_root.
>> So won't the reloc tree be left with a refcount of 1 ?
> 
> It's a global root, so it's final put happens in btrfs_free_fs_info(),
> we just need to drop the radix tree ref here.  Thanks,


Fair enough, but this really shows that btrfs_drop_and_free_fs_root has
a horrible name which doesn't reflect what it does fully...

Any case the patch itself is good, so :

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> 
> Josef
> 

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

On 8/21/20 3:31 AM, Nikolay Borisov wrote:
> 
> 
> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
>> While testing a weird problem with -o degraded, I noticed I was getting
>> leaked root errors
>>
>> BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
>> BTRFS error (device loop0): open_ctree failed
>> BTRFS error (device loop0): leaked root -9-0 refcount 1
>>
>> This is the DATA_RELOC root, which gets read before the other fs roots,
>> but is included in the fs roots radix tree.  Handle this by adding a
>> btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
>> is ok to do here if we fail further up because we will only drop the ref
>> if we delete the root from the radix tree, and all other cleanup won't
>> be duplicated.
>>
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/disk-io.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 814f8de395fe..ac6d6fddd5f4 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -3418,6 +3418,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>>   	btrfs_put_block_group_cache(fs_info);
>>   
>>   fail_tree_roots:
>> +	if (fs_info->data_reloc_root)
>> +		btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);
> 
> But will this really free the root? So the newly allocated
> data_reloc_root has it's ref set to 1 from
> btrfs_get_root_ref->btrfs_read_tree_root->btrfs_alloc_root and to 2 from
> being added to the radix tree in btrfs_insert_fs_root().
> 
> But btrfs_drop_and_free_fs_root makes a single call to btrfs_put_root.
> So won't the reloc tree be left with a refcount of 1 ?

It's a global root, so it's final put happens in btrfs_free_fs_info(), 
we just need to drop the radix tree ref here.  Thanks,

Josef

Re: [PATCH 1/2] btrfs: free fs roots on failed mount

[Nikolay Borisov]

On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
> While testing a weird problem with -o degraded, I noticed I was getting
> leaked root errors
> 
> BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
> BTRFS error (device loop0): open_ctree failed
> BTRFS error (device loop0): leaked root -9-0 refcount 1
> 
> This is the DATA_RELOC root, which gets read before the other fs roots,
> but is included in the fs roots radix tree.  Handle this by adding a
> btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
> is ok to do here if we fail further up because we will only drop the ref
> if we delete the root from the radix tree, and all other cleanup won't
> be duplicated.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/disk-io.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 814f8de395fe..ac6d6fddd5f4 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -3418,6 +3418,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
>  	btrfs_put_block_group_cache(fs_info);
>  
>  fail_tree_roots:
> +	if (fs_info->data_reloc_root)
> +		btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);

But will this really free the root? So the newly allocated
data_reloc_root has it's ref set to 1 from
btrfs_get_root_ref->btrfs_read_tree_root->btrfs_alloc_root and to 2 from
being added to the radix tree in btrfs_insert_fs_root().

But btrfs_drop_and_free_fs_root makes a single call to btrfs_put_root.
So won't the reloc tree be left with a refcount of 1 ?

>  	free_root_pointers(fs_info, true);
>  	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
>  
> 

[PATCH 1/2] btrfs: free fs roots on failed mount

[Josef Bacik]

While testing a weird problem with -o degraded, I noticed I was getting
leaked root errors

BTRFS warning (device loop0): writable mount is not allowed due to too many missing devices
BTRFS error (device loop0): open_ctree failed
BTRFS error (device loop0): leaked root -9-0 refcount 1

This is the DATA_RELOC root, which gets read before the other fs roots,
but is included in the fs roots radix tree.  Handle this by adding a
btrfs_drop_and_free_fs_root() on the data reloc root if it exists.  This
is ok to do here if we fail further up because we will only drop the ref
if we delete the root from the radix tree, and all other cleanup won't
be duplicated.

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

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 814f8de395fe..ac6d6fddd5f4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -3418,6 +3418,8 @@ int __cold open_ctree(struct super_block *sb, struct btrfs_fs_devices *fs_device
 	btrfs_put_block_group_cache(fs_info);
 
 fail_tree_roots:
+	if (fs_info->data_reloc_root)
+		btrfs_drop_and_free_fs_root(fs_info, fs_info->data_reloc_root);
 	free_root_pointers(fs_info, true);
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 
-- 
2.24.1