Hi Nikolay, In my kernel (4.14.x) the flag is called EXTENT_BUFFER_DUMMY, and indeed I see that there is an extra dec-ref for them in free_extent_buffer().
Thanks for clearing that up, Alex. On Wed, Feb 6, 2019 at 4:36 PM Nikolay Borisov <[email protected]> wrote: > > > > 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 <[email protected]> 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. > >
