Re: [bug report] memory leak of xa_node in collapse_file() when rollbacks

["David Hildenbrand (Red Hat)" <david@kernel.org>]

On 12/31/25 07:29, Jinjiang Tu wrote:
> 
> 在 2025/12/31 5:03, David Hildenbrand (Red Hat) 写道:
>> On 12/27/25 02:24, Jinjiang Tu wrote:
>>>
>>> 在 2025/12/25 12:15, Shardul Bankar 写道:
>>>> On Thu, 2025-12-18 at 21:11 +0800, Jinjiang Tu wrote:
>>>>> 在 2025/12/18 20:49, David Hildenbrand (Red Hat) 写道:
>>>>>>     Thanks for checking. I thought that was also discussed as part of
>>>>>> the other fix.
>>>>>>        See [2] where we have
>>>>>>        "Note: This fixes the leak of pre-allocated nodes. A
>>>>>> separate fix
>>>>>> will
>>>>>>     be needed to clean up empty nodes that were inserted into the tree
>>>>>> by
>>>>>>     xas_create_range() but never populated."
>>>>>>        Is that the issue you are describing? (sounds like it, but I
>>>>>> only
>>>>>> skimmed over the details).
>>>>>>        CCing Shardul.
>>>>> Yes, the same issue. As I descirbed in the first email:
>>>>> "
>>>>> At first, I tried to destory the empty nodes when collapse_file()
>>>>> goes to rollback path. However,
>>>>> collapse_file() only holds xarray lock and may release the lock, so
>>>>> we couldn't prevent concurrent
>>>>> call of collapse_file(), so the deleted empty nodes may be needed by
>>>>> other collapse_file() calls.
>>>>> "
>>>> Hi David, Jinjiang,
>>>>
>>>> As Jinjiang mentioned, this appears to address what I had originally
>>>> referred to in the "Note:" in [1].
>>>>
>>>> Just to clarify the context of the "Note:", that was based on my
>>>> assumption at the time that such empty nodes would be considered leaks.
>>>> After Dev’s feedback in [2]:
>>>> "No "fix" is needed in this case, the empty nodes are there in the tree
>>>> and there is no leak."
>>>>
>>>> and looking at the older discussion in [3]:
>>>> "There's nothing to free; if a node is allocated, then it's stored in
>>>> the tree where it can later be found and reused. "
>>>
>>> However, if the empty nodes aren't reused, When the file is deleted,
>>> shmem_evict_inode()->shmem_truncate_range() traverses all entries and
>>> calls xas_store(xas, NULL) to delete, if the leaf xa_node that stores
>>> deleted entry becomes empty, xas_store() will automatically delete the
>>> empty node and delete it's parent is empty too, until parent node isn't
>>> empty. shmem_evict_inode() won't traverse the empty nodes created by
>>> xas_create_range() due to these nodes doesn't store any entries.
>>
>> So you're saying that nothing/nobody would clean up these xarray
>> entries and we'd be leaking them?
> Yes.
>>
>> "struct xarray" documents "If all of the entries in the array are
>> NULL, @xa_head is a NULL pointer.". So we depend on all entries being
>> set to NULL in order to properly cleanup/free the xarray automatically.
>>
> Yes

Okay, then we really have to tackle this. Any takers? :)

-- 
Cheers

David