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? 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.
