Re: [PATCH 0/7] eb reference count cleanups

[Nikolay Borisov <nborisov@suse.com>]

On 6.02.19 г. 16:26 ч., Alex Lyakas wrote:
> Hi Nikolay, David,
> 
> Isn't patch 5 (btrfs: Remove extra reference count bumps in
> btrfs_compare_trees) fixing a memory leak, and hence should be tagged
> as "stable"? I am specifically interested in 4.14.x.
> 
> The comment says "remove redundant calls to extent_buffer_get since
> they don't really add any value". But with the extra ref-count, the
> extent buffer will not be properly freed and will cause a memory leak,
> won't it?

No, take a look into the logic of free_extent_buffer and see there is
special handling for cloned buffers. And in fact, the series this patch
was part of exactly removed this special handling since it's rather
non-intuitive, your email being case in point.

> 
> Thanks,
> Alex.
> 
> 
> 
> On Tue, Nov 6, 2018 at 4:30 PM David Sterba <dsterba@suse.cz> wrote:
>>
>> On Wed, Aug 15, 2018 at 06:26:50PM +0300, Nikolay Borisov wrote:
>>> Here is a series which simplifies the way eb are used in EXTENT_BUFFER_UNMAPPED
>>> context. The end goal was to remove the special "if we have ref count of 2 and
>>> EXTENT_BUFFER_UNMAPPED flag then act as if this is the last ref and free the
>>> buffer" case. To enable this the first 6 patches modify call sites which
>>> needlessly bump the reference count.
>>>
>>> Patch 1 & 2 remove some btree locking when we are operating on unmapped extent
>>> buffers. Each patch's changelog explains why this is safe to do .
>>>
>>> Patch 3,4,5 and 6 remove redundant calls to extent_buffer_get since they don't
>>> really add any value. In all 3 cases having a reference count of 1 is sufficient
>>> for the eb to be freed via btrfs_release_path.
>>>
>>> Patch 7 removes the special handling of EXTENT_BUFFER_UNMAPPED flag in
>>> free_extent_buffer. Also adjust the selftest code to account for this change
>>> by calling one extra time free_extent_buffer. Also document which references
>>> are being dropped. All in all this shouldn't have any functional bearing.
>>>
>>> This was tested with multiple full xfstest runs as well as unloading the btrfs
>>> module after each one to trigger the leak check and ensure no eb's are leaked.
>>> I've also run it through btrfs' selftests multiple times with no problems.
>>>
>>> With this set applied EXTENT_BUFFER_UNMAPPED seems to be relevant only for
>>> selftest which leads me to believe it can be removed altogether. I will
>>> investigate this next but in the meantime this series should be good to go.
>>
>> Besides the 8/7 patch, the rest was in for-next for a long time so I'm
>> merging that to misc-next, targeting 4.21. I'll do one last pass
>> thhrough fstests with the full set and then upddate and push the branch
>> so there might be some delay before it appears in the public repo.
>> Thanks for the cleanup.
>