[PATCH 0/2][v2] Some leaked root fixes

[PATCH 0/2][v2] Some leaked root fixes

[Josef Bacik]

Hello,

These are two fixes for a leaked root problem I noticed a while ago.  One is the
actual leaked root fix, the other is to add a pretty print for the leaked root
message, as figuring out that some giant %llu number was the data reloc root is
not a feat meant for humans.  Thanks,

Josef

[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

[PATCH 2/2] btrfs: pretty print leaked root name

[Josef Bacik]

I'm a actual human being so am incapable of converting u64 to s64 in my
head, so add a helper to get the pretty name of a root objectid and use
that helper to spit out the name for any special roots for leaked roots,
so I don't have to scratch my head and figure out which root I messed up
the refs for.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/disk-io.c    |  8 +++++---
 fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
 fs/btrfs/print-tree.h |  1 +
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index ac6d6fddd5f4..a7358e0f59de 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
 	struct btrfs_root *root;
 
 	while (!list_empty(&fs_info->allocated_roots)) {
+		const char *name = btrfs_root_name(root->root_key.objectid);
+
 		root = list_first_entry(&fs_info->allocated_roots,
 					struct btrfs_root, leak_list);
-		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
-			  root->root_key.objectid, root->root_key.offset,
-			  refcount_read(&root->refs));
+		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
+			  name ? name : "", root->root_key.objectid,
+			  root->root_key.offset, refcount_read(&root->refs));
 		while (refcount_read(&root->refs) > 1)
 			btrfs_put_root(root);
 		btrfs_put_root(root);
diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
index 61f44e78e3c9..c633aec8973d 100644
--- a/fs/btrfs/print-tree.c
+++ b/fs/btrfs/print-tree.c
@@ -7,6 +7,43 @@
 #include "disk-io.h"
 #include "print-tree.h"
 
+struct name_map {
+	u64 id;
+	const char *name;
+};
+
+static const struct name_map root_map[] = {
+	{ BTRFS_ROOT_TREE_OBJECTID,		"ROOT_TREE"		},
+	{ BTRFS_EXTENT_TREE_OBJECTID,		"EXTENT_TREE"		},
+	{ BTRFS_CHUNK_TREE_OBJECTID,		"CHUNK_TREE"		},
+	{ BTRFS_DEV_TREE_OBJECTID,		"DEV_TREE"		},
+	{ BTRFS_FS_TREE_OBJECTID,		"FS_TREE"		},
+	{ BTRFS_ROOT_TREE_DIR_OBJECTID,		"ROOT_TREE_DIR"		},
+	{ BTRFS_CSUM_TREE_OBJECTID,		"CSUM_TREE"		},
+	{ BTRFS_TREE_LOG_OBJECTID,		"TREE_LOG"		},
+	{ BTRFS_QUOTA_TREE_OBJECTID,		"QUOTA_TREE"		},
+	{ BTRFS_TREE_RELOC_OBJECTID,		"TREE_RELOC"		},
+	{ BTRFS_UUID_TREE_OBJECTID,		"UUID_TREE"		},
+	{ BTRFS_FREE_SPACE_TREE_OBJECTID,	"FREE_SPACE_TREE"	},
+	{ BTRFS_DATA_RELOC_TREE_OBJECTID,	"DATA_RELOC_TREE"	},
+};
+
+const char *btrfs_root_name(u64 objectid)
+{
+	int i;
+
+	if (objectid >= BTRFS_FIRST_FREE_OBJECTID &&
+	    objectid <= BTRFS_LAST_FREE_OBJECTID)
+		return NULL;
+
+	for (i = 0; i < ARRAY_SIZE(root_map); i++) {
+		if (root_map[i].id == objectid)
+			return root_map[i].name;
+	}
+
+	return NULL;
+}
+
 static void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk)
 {
 	int num_stripes = btrfs_chunk_num_stripes(eb, chunk);
diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h
index e6bb38fd75ad..dffdfa495297 100644
--- a/fs/btrfs/print-tree.h
+++ b/fs/btrfs/print-tree.h
@@ -8,5 +8,6 @@
 
 void btrfs_print_leaf(struct extent_buffer *l);
 void btrfs_print_tree(struct extent_buffer *c, bool follow);
+const char *btrfs_root_name(u64 objectid);
 
 #endif
-- 
2.24.1

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

Re: [PATCH 2/2] btrfs: pretty print leaked root name

[Nikolay Borisov]

On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
> I'm a actual human being so am incapable of converting u64 to s64 in my
> head, so add a helper to get the pretty name of a root objectid and use
> that helper to spit out the name for any special roots for leaked roots,
> so I don't have to scratch my head and figure out which root I messed up
> the refs for.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/disk-io.c    |  8 +++++---
>  fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
>  fs/btrfs/print-tree.h |  1 +
>  3 files changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index ac6d6fddd5f4..a7358e0f59de 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>  	struct btrfs_root *root;
>  
>  	while (!list_empty(&fs_info->allocated_roots)) {
> +		const char *name = btrfs_root_name(root->root_key.objectid);
> +
>  		root = list_first_entry(&fs_info->allocated_roots,
>  					struct btrfs_root, leak_list);
> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> -			  root->root_key.objectid, root->root_key.offset,
> -			  refcount_read(&root->refs));
> +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",

nit: Won't this string result in some rather awkward looking strings,
such as:

"leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
(objectid,offset) pair be marked with parentheses?

> +			  name ? name : "", root->root_key.objectid,
> +			  root->root_key.offset, refcount_read(&root->refs));
>  		while (refcount_read(&root->refs) > 1)
>  			btrfs_put_root(root);
>  		btrfs_put_root(root);
> diff --git a/fs/btrfs/print-tree.c b/fs/btrfs/print-tree.c
> index 61f44e78e3c9..c633aec8973d 100644
> --- a/fs/btrfs/print-tree.c
> +++ b/fs/btrfs/print-tree.c
> @@ -7,6 +7,43 @@
>  #include "disk-io.h"
>  #include "print-tree.h"
>  
> +struct name_map {
> +	u64 id;
> +	const char *name;
> +};
> +
> +static const struct name_map root_map[] = {
> +	{ BTRFS_ROOT_TREE_OBJECTID,		"ROOT_TREE"		},
> +	{ BTRFS_EXTENT_TREE_OBJECTID,		"EXTENT_TREE"		},
> +	{ BTRFS_CHUNK_TREE_OBJECTID,		"CHUNK_TREE"		},
> +	{ BTRFS_DEV_TREE_OBJECTID,		"DEV_TREE"		},
> +	{ BTRFS_FS_TREE_OBJECTID,		"FS_TREE"		},
> +	{ BTRFS_ROOT_TREE_DIR_OBJECTID,		"ROOT_TREE_DIR"		},
> +	{ BTRFS_CSUM_TREE_OBJECTID,		"CSUM_TREE"		},
> +	{ BTRFS_TREE_LOG_OBJECTID,		"TREE_LOG"		},
> +	{ BTRFS_QUOTA_TREE_OBJECTID,		"QUOTA_TREE"		},
> +	{ BTRFS_TREE_RELOC_OBJECTID,		"TREE_RELOC"		},
> +	{ BTRFS_UUID_TREE_OBJECTID,		"UUID_TREE"		},
> +	{ BTRFS_FREE_SPACE_TREE_OBJECTID,	"FREE_SPACE_TREE"	},
> +	{ BTRFS_DATA_RELOC_TREE_OBJECTID,	"DATA_RELOC_TREE"	},
> +};
> +
> +const char *btrfs_root_name(u64 objectid)
> +{
> +	int i;
> +
> +	if (objectid >= BTRFS_FIRST_FREE_OBJECTID &&
> +	    objectid <= BTRFS_LAST_FREE_OBJECTID)
> +		return NULL;
> +
> +	for (i = 0; i < ARRAY_SIZE(root_map); i++) {
> +		if (root_map[i].id == objectid)
> +			return root_map[i].name;
> +	}
> +
> +	return NULL;
> +}
> +
>  static void print_chunk(struct extent_buffer *eb, struct btrfs_chunk *chunk)
>  {
>  	int num_stripes = btrfs_chunk_num_stripes(eb, chunk);
> diff --git a/fs/btrfs/print-tree.h b/fs/btrfs/print-tree.h
> index e6bb38fd75ad..dffdfa495297 100644
> --- a/fs/btrfs/print-tree.h
> +++ b/fs/btrfs/print-tree.h
> @@ -8,5 +8,6 @@
>  
>  void btrfs_print_leaf(struct extent_buffer *l);
>  void btrfs_print_tree(struct extent_buffer *c, bool follow);
> +const char *btrfs_root_name(u64 objectid);
>  
>  #endif
> 

Re: [PATCH 2/2] btrfs: pretty print leaked root name

[David Sterba]

On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote:
> 
> 
> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
> > I'm a actual human being so am incapable of converting u64 to s64 in my
> > head, so add a helper to get the pretty name of a root objectid and use
> > that helper to spit out the name for any special roots for leaked roots,
> > so I don't have to scratch my head and figure out which root I messed up
> > the refs for.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/disk-io.c    |  8 +++++---
> >  fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
> >  fs/btrfs/print-tree.h |  1 +
> >  3 files changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index ac6d6fddd5f4..a7358e0f59de 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
> >  	struct btrfs_root *root;
> >  
> >  	while (!list_empty(&fs_info->allocated_roots)) {
> > +		const char *name = btrfs_root_name(root->root_key.objectid);
> > +
> >  		root = list_first_entry(&fs_info->allocated_roots,
> >  					struct btrfs_root, leak_list);
> > -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> > -			  root->root_key.objectid, root->root_key.offset,
> > -			  refcount_read(&root->refs));
> > +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
> 
> nit: Won't this string result in some rather awkward looking strings,
> such as:
> 
> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
> (objectid,offset) pair be marked with parentheses?

I don't understand why need/want to print the offset here. It is from
the key.offset but for a message we should print it in an understandable
way.

Re: [PATCH 2/2] btrfs: pretty print leaked root name

[Nikolay Borisov]

On 21.08.20 г. 13:13 ч., David Sterba wrote:
> On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
>>> I'm a actual human being so am incapable of converting u64 to s64 in my
>>> head, so add a helper to get the pretty name of a root objectid and use
>>> that helper to spit out the name for any special roots for leaked roots,
>>> so I don't have to scratch my head and figure out which root I messed up
>>> the refs for.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>  fs/btrfs/disk-io.c    |  8 +++++---
>>>  fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
>>>  fs/btrfs/print-tree.h |  1 +
>>>  3 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index ac6d6fddd5f4..a7358e0f59de 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>>>  	struct btrfs_root *root;
>>>  
>>>  	while (!list_empty(&fs_info->allocated_roots)) {
>>> +		const char *name = btrfs_root_name(root->root_key.objectid);
>>> +
>>>  		root = list_first_entry(&fs_info->allocated_roots,
>>>  					struct btrfs_root, leak_list);
>>> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
>>> -			  root->root_key.objectid, root->root_key.offset,
>>> -			  refcount_read(&root->refs));
>>> +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
>>
>> nit: Won't this string result in some rather awkward looking strings,
>> such as:
>>
>> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
>> (objectid,offset) pair be marked with parentheses?
> 
> I don't understand why need/want to print the offset here. It is from
> the key.offset but for a message we should print it in an understandable
> way.

According to the dev docs:

btrfs_key.offset = either 0 if objectid is one of the
BTRFS_*_TREE_OBJECTID defines or if a subvolume and not a snapshot, or
if a snapshot the transaction id when this snapshot was created.



So it doesn't have a human-readable value either ways.
> 

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 2/2] btrfs: pretty print leaked root name

[Josef Bacik]

On 8/21/20 6:13 AM, David Sterba wrote:
> On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
>>> I'm a actual human being so am incapable of converting u64 to s64 in my
>>> head, so add a helper to get the pretty name of a root objectid and use
>>> that helper to spit out the name for any special roots for leaked roots,
>>> so I don't have to scratch my head and figure out which root I messed up
>>> the refs for.
>>>
>>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>>> ---
>>>   fs/btrfs/disk-io.c    |  8 +++++---
>>>   fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
>>>   fs/btrfs/print-tree.h |  1 +
>>>   3 files changed, 43 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index ac6d6fddd5f4..a7358e0f59de 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
>>>   	struct btrfs_root *root;
>>>   
>>>   	while (!list_empty(&fs_info->allocated_roots)) {
>>> +		const char *name = btrfs_root_name(root->root_key.objectid);
>>> +
>>>   		root = list_first_entry(&fs_info->allocated_roots,
>>>   					struct btrfs_root, leak_list);
>>> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
>>> -			  root->root_key.objectid, root->root_key.offset,
>>> -			  refcount_read(&root->refs));
>>> +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
>>
>> nit: Won't this string result in some rather awkward looking strings,
>> such as:
>>
>> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
>> (objectid,offset) pair be marked with parentheses?
> 
> I don't understand why need/want to print the offset here. It is from
> the key.offset but for a message we should print it in an understandable
> way.
> 

The offset matters for the TREE_RELOC roots, because it's the object id 
of the fs root that the reloc root refers to.  Thanks,

Josef

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 2/2] btrfs: pretty print leaked root name

[David Sterba]

On Fri, Aug 21, 2020 at 10:00:35AM -0400, Josef Bacik wrote:
> On 8/21/20 6:13 AM, David Sterba wrote:
> > On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote:
> >>>   		root = list_first_entry(&fs_info->allocated_roots,
> >>>   					struct btrfs_root, leak_list);
> >>> -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> >>> -			  root->root_key.objectid, root->root_key.offset,
> >>> -			  refcount_read(&root->refs));
> >>> +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
> >>
> >> nit: Won't this string result in some rather awkward looking strings,
> >> such as:
> >>
> >> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
> >> (objectid,offset) pair be marked with parentheses?
> > 
> > I don't understand why need/want to print the offset here. It is from
> > the key.offset but for a message we should print it in an understandable
> > way.
> 
> The offset matters for the TREE_RELOC roots, because it's the object id 
> of the fs root that the reloc root refers to.  Thanks,

Ok, that makes sense to print, though printing it as offset=... looks
confusing. Could it be rephrased to refer to the fs tree and reloc tree?

Re: [PATCH 2/2] btrfs: pretty print leaked root name

[David Sterba]

On Fri, Aug 21, 2020 at 10:35:38AM +0300, Nikolay Borisov wrote:
> 
> 
> On 20.08.20 г. 23:00 ч., Josef Bacik wrote:
> > I'm a actual human being so am incapable of converting u64 to s64 in my
> > head, so add a helper to get the pretty name of a root objectid and use
> > that helper to spit out the name for any special roots for leaked roots,
> > so I don't have to scratch my head and figure out which root I messed up
> > the refs for.
> > 
> > Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> > ---
> >  fs/btrfs/disk-io.c    |  8 +++++---
> >  fs/btrfs/print-tree.c | 37 +++++++++++++++++++++++++++++++++++++
> >  fs/btrfs/print-tree.h |  1 +
> >  3 files changed, 43 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> > index ac6d6fddd5f4..a7358e0f59de 100644
> > --- a/fs/btrfs/disk-io.c
> > +++ b/fs/btrfs/disk-io.c
> > @@ -1506,11 +1506,13 @@ void btrfs_check_leaked_roots(struct btrfs_fs_info *fs_info)
> >  	struct btrfs_root *root;
> >  
> >  	while (!list_empty(&fs_info->allocated_roots)) {
> > +		const char *name = btrfs_root_name(root->root_key.objectid);
> > +
> >  		root = list_first_entry(&fs_info->allocated_roots,
> >  					struct btrfs_root, leak_list);
> > -		btrfs_err(fs_info, "leaked root %llu-%llu refcount %d",
> > -			  root->root_key.objectid, root->root_key.offset,
> > -			  refcount_read(&root->refs));
> > +		btrfs_err(fs_info, "leaked root %s%lld-%llu refcount %d",
> 
> nit: Won't this string result in some rather awkward looking strings,
> such as:
> 
> "leaked root ROOT_TREE<objectid>-<offset>..." i.e shouldn't the
> (objectid,offset) pair be marked with parentheses?

We need to print either string name or value, and in one go. Unlike
in progs, I'd rather avoid split stings in kernel and using
pr_cont/KERN_CONT.

The option is see is to supply a buffer to the pretty printer that would
be used in case we need the numerical id. Like:

	while (allocated_roots) {
		char root_name[24];

		...
		btrfs_err(..., "leaked root %s ...",
			get_root_name(id, root_name));
	}

where

	char *get_root_name(u64 id, char *buf) {
		if (numeric root) {
			sprintf(buf, "%llu", ud);
			return buf;
		}
		return <root name from the table>;
	}

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

[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 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 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 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

[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