[PATCH 0/7] eb reference count cleanups

[PATCH 0/7] eb reference count cleanups

[Nikolay Borisov]

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.


Nikolay Borisov (7):
  btrfs: Remove needless locking in iterate_inode_refs
  btrfs: Remove needless locking in iterate_inode_extrefs
  btrfs: Remove redundant extent_buffer_get in get_old_root
  btrfs: Remove extraneous extent_buffer_get from tree_mod_log_rewind
  btrfs: Remove extra reference count bumps in btrfs_compare_trees
  btrfs: Remove unnecessary locking code in qgroup_rescan_leaf
  btrfs: Remove special handling of EXTENT_BUFFER_UNMAPPED while freeing

 fs/btrfs/backref.c           |  9 ---------
 fs/btrfs/ctree.c             |  4 ----
 fs/btrfs/extent_io.c         | 11 -----------
 fs/btrfs/qgroup.c            |  7 +------
 fs/btrfs/tests/btrfs-tests.c |  6 +++++-
 5 files changed, 6 insertions(+), 31 deletions(-)

-- 
2.7.4

[PATCH 1/7] btrfs: Remove needless locking in iterate_inode_refs

[Nikolay Borisov]

In iterate_inode_refs the eb is cloned via btrfs_clone_extent_buffer
which creates a private extent buffer with the dummy flag set and
ref count of 1. Then this buffer is locked for reading and its ref
count is incremented by 1. Finally it's fed to the passed iterate_irefs_t
function. The actual iterate call back is inode_to_path (coming from
paths_from_inode) which feeds the eb to btrfs_ref_to_path. In this
final function the passed eb is only read by first assigning it to
the local eb variable. This variable is only modified in the case
another eb was referenced from the passed path that is eb != eb_in
check triggers.

Considering this there is no point in locking the cloned eb in
iterate_inode_refs since it's never being modified and is not published
anywhere. Furthermore the cloned eb is completely fine having its ref
count be 1.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/backref.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index ae750b1574a2..1cb0b31c5059 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2018,9 +2018,6 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 			ret = -ENOMEM;
 			break;
 		}
-		extent_buffer_get(eb);
-		btrfs_tree_read_lock(eb);
-		btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
 		btrfs_release_path(path);
 
 		item = btrfs_item_nr(slot);
@@ -2039,7 +2036,6 @@ static int iterate_inode_refs(u64 inum, struct btrfs_root *fs_root,
 			len = sizeof(*iref) + name_len;
 			iref = (struct btrfs_inode_ref *)((char *)iref + len);
 		}
-		btrfs_tree_read_unlock_blocking(eb);
 		free_extent_buffer(eb);
 	}
 
-- 
2.7.4

[PATCH 2/7] btrfs: Remove needless locking in iterate_inode_extrefs

[Nikolay Borisov]

In iterate_inode_exrefs the eb is cloned via btrfs_clone_extent_buffer
which creates a private extent buffer with the dummy flag set and
ref count of 1. Then this buffer is locked for reading and its ref
count is incremented by 1. Finally it's fed to the passed iterate_irefs_t
function. The actual iterate call back is inode_to_path (coming from
paths_from_inode) which feeds the eb to btrfs_ref_to_path. In this
final function the passed eb is only read by first assigning it to
the local eb variable. This variable is only modified in the case
another eb was referenced from the passed path that is eb != eb_in
check triggers.

Considering this there is no point in locking the cloned eb in
iterate_inode_refs since it's never being modified and is not published
anywhere. Furthermore the cloned eb is completely fine having its ref
count be 1.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/backref.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/btrfs/backref.c b/fs/btrfs/backref.c
index 1cb0b31c5059..99e43f438d90 100644
--- a/fs/btrfs/backref.c
+++ b/fs/btrfs/backref.c
@@ -2076,10 +2076,6 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
 			ret = -ENOMEM;
 			break;
 		}
-		extent_buffer_get(eb);
-
-		btrfs_tree_read_lock(eb);
-		btrfs_set_lock_blocking_rw(eb, BTRFS_READ_LOCK);
 		btrfs_release_path(path);
 
 		item_size = btrfs_item_size_nr(eb, slot);
@@ -2100,7 +2096,6 @@ static int iterate_inode_extrefs(u64 inum, struct btrfs_root *fs_root,
 			cur_offset += btrfs_inode_extref_name_len(eb, extref);
 			cur_offset += sizeof(*extref);
 		}
-		btrfs_tree_read_unlock_blocking(eb);
 		free_extent_buffer(eb);
 
 		offset++;
-- 
2.7.4

[PATCH 3/7] btrfs: Remove redundant extent_buffer_get in get_old_root

[Nikolay Borisov]

get_old_root used used only by btrfs_search_old_slot to initialise the
path structure. The old root is always a cloned buffer (either via
alloc dummy or via btrfs_clone_extent_buffer) and its reference count is
2 - 1 from allocation 1 from extent_buffer_get call in get_old_root.
This latter explicit ref count acquire operation is in fact unnecessary
since the semantic is such that the newly allocated buffer is
handed over to the btrfs_path for lifetime management. Considering this
just remove the extra extent_buffer_get in get_old_root.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index d436fb4c002e..4dc5d4becd49 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1382,7 +1382,6 @@ get_old_root(struct btrfs_root *root, u64 time_seq)
 
 	if (!eb)
 		return NULL;
-	extent_buffer_get(eb);
 	btrfs_tree_read_lock(eb);
 	if (old_root) {
 		btrfs_set_header_bytenr(eb, eb->start);
-- 
2.7.4

[PATCH 4/7] btrfs: Remove extraneous extent_buffer_get from tree_mod_log_rewind

[Nikolay Borisov]

When a rewound buffer is created it already has a ref count of 1 and
the dummy flag set. Then another ref is taken bumping the count to 2.
Finally when this buffer is released from btrfs_release_path the extra
reference is decremented by the special handling code in
free_extent_buffer. However, this special code is in fact redundant
sinca ref count of 1 is still correct since the buffer is only
accessed via btrfs_path struct. This paves the way forward of removing
the special handling in free_extent_buffer.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 4dc5d4becd49..c3d298580bc9 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -1310,7 +1310,6 @@ tree_mod_log_rewind(struct btrfs_fs_info *fs_info, struct btrfs_path *path,
 	btrfs_tree_read_unlock_blocking(eb);
 	free_extent_buffer(eb);
 
-	extent_buffer_get(eb_rewin);
 	btrfs_tree_read_lock(eb_rewin);
 	__tree_mod_log_rewind(fs_info, eb_rewin, time_seq, tm);
 	WARN_ON(btrfs_header_nritems(eb_rewin) >
-- 
2.7.4

[PATCH 5/7] btrfs: Remove extra reference count bumps in btrfs_compare_trees

[Nikolay Borisov]

When the 2 comparison treees roots are initialised they are private to
the function and already have referencecounts of 1 each. There is no
need to further increment the reference count since the cloned buffers
are already accessed via struct btrfs_path. Eventually the 2 paths
used for comparison are going to be released, effectively disposing of
the cloned buffers.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index c3d298580bc9..be0eb26fd4eb 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -5413,7 +5413,6 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 		ret = -ENOMEM;
 		goto out;
 	}
-	extent_buffer_get(left_path->nodes[left_level]);
 
 	right_level = btrfs_header_level(right_root->commit_root);
 	right_root_level = right_level;
@@ -5424,7 +5423,6 @@ int btrfs_compare_trees(struct btrfs_root *left_root,
 		ret = -ENOMEM;
 		goto out;
 	}
-	extent_buffer_get(right_path->nodes[right_level]);
 	up_read(&fs_info->commit_root_sem);
 
 	if (left_level == 0)
-- 
2.7.4

[PATCH 6/7] btrfs: Remove unnecessary locking code in qgroup_rescan_leaf

[Nikolay Borisov]

In qgroup_rescan_leaf a copy is made of the target leaf by calling
btrfs_clone_extent_buffer. The latter allocates a new buffer and
attaches a new set of pages and copies the content of the source
buffer. The new scratch buffer is only used to iterate it's items, it's
not published anywhere and cannot be accessed by a third party. Hence,
it's not necessary to perform any locking on it whatsoever. Furthermore,
remove the extra extent_buffer_get call since the new buffer is always
allocated with a reference count of 1 which is sufficient here.
No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/qgroup.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/btrfs/qgroup.c b/fs/btrfs/qgroup.c
index 4353bb69bb86..4d3efc8f391e 100644
--- a/fs/btrfs/qgroup.c
+++ b/fs/btrfs/qgroup.c
@@ -2689,9 +2689,6 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
 		mutex_unlock(&fs_info->qgroup_rescan_lock);
 		goto out;
 	}
-	extent_buffer_get(scratch_leaf);
-	btrfs_tree_read_lock(scratch_leaf);
-	btrfs_set_lock_blocking_rw(scratch_leaf, BTRFS_READ_LOCK);
 	slot = path->slots[0];
 	btrfs_release_path(path);
 	mutex_unlock(&fs_info->qgroup_rescan_lock);
@@ -2717,10 +2714,8 @@ static int qgroup_rescan_leaf(struct btrfs_trans_handle *trans,
 			goto out;
 	}
 out:
-	if (scratch_leaf) {
-		btrfs_tree_read_unlock_blocking(scratch_leaf);
+	if (scratch_leaf)
 		free_extent_buffer(scratch_leaf);
-	}
 
 	if (done && !ret) {
 		ret = 1;
-- 
2.7.4

[PATCH 7/7] btrfs: Remove special handling of EXTENT_BUFFER_UNMAPPED while freeing

[Nikolay Borisov]

Now that the whole of btrfs code has been audited for eb reference
count management it's time to remove the hunk in free_extent_buffer
that essentially considered the condition "eb->ref == 2 &&
EXTENT_BUFFER_DUMMY" to equal eb->ref = 1. Also remove the last
location which takes an extra reference count in
alloc_test_extent_buffer.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/extent_io.c         | 11 -----------
 fs/btrfs/tests/btrfs-tests.c |  6 +++++-
 2 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 628f1aef34b0..aed4119ffb99 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4908,13 +4908,6 @@ struct extent_buffer *alloc_test_extent_buffer(struct btrfs_fs_info *fs_info,
 	check_buffer_tree_ref(eb);
 	set_bit(EXTENT_BUFFER_IN_TREE, &eb->bflags);
 
-	/*
-	 * We will free dummy extent buffer's if they come into
-	 * free_extent_buffer with a ref count of 2, but if we are using this we
-	 * want the buffers to stay in memory until we're done with them, so
-	 * bump the ref count again.
-	 */
-	atomic_inc(&eb->refs);
 	return eb;
 free_eb:
 	btrfs_release_extent_buffer(eb);
@@ -5105,10 +5098,6 @@ void free_extent_buffer(struct extent_buffer *eb)
 
 	spin_lock(&eb->refs_lock);
 	if (atomic_read(&eb->refs) == 2 &&
-	    test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags))
-		atomic_dec(&eb->refs);
-
-	if (atomic_read(&eb->refs) == 2 &&
 	    test_bit(EXTENT_BUFFER_STALE, &eb->bflags) &&
 	    !extent_buffer_under_io(eb) &&
 	    test_and_clear_bit(EXTENT_BUFFER_TREE_REF, &eb->bflags))
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index db72b3b6209e..4cb8fcfd3ec4 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -174,8 +174,12 @@ void btrfs_free_dummy_root(struct btrfs_root *root)
 	/* Will be freed by btrfs_free_fs_roots */
 	if (WARN_ON(test_bit(BTRFS_ROOT_IN_RADIX, &root->state)))
 		return;
-	if (root->node)
+	if (root->node) {
+		/* One for allocate_extent_buffer */
 		free_extent_buffer(root->node);
+		/* One for get_exent_buffer */
+		free_extent_buffer(root->node);
+	}
 	kfree(root);
 }
 
-- 
2.7.4

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

[David Sterba]

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.

[PATCH] btrfs: Adjust loop in free_extent_buffer

[Nikolay Borisov]

The loop construct in free_extent_buffer was added in
242e18c7c1a8 ("Btrfs: reduce lock contention on extent buffer locks")
as means of reducing the times the eb lock is taken, the non-last ref
count is decremented and lock is released. As the special handling
of UNMAPPED extent buffers was removed now there is only one decrement
op which is happening for EXTENT_BUFFER_UNMAPPED case. This commit
modifies the loop condition so that in case of UNMAPPED buffers the
eb's lock is taken only if we are 100% sure the eb is going to be freed
by the current executor of the code. Additionally, remove superfluous 
ref count  ops in btrfs test. 

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

This was tested with xfstest multiple times + unloading of btrfs module to 
ensure no eb leaks are present. 


 fs/btrfs/extent_io.c         | 4 +++-
 fs/btrfs/tests/btrfs-tests.c | 2 --
 fs/btrfs/tests/inode-tests.c | 6 ------
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 0e9d4f28379d..544a1a5fd416 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5095,7 +5095,9 @@ void free_extent_buffer(struct extent_buffer *eb)
 
 	while (1) {
 		refs = atomic_read(&eb->refs);
-		if (refs <= 3)
+		if ((!test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags) && refs <= 3)
+		    || (test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags) &&
+			refs == 1))
 			break;
 		old = atomic_cmpxchg(&eb->refs, refs, refs - 1);
 		if (old == refs)
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index 4cb8fcfd3ec4..8a59597f1883 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -177,8 +177,6 @@ void btrfs_free_dummy_root(struct btrfs_root *root)
 	if (root->node) {
 		/* One for allocate_extent_buffer */
 		free_extent_buffer(root->node);
-		/* One for get_exent_buffer */
-		free_extent_buffer(root->node);
 	}
 	kfree(root);
 }
diff --git a/fs/btrfs/tests/inode-tests.c b/fs/btrfs/tests/inode-tests.c
index 64043f028820..af0c8e30d9e2 100644
--- a/fs/btrfs/tests/inode-tests.c
+++ b/fs/btrfs/tests/inode-tests.c
@@ -254,11 +254,6 @@ static noinline int test_btrfs_get_extent(u32 sectorsize, u32 nodesize)
 		goto out;
 	}
 
-	/*
-	 * We will just free a dummy node if it's ref count is 2 so we need an
-	 * extra ref so our searches don't accidentally release our page.
-	 */
-	extent_buffer_get(root->node);
 	btrfs_set_header_nritems(root->node, 0);
 	btrfs_set_header_level(root->node, 0);
 	ret = -EINVAL;
@@ -860,7 +855,6 @@ static int test_hole_first(u32 sectorsize, u32 nodesize)
 		goto out;
 	}
 
-	extent_buffer_get(root->node);
 	btrfs_set_header_nritems(root->node, 0);
 	btrfs_set_header_level(root->node, 0);
 	BTRFS_I(inode)->root = root;
-- 
2.7.4

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

[David Sterba]

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.

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

[Alex Lyakas]

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

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

[Nikolay Borisov]

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

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

[Alex Lyakas]

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 <nborisov@suse.com> 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 <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.
> >