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

From: Qu Wenruo <quwenruo.btrfs@gmx.com>
To: Josef Bacik <josef@toxicpanda.com>,
	dsterba@suse.cz, linux-btrfs@vger.kernel.org, kernel-team@fb.com
Subject: Re: [PATCH 4/8] btrfs: free the reloc_control in a consistent way
Date: Sat, 14 Mar 2020 08:13:18 +0800	[thread overview]
Message-ID: <e466182b-902b-2cf8-49b2-f506c35b3b48@gmx.com> (raw)
In-Reply-To: <940ee1b7-a854-275e-e01f-05770435ee15@toxicpanda.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 --]