| Message ID | 20200304161830.2360-5-josef@toxicpanda.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | relocation error handling fixes | expand |
On 2020/3/5 上午12:18, 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..f6237d885fe0 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 rb_node *rb_node; > + struct mapping_node *node; > + > + free_reloc_roots(&rc->reloc_roots); > + while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { rbtree_postorder_for_each_entry_safe(). So that we don't need to bother the re-balance of rbtree. Thanks, Qu > + node = rb_entry(rb_node, struct mapping_node, rb_node); > + rb_erase(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); >
On Thu, Mar 05, 2020 at 07:39:33PM +0800, Qu Wenruo wrote: > > > On 2020/3/5 上午12:18, 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..f6237d885fe0 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 rb_node *rb_node; > > + struct mapping_node *node; > > + > > + free_reloc_roots(&rc->reloc_roots); > > + while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { > > rbtree_postorder_for_each_entry_safe(). > > So that we don't need to bother the re-balance of rbtree. I'll update the patch with this --- a/fs/btrfs/relocation.c +++ b/fs/btrfs/relocation.c @@ -4240,15 +4240,13 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) static void free_reloc_control(struct reloc_control *rc) { - struct rb_node *rb_node; - struct mapping_node *node; + struct mapping_node *node, *tmp; free_reloc_roots(&rc->reloc_roots); - while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { - node = rb_entry(rb_node, struct mapping_node, rb_node); - rb_erase(rb_node, &rc->reloc_root_tree.rb_root); + rbtree_postorder_for_each_entry_safe(node, tmp, + &rc->reloc_root_tree.rb_root, rb_node) kfree(node); - } + kfree(rc); }
On 3/13/20 11:18 AM, David Sterba wrote: > On Thu, Mar 05, 2020 at 07:39:33PM +0800, Qu Wenruo wrote: >> >> >> On 2020/3/5 上午12:18, 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..f6237d885fe0 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 rb_node *rb_node; >>> + struct mapping_node *node; >>> + >>> + free_reloc_roots(&rc->reloc_roots); >>> + while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { >> >> rbtree_postorder_for_each_entry_safe(). >> >> So that we don't need to bother the re-balance of rbtree. > > I'll update the patch with this > > --- a/fs/btrfs/relocation.c > +++ b/fs/btrfs/relocation.c > @@ -4240,15 +4240,13 @@ static struct reloc_control *alloc_reloc_control(struct btrfs_fs_info *fs_info) > > static void free_reloc_control(struct reloc_control *rc) > { > - struct rb_node *rb_node; > - struct mapping_node *node; > + struct mapping_node *node, *tmp; > > free_reloc_roots(&rc->reloc_roots); > - while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { > - node = rb_entry(rb_node, struct mapping_node, rb_node); > - rb_erase(rb_node, &rc->reloc_root_tree.rb_root); > + rbtree_postorder_for_each_entry_safe(node, tmp, > + &rc->reloc_root_tree.rb_root, rb_node) > kfree(node); You need an rb_erase() in here. I'm updating the series so I'll fix it before I send the new set. Thanks, Josef
On 2020/3/13 下午11:32, Josef Bacik wrote: > On 3/13/20 11:18 AM, David Sterba wrote: >> On Thu, Mar 05, 2020 at 07:39:33PM +0800, Qu Wenruo wrote: >>> >>> >>> On 2020/3/5 上午12:18, 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..f6237d885fe0 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 rb_node *rb_node; >>>> + struct mapping_node *node; >>>> + >>>> + free_reloc_roots(&rc->reloc_roots); >>>> + while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { >>> >>> rbtree_postorder_for_each_entry_safe(). >>> >>> So that we don't need to bother the re-balance of rbtree. >> >> I'll update the patch with this >> >> --- a/fs/btrfs/relocation.c >> +++ b/fs/btrfs/relocation.c >> @@ -4240,15 +4240,13 @@ static struct reloc_control >> *alloc_reloc_control(struct btrfs_fs_info *fs_info) >> static void free_reloc_control(struct reloc_control *rc) >> { >> - struct rb_node *rb_node; >> - struct mapping_node *node; >> + struct mapping_node *node, *tmp; >> free_reloc_roots(&rc->reloc_roots); >> - while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { >> - node = rb_entry(rb_node, struct mapping_node, rb_node); >> - rb_erase(rb_node, &rc->reloc_root_tree.rb_root); >> + rbtree_postorder_for_each_entry_safe(node, tmp, >> + &rc->reloc_root_tree.rb_root, rb_node) >> kfree(node); > > You need an rb_erase() in here. I'm updating the series so I'll fix it > before I send the new set. Thanks, Nope, you don't. And that's why we use post order iteration for rbtree. Thanks, Qu > > Josef
diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c index c496f8ed8c7e..f6237d885fe0 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 rb_node *rb_node; + struct mapping_node *node; + + free_reloc_roots(&rc->reloc_roots); + while ((rb_node = rb_first(&rc->reloc_root_tree.rb_root))) { + node = rb_entry(rb_node, struct mapping_node, rb_node); + rb_erase(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(-)