| Message ID | 9c6e581e607954968d08179961bb20a62491a655.1597953516.git.josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Some leaked root fixes | expand |
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); > >
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
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 >
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);
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(+)