| Message ID | 20200722160722.8641-2-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] btrfs: free fs roots on failed mount | expand |
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.
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
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.
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.)
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
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.
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)
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(-)