Re: [PATCH 1/7] btrfs: drop block from cache on error in relocation

[Josef Bacik <josef@toxicpanda.com>]

On 3/3/20 9:59 AM, David Sterba wrote:
> On Mon, Mar 02, 2020 at 01:47:51PM -0500, Josef Bacik wrote:
>> If we have an error while building the backref tree in relocation we'll
>> process all the pending edges and then free the node.  This isn't quite
>> right however as the node could be integrated into the existing cache
>> partially, linking children within itself into the cache, but not
>> properly linked into the cache itself.
> 
> I'm missing description of what's the problem. Something is linked and
> then freed, followed by 'fixed by'.
> 
>> The fix for this is simple, use
>> remove_backref_node() instead of free_backref_node(), which will clean
>> up the cache related to this node completely.
> 
> So this means that some entries are left in the cache? Leaked memory or
> something else?

Yeah leaked memory and root references, I'll update the changelog to be more clear.

> 
>> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
>> ---
>>   fs/btrfs/relocation.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/relocation.c b/fs/btrfs/relocation.c
>> index 4fb7e3cc2aca..507361e99316 100644
>> --- a/fs/btrfs/relocation.c
>> +++ b/fs/btrfs/relocation.c
>> @@ -1244,7 +1244,7 @@ struct backref_node *build_backref_tree(struct reloc_control *rc,
>>   			free_backref_node(cache, lower);
>>   		}
>>   
>> -		free_backref_node(cache, node);
>> +		remove_backref_node(cache, node);
>>   		return ERR_PTR(err);
>>   	}
>>   	ASSERT(!node || !node->detached);
> 
> There's a similar pattern in clone_backref_node
> 
> 1317 fail:
> 1318         while (!list_empty(&new_node->lower)) {
> 1319                 new_edge = list_entry(new_node->lower.next,
> 1320                                       struct backref_edge, list[UPPER]);
> 1321                 list_del(&new_edge->list[UPPER]);
> 1322                 free_backref_edge(cache, new_edge);
> 1323         }
> 1324         free_backref_node(cache, new_node);
> 
> Does this also need to be fixed?
> 

No this is fine, this essentially does what remove_backref_node() does.  The 
build_backref_tree() cleanup just handles the local lists, not the edges 
attached to the node.  clone_backref_node does the cleanup properly.  Thanks,

Josef