Re: [PATCH 4/8] btrfs: free the reloc_control in a consistent way

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2630 bytes --]



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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]