| Message ID | 20200313154448.53461-5-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | relocation error handling fixes | expand |
On Fri, Mar 13, 2020 at 11:44:44AM -0400, Josef Bacik wrote: > If we have an error while processing the reloc roots we could leak roots > that were added to rc->reloc_roots before we hit the error. We could > have also not removed the reloct tree mapping from our rb_tree, so clean > up any remaining nodes in the reloc root rb_tree. > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > --- > fs/btrfs/relocation.c | 18 ++++++++++++++++-- > 1 file changed, 16 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > index c496f8ed8c7e..721d049ff2b5 100644 > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4387,6 +4387,20 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) > return rc; > } > > +static void free_reloc_control(struct reloc_control *rc) > +{ > + struct mapping_node *node, *tmp; > + > + free_reloc_roots(&rc->reloc_roots); > + rbtree_postorder_for_each_entry_safe(node, tmp, > + &rc->reloc_root_tree.rb_root, > + rb_node) { > + rb_erase(&node->rb_node, &rc->reloc_root_tree.rb_root); The rb_erase is not needed here, the postorder traversal just goes over all nodes and allows to free the containing structures together with the rb_node. Dangling pointers are not an issue. > + kfree(node); > + } > + kfree(rc); > +}
On Fri, Mar 13, 2020 at 06:18:51PM +0100, David Sterba wrote: > On Fri, Mar 13, 2020 at 11:44:44AM -0400, Josef Bacik wrote: > > If we have an error while processing the reloc roots we could leak roots > > that were added to rc->reloc_roots before we hit the error. We could > > have also not removed the reloct tree mapping from our rb_tree, so clean > > up any remaining nodes in the reloc root rb_tree. > > > > Signed-off-by: Josef Bacik <josef@toxicpanda.com> > > --- > > fs/btrfs/relocation.c | 18 ++++++++++++++++-- > > 1 file changed, 16 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c > > index c496f8ed8c7e..721d049ff2b5 100644 > > --- a/fs/btrfs/relocation.c > > +++ b/fs/btrfs/relocation.c > > @@ -4387,6 +4387,20 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) > > return rc; > > } > > > > +static void free_reloc_control(struct reloc_control *rc) > > +{ > > + struct mapping_node *node, *tmp; > > + > > + free_reloc_roots(&rc->reloc_roots); > > + rbtree_postorder_for_each_entry_safe(node, tmp, > > + &rc->reloc_root_tree.rb_root, > > + rb_node) { > > + rb_erase(&node->rb_node, &rc->reloc_root_tree.rb_root); > > The rb_erase is not needed here, the postorder traversal just goes over > all nodes and allows to free the containing structures together with the > rb_node. Dangling pointers are not an issue. I had not seen your reply when I replied to the v2 patch but if you think the rb_erase is needed, I don't see why.
On 3/13/20 1:38 PM, David Sterba wrote: > On Fri, Mar 13, 2020 at 06:18:51PM +0100, David Sterba wrote: >> On Fri, Mar 13, 2020 at 11:44:44AM -0400, Josef Bacik wrote: >>> If we have an error while processing the reloc roots we could leak roots >>> that were added to rc->reloc_roots before we hit the error. We could >>> have also not removed the reloct tree mapping from our rb_tree, so clean >>> up any remaining nodes in the reloc root rb_tree. >>> >>> Signed-off-by: Josef Bacik <josef@toxicpanda.com> >>> --- >>> fs/btrfs/relocation.c | 18 ++++++++++++++++-- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c >>> index c496f8ed8c7e..721d049ff2b5 100644 >>> --- a/fs/btrfs/relocation.c >>> +++ b/fs/btrfs/relocation.c >>> @@ -4387,6 +4387,20 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) >>> return rc; >>> } >>> >>> +static void free_reloc_control(struct reloc_control *rc) >>> +{ >>> + struct mapping_node *node, *tmp; >>> + >>> + free_reloc_roots(&rc->reloc_roots); >>> + rbtree_postorder_for_each_entry_safe(node, tmp, >>> + &rc->reloc_root_tree.rb_root, >>> + rb_node) { >>> + rb_erase(&node->rb_node, &rc->reloc_root_tree.rb_root); >> >> The rb_erase is not needed here, the postorder traversal just goes over >> all nodes and allows to free the containing structures together with the >> rb_node. Dangling pointers are not an issue. > > I had not seen your reply when I replied to the v2 patch but if you > think the rb_erase is needed, I don't see why. > Because I looked at it and thought it was needed and was confused and had to go look when you replied when you said it wasn't. So it's needed for clarity sake ;). Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index c496f8ed8c7e..721d049ff2b5 100644 --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4387,6 +4387,20 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) return rc; } +static void free_reloc_control(struct reloc_control *rc) +{ + struct mapping_node *node, *tmp; + + free_reloc_roots(&rc->reloc_roots); + rbtree_postorder_for_each_entry_safe(node, tmp, + &rc->reloc_root_tree.rb_root, + rb_node) { + rb_erase(&node->rb_node, &rc->reloc_root_tree.rb_root); + kfree(node); + } + kfree(rc); +} + /* * Print the block group being relocated */ @@ -4531,7 +4545,7 @@ int btrfs_relocate_block_group(struct btrfs_fs_info *fs_info, u64 group_start) btrfs_dec_block_group_ro(rc->block_group); iput(rc->data_inode); btrfs_put_block_group(rc->block_group); - kfree(rc); + free_reloc_control(rc); return err; } @@ -4708,7 +4722,7 @@ int btrfs_recover_relocation(struct btrfs_root *root) out_unset: unset_reloc_control(rc); out_free: - kfree(rc); + free_reloc_control(rc); out: if (!list_empty(&reloc_roots)) free_reloc_roots(&reloc_roots);
If we have an error while processing the reloc roots we could leak roots that were added to rc->reloc_roots before we hit the error. We could have also not removed the reloct tree mapping from our rb_tree, so clean up any remaining nodes in the reloc root rb_tree. Signed-off-by: Josef Bacik <josef@toxicpanda.com> --- fs/btrfs/relocation.c | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-)