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

Message ID 20200722160722.8641-2-josef@toxicpanda.com
State New
Headers show
Series
  • [1/2] btrfs: free fs roots on failed mount
Related show

Commit Message

Josef Bacik July 22, 2020, 4:07 p.m. UTC
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(-)

Comments

David Sterba July 23, 2020, 2:20 p.m. UTC | #1
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.
Qu Wenruo July 24, 2020, 12:40 a.m. UTC | #2
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
David Sterba July 27, 2020, 2:03 p.m. UTC | #3
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.
David Sterba July 27, 2020, 2:07 p.m. UTC | #4
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.)
Qu Wenruo July 27, 2020, 11:47 p.m. UTC | #5
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
David Sterba July 28, 2020, 3:09 p.m. UTC | #6
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.

Patch
diff mbox series

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)