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.
It's not just for the tests but for the tree-mod-log things, so it can't be removed yet. The patchset has been in for-next for a few weaks, no problems reported. Some of the artificial references are obviously redundant (get/free in the same function), the tree-mod-log (get_old_root) span several functions but otherwise this seems safe to remove as well. I'd ask for more asserts and comments, eg. that the unmapped eb must have refcount 1 at the time it's being freed. As it's used in special context, the lifetime can be verified statically and the assert would catch any future violations. Anyway, I'm going to move the patches to misc-next and schedule for 4.20.
