[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

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

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