[PATCH 0/8] Locking cleanups and lockdep fix

[PATCH 0/8] Locking cleanups and lockdep fix

[Josef Bacik]

Hello,

Filipe reported a lockdep splat that he hit on btrfs/187, but honestly could be
hit anywhere we do readdir on a sufficiently large fs.  Fixing this is fairly
straightforward, but enabled me to do a lot of extra cleanups, especially now
that my other locking fixes have been merged.  The first two patches are to
address the lockdep problem.  The followup patches are cleaning out the
recursive locking support, which we no longer require.  I would have separated
this work, but btrfs_next_old_leaf was a heavy user of this, so it would be
annoying to take separately, hence putting it all together.  Thanks,

Josef

Josef Bacik (8):
  btrfs: cleanup the locking in btrfs_next_old_leaf
  btrfs: unlock to current level in btrfs_next_old_leaf
  btrfs: kill path->recurse
  btrfs: remove the recursion handling code in locking.c
  btrfs: remove __btrfs_read_lock_root_node
  btrfs: use btrfs_tree_read_lock in btrfs_search_slot
  btrfs: remove the recurse parameter from __btrfs_tree_read_lock
  btrfs: remove ->recursed from extent_buffer

 fs/btrfs/ctree.c     | 47 +++++++++++++----------------
 fs/btrfs/ctree.h     |  1 -
 fs/btrfs/extent_io.c |  1 -
 fs/btrfs/extent_io.h |  1 -
 fs/btrfs/locking.c   | 72 +++-----------------------------------------
 fs/btrfs/locking.h   | 11 ++-----
 6 files changed, 28 insertions(+), 105 deletions(-)

-- 
2.26.2

[PATCH 1/8] btrfs: cleanup the locking in btrfs_next_old_leaf

[Josef Bacik]

We are carrying around this next_rw_lock from when we would do spinning
vs blocking read locks.  Now that we have the rwsem locking we can
simply use the read lock flag unconditionally and the read lock helpers.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index d2d5854d51a7..3a01e6e048c0 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -5327,7 +5327,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 	struct btrfs_key key;
 	u32 nritems;
 	int ret;
-	int next_rw_lock = 0;
 
 	nritems = btrfs_header_nritems(path->nodes[0]);
 	if (nritems == 0)
@@ -5337,7 +5336,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 again:
 	level = 1;
 	next = NULL;
-	next_rw_lock = 0;
 	btrfs_release_path(path);
 
 	path->keep_locks = 1;
@@ -5401,12 +5399,11 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 		}
 
 		if (next) {
-			btrfs_tree_unlock_rw(next, next_rw_lock);
+			btrfs_tree_read_unlock(next);
 			free_extent_buffer(next);
 		}
 
 		next = c;
-		next_rw_lock = path->locks[level];
 		ret = read_block_for_search(root, path, &next, level,
 					    slot, &key);
 		if (ret == -EAGAIN)
@@ -5437,7 +5434,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 						       BTRFS_NESTING_RIGHT,
 						       path->recurse);
 			}
-			next_rw_lock = BTRFS_READ_LOCK;
 		}
 		break;
 	}
@@ -5446,13 +5442,13 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 		level--;
 		c = path->nodes[level];
 		if (path->locks[level])
-			btrfs_tree_unlock_rw(c, path->locks[level]);
+			btrfs_tree_read_unlock(c);
 
 		free_extent_buffer(c);
 		path->nodes[level] = next;
 		path->slots[level] = 0;
 		if (!path->skip_locking)
-			path->locks[level] = next_rw_lock;
+			path->locks[level] = BTRFS_READ_LOCK;
 		if (!level)
 			break;
 
@@ -5466,11 +5462,9 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 			goto done;
 		}
 
-		if (!path->skip_locking) {
+		if (!path->skip_locking)
 			__btrfs_tree_read_lock(next, BTRFS_NESTING_RIGHT,
 					       path->recurse);
-			next_rw_lock = BTRFS_READ_LOCK;
-		}
 	}
 	ret = 0;
 done:
-- 
2.26.2

Re: [PATCH 1/8] btrfs: cleanup the locking in btrfs_next_old_leaf

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> We are carrying around this next_rw_lock from when we would do spinning
> vs blocking read locks.  Now that we have the rwsem locking we can
> simply use the read lock flag unconditionally and the read lock helpers.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/ctree.c | 14 ++++----------
>  1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index d2d5854d51a7..3a01e6e048c0 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -5327,7 +5327,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>         struct btrfs_key key;
>         u32 nritems;
>         int ret;
> -       int next_rw_lock = 0;
>
>         nritems = btrfs_header_nritems(path->nodes[0]);
>         if (nritems == 0)
> @@ -5337,7 +5336,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>  again:
>         level = 1;
>         next = NULL;
> -       next_rw_lock = 0;
>         btrfs_release_path(path);
>
>         path->keep_locks = 1;
> @@ -5401,12 +5399,11 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                 }
>
>                 if (next) {
> -                       btrfs_tree_unlock_rw(next, next_rw_lock);
> +                       btrfs_tree_read_unlock(next);
>                         free_extent_buffer(next);
>                 }
>
>                 next = c;
> -               next_rw_lock = path->locks[level];
>                 ret = read_block_for_search(root, path, &next, level,
>                                             slot, &key);
>                 if (ret == -EAGAIN)
> @@ -5437,7 +5434,6 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                                                        BTRFS_NESTING_RIGHT,
>                                                        path->recurse);
>                         }
> -                       next_rw_lock = BTRFS_READ_LOCK;
>                 }
>                 break;
>         }
> @@ -5446,13 +5442,13 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                 level--;
>                 c = path->nodes[level];
>                 if (path->locks[level])
> -                       btrfs_tree_unlock_rw(c, path->locks[level]);
> +                       btrfs_tree_read_unlock(c);
>
>                 free_extent_buffer(c);
>                 path->nodes[level] = next;
>                 path->slots[level] = 0;
>                 if (!path->skip_locking)
> -                       path->locks[level] = next_rw_lock;
> +                       path->locks[level] = BTRFS_READ_LOCK;
>                 if (!level)
>                         break;
>
> @@ -5466,11 +5462,9 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                         goto done;
>                 }
>
> -               if (!path->skip_locking) {
> +               if (!path->skip_locking)
>                         __btrfs_tree_read_lock(next, BTRFS_NESTING_RIGHT,
>                                                path->recurse);
> -                       next_rw_lock = BTRFS_READ_LOCK;
> -               }
>         }
>         ret = 0;
>  done:
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 2/8] btrfs: unlock to current level in btrfs_next_old_leaf

[Josef Bacik]

Filipe reported the following lockdep splat

======================================================
WARNING: possible circular locking dependency detected
5.10.0-rc2-btrfs-next-71 #1 Not tainted
------------------------------------------------------
find/324157 is trying to acquire lock:
ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

but task is already holding lock:
ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

which lock already depends on the new lock.

the existing dependency chain (in reverse order) is:

-> #1 (btrfs-tree-00){++++}-{3:3}:
       lock_acquire+0xd8/0x490
       down_write_nested+0x44/0x120
       __btrfs_tree_lock+0x27/0x120 [btrfs]
       btrfs_search_slot+0x2a3/0xc50 [btrfs]
       btrfs_insert_empty_items+0x58/0xa0 [btrfs]
       insert_with_overflow+0x44/0x110 [btrfs]
       btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
       btrfs_setxattr+0xd6/0x4c0 [btrfs]
       btrfs_setxattr_trans+0x68/0x100 [btrfs]
       __vfs_setxattr+0x66/0x80
       __vfs_setxattr_noperm+0x70/0x200
       vfs_setxattr+0x6b/0x120
       setxattr+0x125/0x240
       path_setxattr+0xba/0xd0
       __x64_sys_setxattr+0x27/0x30
       do_syscall_64+0x33/0x80
       entry_SYSCALL_64_after_hwframe+0x44/0xa9

-> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
       check_prev_add+0x91/0xc60
       __lock_acquire+0x1689/0x3130
       lock_acquire+0xd8/0x490
       down_read_nested+0x45/0x220
       __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
       btrfs_next_old_leaf+0x27d/0x580 [btrfs]
       btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
       iterate_dir+0x170/0x1c0
       __x64_sys_getdents64+0x83/0x140
       do_syscall_64+0x33/0x80
       entry_SYSCALL_64_after_hwframe+0x44/0xa9

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0                    CPU1
       ----                    ----
  lock(btrfs-tree-00);
                               lock(btrfs-tree-01#2/3);
                               lock(btrfs-tree-00);
  lock(btrfs-tree-01#2/3);

 *** DEADLOCK ***

5 locks held by find/324157:
 #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
 #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
 #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]

stack backtrace:
CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
Call Trace:
 dump_stack+0x8d/0xb5
 check_noncircular+0xff/0x110
 ? mark_lock.part.0+0x468/0xe90
 check_prev_add+0x91/0xc60
 __lock_acquire+0x1689/0x3130
 ? kvm_clock_read+0x14/0x30
 ? kvm_sched_clock_read+0x5/0x10
 lock_acquire+0xd8/0x490
 ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 down_read_nested+0x45/0x220
 ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
 btrfs_next_old_leaf+0x27d/0x580 [btrfs]
 btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
 iterate_dir+0x170/0x1c0
 __x64_sys_getdents64+0x83/0x140
 ? filldir+0x1d0/0x1d0
 do_syscall_64+0x33/0x80
 entry_SYSCALL_64_after_hwframe+0x44/0xa9

This happens because btrfs_next_old_leaf searches down to our current
key, and then walks up the path until we can move to the next slot, and
then reads back down the path so we get the next leaf.

However it doesn't unlock any lower levels until it replaces them with
the new extent buffer.  This is technically fine, but of course causes
lockdep to complain, because we could be holding locks on lower levels
while locking upper levels.

Fix this by dropping all nodes below the level that we use as our new
starting point before we start reading back down the path.  This also
allows us to drop the NESTED magic, because we're no longer locking two
nodes at the same level anymore.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 32 +++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 3a01e6e048c0..dcd17f7167d1 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -5327,6 +5327,7 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 	struct btrfs_key key;
 	u32 nritems;
 	int ret;
+	int i;
 
 	nritems = btrfs_header_nritems(path->nodes[0]);
 	if (nritems == 0)
@@ -5398,9 +5399,19 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 			continue;
 		}
 
-		if (next) {
-			btrfs_tree_read_unlock(next);
-			free_extent_buffer(next);
+
+		/*
+		 * Our current level is where we're going to start from, and to
+		 * make sure lockdep doesn't complain we need to drop our locks
+		 * and nodes from 0 to our current level.
+		 */
+		for (i = 0; i < level; i++) {
+			if (path->locks[level]) {
+				btrfs_tree_read_unlock(path->nodes[i]);
+				path->locks[i] = 0;
+			}
+			free_extent_buffer(path->nodes[i]);
+			path->nodes[i] = NULL;
 		}
 
 		next = c;
@@ -5429,22 +5440,14 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 				cond_resched();
 				goto again;
 			}
-			if (!ret) {
-				__btrfs_tree_read_lock(next,
-						       BTRFS_NESTING_RIGHT,
-						       path->recurse);
-			}
+			if (!ret)
+				btrfs_tree_read_lock(next);
 		}
 		break;
 	}
 	path->slots[level] = slot;
 	while (1) {
 		level--;
-		c = path->nodes[level];
-		if (path->locks[level])
-			btrfs_tree_read_unlock(c);
-
-		free_extent_buffer(c);
 		path->nodes[level] = next;
 		path->slots[level] = 0;
 		if (!path->skip_locking)
@@ -5463,8 +5466,7 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
 		}
 
 		if (!path->skip_locking)
-			__btrfs_tree_read_lock(next, BTRFS_NESTING_RIGHT,
-					       path->recurse);
+			btrfs_tree_read_lock(next);
 	}
 	ret = 0;
 done:
-- 
2.26.2

Re: [PATCH 2/8] btrfs: unlock to current level in btrfs_next_old_leaf

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Filipe reported the following lockdep splat
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 5.10.0-rc2-btrfs-next-71 #1 Not tainted
> ------------------------------------------------------
> find/324157 is trying to acquire lock:
> ffff8ebc48d293a0 (btrfs-tree-01#2/3){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> but task is already holding lock:
> ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> which lock already depends on the new lock.
>
> the existing dependency chain (in reverse order) is:
>
> -> #1 (btrfs-tree-00){++++}-{3:3}:
>        lock_acquire+0xd8/0x490
>        down_write_nested+0x44/0x120
>        __btrfs_tree_lock+0x27/0x120 [btrfs]
>        btrfs_search_slot+0x2a3/0xc50 [btrfs]
>        btrfs_insert_empty_items+0x58/0xa0 [btrfs]
>        insert_with_overflow+0x44/0x110 [btrfs]
>        btrfs_insert_xattr_item+0xb8/0x1d0 [btrfs]
>        btrfs_setxattr+0xd6/0x4c0 [btrfs]
>        btrfs_setxattr_trans+0x68/0x100 [btrfs]
>        __vfs_setxattr+0x66/0x80
>        __vfs_setxattr_noperm+0x70/0x200
>        vfs_setxattr+0x6b/0x120
>        setxattr+0x125/0x240
>        path_setxattr+0xba/0xd0
>        __x64_sys_setxattr+0x27/0x30
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> -> #0 (btrfs-tree-01#2/3){++++}-{3:3}:
>        check_prev_add+0x91/0xc60
>        __lock_acquire+0x1689/0x3130
>        lock_acquire+0xd8/0x490
>        down_read_nested+0x45/0x220
>        __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>        btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>        btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>        iterate_dir+0x170/0x1c0
>        __x64_sys_getdents64+0x83/0x140
>        do_syscall_64+0x33/0x80
>        entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> other info that might help us debug this:
>
>  Possible unsafe locking scenario:
>
>        CPU0                    CPU1
>        ----                    ----
>   lock(btrfs-tree-00);
>                                lock(btrfs-tree-01#2/3);
>                                lock(btrfs-tree-00);
>   lock(btrfs-tree-01#2/3);
>
>  *** DEADLOCK ***
>
> 5 locks held by find/324157:
>  #0: ffff8ebc502c6e00 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x4d/0x60
>  #1: ffff8eb97f689980 (&type->i_mutex_dir_key#10){++++}-{3:3}, at: iterate_dir+0x52/0x1c0
>  #2: ffff8ebaec00ca58 (btrfs-tree-02#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #3: ffff8eb98f986f78 (btrfs-tree-01#2){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  #4: ffff8eb9932c5088 (btrfs-tree-00){++++}-{3:3}, at: __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>
> stack backtrace:
> CPU: 2 PID: 324157 Comm: find Not tainted 5.10.0-rc2-btrfs-next-71 #1
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.13.0-0-gf21b5a4aeb02-prebuilt.qemu.org 04/01/2014
> Call Trace:
>  dump_stack+0x8d/0xb5
>  check_noncircular+0xff/0x110
>  ? mark_lock.part.0+0x468/0xe90
>  check_prev_add+0x91/0xc60
>  __lock_acquire+0x1689/0x3130
>  ? kvm_clock_read+0x14/0x30
>  ? kvm_sched_clock_read+0x5/0x10
>  lock_acquire+0xd8/0x490
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  down_read_nested+0x45/0x220
>  ? __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  __btrfs_tree_read_lock+0x32/0x1a0 [btrfs]
>  btrfs_next_old_leaf+0x27d/0x580 [btrfs]
>  btrfs_real_readdir+0x1e3/0x4b0 [btrfs]
>  iterate_dir+0x170/0x1c0
>  __x64_sys_getdents64+0x83/0x140
>  ? filldir+0x1d0/0x1d0
>  do_syscall_64+0x33/0x80
>  entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> This happens because btrfs_next_old_leaf searches down to our current
> key, and then walks up the path until we can move to the next slot, and
> then reads back down the path so we get the next leaf.
>
> However it doesn't unlock any lower levels until it replaces them with
> the new extent buffer.  This is technically fine, but of course causes
> lockdep to complain, because we could be holding locks on lower levels
> while locking upper levels.
>
> Fix this by dropping all nodes below the level that we use as our new
> starting point before we start reading back down the path.  This also
> allows us to drop the NESTED magic, because we're no longer locking two
> nodes at the same level anymore.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks. It also survived a full weekend of tests and
didn't trigger the splat anymore.

> ---
>  fs/btrfs/ctree.c | 32 +++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index 3a01e6e048c0..dcd17f7167d1 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -5327,6 +5327,7 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>         struct btrfs_key key;
>         u32 nritems;
>         int ret;
> +       int i;
>
>         nritems = btrfs_header_nritems(path->nodes[0]);
>         if (nritems == 0)
> @@ -5398,9 +5399,19 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                         continue;
>                 }
>
> -               if (next) {
> -                       btrfs_tree_read_unlock(next);
> -                       free_extent_buffer(next);
> +
> +               /*
> +                * Our current level is where we're going to start from, and to
> +                * make sure lockdep doesn't complain we need to drop our locks
> +                * and nodes from 0 to our current level.
> +                */
> +               for (i = 0; i < level; i++) {
> +                       if (path->locks[level]) {
> +                               btrfs_tree_read_unlock(path->nodes[i]);
> +                               path->locks[i] = 0;
> +                       }
> +                       free_extent_buffer(path->nodes[i]);
> +                       path->nodes[i] = NULL;
>                 }
>
>                 next = c;
> @@ -5429,22 +5440,14 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                                 cond_resched();
>                                 goto again;
>                         }
> -                       if (!ret) {
> -                               __btrfs_tree_read_lock(next,
> -                                                      BTRFS_NESTING_RIGHT,
> -                                                      path->recurse);
> -                       }
> +                       if (!ret)
> +                               btrfs_tree_read_lock(next);
>                 }
>                 break;
>         }
>         path->slots[level] = slot;
>         while (1) {
>                 level--;
> -               c = path->nodes[level];
> -               if (path->locks[level])
> -                       btrfs_tree_read_unlock(c);
> -
> -               free_extent_buffer(c);
>                 path->nodes[level] = next;
>                 path->slots[level] = 0;
>                 if (!path->skip_locking)
> @@ -5463,8 +5466,7 @@ int btrfs_next_old_leaf(struct btrfs_root *root, struct btrfs_path *path,
>                 }
>
>                 if (!path->skip_locking)
> -                       __btrfs_tree_read_lock(next, BTRFS_NESTING_RIGHT,
> -                                              path->recurse);
> +                       btrfs_tree_read_lock(next);
>         }
>         ret = 0;
>  done:
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 3/8] btrfs: kill path->recurse

[Josef Bacik]

With my async free space cache loading patches we no longer have a user
of path->recurse.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 4 ++--
 fs/btrfs/ctree.h | 1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index dcd17f7167d1..cdd86ced917a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2588,7 +2588,7 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 		 * We don't know the level of the root node until we actually
 		 * have it read locked
 		 */
-		b = __btrfs_read_lock_root_node(root, p->recurse);
+		b = __btrfs_read_lock_root_node(root, 0);
 		level = btrfs_header_level(b);
 		if (level > write_lock_level)
 			goto out;
@@ -2858,7 +2858,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 				p->locks[level] = BTRFS_WRITE_LOCK;
 			} else {
 				__btrfs_tree_read_lock(b, BTRFS_NESTING_NORMAL,
-						       p->recurse);
+						       0);
 				p->locks[level] = BTRFS_READ_LOCK;
 			}
 			p->nodes[level] = b;
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index dc5d36aa4d28..4442e872d873 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -374,7 +374,6 @@ struct btrfs_path {
 	unsigned int search_commit_root:1;
 	unsigned int need_commit_sem:1;
 	unsigned int skip_release_on_error:1;
-	unsigned int recurse:1;
 };
 #define BTRFS_MAX_EXTENT_ITEM_SIZE(r) ((BTRFS_LEAF_DATA_SIZE(r->fs_info) >> 4) - \
 					sizeof(struct btrfs_item))
-- 
2.26.2

Re: [PATCH 3/8] btrfs: kill path->recurse

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> With my async free space cache loading patches we no longer have a user
> of path->recurse.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/ctree.c | 4 ++--
>  fs/btrfs/ctree.h | 1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index dcd17f7167d1..cdd86ced917a 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2588,7 +2588,7 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>                  * We don't know the level of the root node until we actually
>                  * have it read locked
>                  */
> -               b = __btrfs_read_lock_root_node(root, p->recurse);
> +               b = __btrfs_read_lock_root_node(root, 0);
>                 level = btrfs_header_level(b);
>                 if (level > write_lock_level)
>                         goto out;
> @@ -2858,7 +2858,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>                                 p->locks[level] = BTRFS_WRITE_LOCK;
>                         } else {
>                                 __btrfs_tree_read_lock(b, BTRFS_NESTING_NORMAL,
> -                                                      p->recurse);
> +                                                      0);
>                                 p->locks[level] = BTRFS_READ_LOCK;
>                         }
>                         p->nodes[level] = b;
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index dc5d36aa4d28..4442e872d873 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -374,7 +374,6 @@ struct btrfs_path {
>         unsigned int search_commit_root:1;
>         unsigned int need_commit_sem:1;
>         unsigned int skip_release_on_error:1;
> -       unsigned int recurse:1;
>  };
>  #define BTRFS_MAX_EXTENT_ITEM_SIZE(r) ((BTRFS_LEAF_DATA_SIZE(r->fs_info) >> 4) - \
>                                         sizeof(struct btrfs_item))
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[Josef Bacik]

Now that we're no longer using recursion, rip out all of the supporting
code.  Follow up patches will clean up the callers of these functions.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/locking.c | 63 ++--------------------------------------------
 1 file changed, 2 insertions(+), 61 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index d477df1c67db..9b66154803a7 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -28,40 +28,16 @@
  * Additionally we need one level nesting recursion, see below. The rwsem
  * implementation does opportunistic spinning which reduces number of times the
  * locking task needs to sleep.
- *
- *
- * Lock recursion
- * --------------
- *
- * A write operation on a tree might indirectly start a look up on the same
- * tree.  This can happen when btrfs_cow_block locks the tree and needs to
- * lookup free extents.
- *
- * btrfs_cow_block
- *   ..
- *   alloc_tree_block_no_bg_flush
- *     btrfs_alloc_tree_block
- *       btrfs_reserve_extent
- *         ..
- *         load_free_space_cache
- *           ..
- *           btrfs_lookup_file_extent
- *             btrfs_search_slot
- *
  */
 
 /*
  * __btrfs_tree_read_lock: Lock the extent buffer for read.
  * @eb:  the eb to be locked
  * @nest: the nesting level to be used for lockdep
- * @recurse: if this lock is able to be recursed
+ * @recurse: unused.
  *
  * This takes the read lock on the extent buffer, using the specified nesting
  * level for lockdep purposes.
- *
- * If you specify recurse = true, then we will allow this to be taken if we
- * currently own the lock already.  This should only be used in specific
- * usecases, and the subsequent unlock will not change the state of the lock.
  */
 void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
 			    bool recurse)
@@ -71,31 +47,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
 	if (trace_btrfs_tree_read_lock_enabled())
 		start_ns = ktime_get_ns();
 
-	if (unlikely(recurse)) {
-		/* First see if we can grab the lock outright */
-		if (down_read_trylock(&eb->lock))
-			goto out;
-
-		/*
-		 * Ok still doesn't necessarily mean we are already holding the
-		 * lock, check the owner.
-		 */
-		if (eb->lock_owner != current->pid) {
-			down_read_nested(&eb->lock, nest);
-			goto out;
-		}
-
-		/*
-		 * Ok we have actually recursed, but we should only be recursing
-		 * once, so blow up if we're already recursed, otherwise set
-		 * ->lock_recursed and carry on.
-		 */
-		BUG_ON(eb->lock_recursed);
-		eb->lock_recursed = true;
-		goto out;
-	}
 	down_read_nested(&eb->lock, nest);
-out:
 	eb->lock_owner = current->pid;
 	trace_btrfs_tree_read_lock(eb, start_ns);
 }
@@ -136,22 +88,11 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
 }
 
 /*
- * Release read lock.  If the read lock was recursed then the lock stays in the
- * original state that it was before it was recursively locked.
+ * Release read lock.
  */
 void btrfs_tree_read_unlock(struct extent_buffer *eb)
 {
 	trace_btrfs_tree_read_unlock(eb);
-	/*
-	 * if we're nested, we have the write lock.  No new locking
-	 * is needed as long as we are the lock owner.
-	 * The write unlock will do a barrier for us, and the lock_recursed
-	 * field only matters to the lock owner.
-	 */
-	if (eb->lock_recursed && current->pid == eb->lock_owner) {
-		eb->lock_recursed = false;
-		return;
-	}
 	eb->lock_owner = 0;
 	up_read(&eb->lock);
 }
-- 
2.26.2

Re: [PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:31 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> Now that we're no longer using recursion, rip out all of the supporting
> code.  Follow up patches will clean up the callers of these functions.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/locking.c | 63 ++--------------------------------------------
>  1 file changed, 2 insertions(+), 61 deletions(-)
>
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index d477df1c67db..9b66154803a7 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -28,40 +28,16 @@
>   * Additionally we need one level nesting recursion, see below. The rwsem
>   * implementation does opportunistic spinning which reduces number of times the
>   * locking task needs to sleep.
> - *
> - *
> - * Lock recursion
> - * --------------
> - *
> - * A write operation on a tree might indirectly start a look up on the same
> - * tree.  This can happen when btrfs_cow_block locks the tree and needs to
> - * lookup free extents.
> - *
> - * btrfs_cow_block
> - *   ..
> - *   alloc_tree_block_no_bg_flush
> - *     btrfs_alloc_tree_block
> - *       btrfs_reserve_extent
> - *         ..
> - *         load_free_space_cache
> - *           ..
> - *           btrfs_lookup_file_extent
> - *             btrfs_search_slot
> - *
>   */
>
>  /*
>   * __btrfs_tree_read_lock: Lock the extent buffer for read.
>   * @eb:  the eb to be locked
>   * @nest: the nesting level to be used for lockdep
> - * @recurse: if this lock is able to be recursed
> + * @recurse: unused.
>   *
>   * This takes the read lock on the extent buffer, using the specified nesting
>   * level for lockdep purposes.
> - *
> - * If you specify recurse = true, then we will allow this to be taken if we
> - * currently own the lock already.  This should only be used in specific
> - * usecases, and the subsequent unlock will not change the state of the lock.
>   */
>  void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
>                             bool recurse)
> @@ -71,31 +47,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
>         if (trace_btrfs_tree_read_lock_enabled())
>                 start_ns = ktime_get_ns();
>
> -       if (unlikely(recurse)) {
> -               /* First see if we can grab the lock outright */
> -               if (down_read_trylock(&eb->lock))
> -                       goto out;
> -
> -               /*
> -                * Ok still doesn't necessarily mean we are already holding the
> -                * lock, check the owner.
> -                */
> -               if (eb->lock_owner != current->pid) {
> -                       down_read_nested(&eb->lock, nest);
> -                       goto out;
> -               }
> -
> -               /*
> -                * Ok we have actually recursed, but we should only be recursing
> -                * once, so blow up if we're already recursed, otherwise set
> -                * ->lock_recursed and carry on.
> -                */
> -               BUG_ON(eb->lock_recursed);
> -               eb->lock_recursed = true;
> -               goto out;
> -       }
>         down_read_nested(&eb->lock, nest);
> -out:
>         eb->lock_owner = current->pid;
>         trace_btrfs_tree_read_lock(eb, start_ns);
>  }
> @@ -136,22 +88,11 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
>  }
>
>  /*
> - * Release read lock.  If the read lock was recursed then the lock stays in the
> - * original state that it was before it was recursively locked.
> + * Release read lock.
>   */
>  void btrfs_tree_read_unlock(struct extent_buffer *eb)
>  {
>         trace_btrfs_tree_read_unlock(eb);
> -       /*
> -        * if we're nested, we have the write lock.  No new locking
> -        * is needed as long as we are the lock owner.
> -        * The write unlock will do a barrier for us, and the lock_recursed
> -        * field only matters to the lock owner.
> -        */
> -       if (eb->lock_recursed && current->pid == eb->lock_owner) {
> -               eb->lock_recursed = false;
> -               return;
> -       }
>         eb->lock_owner = 0;
>         up_read(&eb->lock);
>  }
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[David Sterba]

On Fri, Nov 06, 2020 at 04:27:32PM -0500, Josef Bacik wrote:
> Now that we're no longer using recursion, rip out all of the supporting
> code.  Follow up patches will clean up the callers of these functions.
> 
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
>  fs/btrfs/locking.c | 63 ++--------------------------------------------
>  1 file changed, 2 insertions(+), 61 deletions(-)
> 
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index d477df1c67db..9b66154803a7 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -28,40 +28,16 @@
>   * Additionally we need one level nesting recursion, see below. The rwsem
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

That needs to be deleted too, the other sentence about spinning seems to
be still useful so I'll keep it ther.

>   * implementation does opportunistic spinning which reduces number of times the
>   * locking task needs to sleep.

Re: [PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[David Sterba]

On Fri, Nov 06, 2020 at 04:27:32PM -0500, Josef Bacik wrote:
> @@ -71,31 +47,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
>  	if (trace_btrfs_tree_read_lock_enabled())
>  		start_ns = ktime_get_ns();
>  
> -	if (unlikely(recurse)) {
> -		/* First see if we can grab the lock outright */
> -		if (down_read_trylock(&eb->lock))
> -			goto out;
> -
> -		/*
> -		 * Ok still doesn't necessarily mean we are already holding the
> -		 * lock, check the owner.
> -		 */
> -		if (eb->lock_owner != current->pid) {

This

> -			down_read_nested(&eb->lock, nest);
> -			goto out;
> -		}
> -
> -		/*
> -		 * Ok we have actually recursed, but we should only be recursing
> -		 * once, so blow up if we're already recursed, otherwise set
> -		 * ->lock_recursed and carry on.
> -		 */
> -		BUG_ON(eb->lock_recursed);
> -		eb->lock_recursed = true;
> -		goto out;
> -	}
>  	down_read_nested(&eb->lock, nest);
> -out:
>  	eb->lock_owner = current->pid;
>  	trace_btrfs_tree_read_lock(eb, start_ns);
>  }
> @@ -136,22 +88,11 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
>  }
>  
>  /*
> - * Release read lock.  If the read lock was recursed then the lock stays in the
> - * original state that it was before it was recursively locked.
> + * Release read lock.
>   */
>  void btrfs_tree_read_unlock(struct extent_buffer *eb)
>  {
>  	trace_btrfs_tree_read_unlock(eb);
> -	/*
> -	 * if we're nested, we have the write lock.  No new locking
> -	 * is needed as long as we are the lock owner.
> -	 * The write unlock will do a barrier for us, and the lock_recursed
> -	 * field only matters to the lock owner.
> -	 */
> -	if (eb->lock_recursed && current->pid == eb->lock_owner) {

And this were the last uses of the lock_owner inside locks, so when the
recursion is gone, the remainig use:

btrfs_init_new_buffer:

4624         /*
4625          * Extra safety check in case the extent tree is corrupted and extent
4626          * allocator chooses to use a tree block which is already used and
4627          * locked.
4628          */
4629         if (buf->lock_owner == current->pid) {
4630                 btrfs_err_rl(fs_info,
4631 "tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
4632                         buf->start, btrfs_header_owner(buf), current->pid);
4633                 free_extent_buffer(buf);
4634                 return ERR_PTR(-EUCLEAN);
4635         }

And

185
186 /*
187  * Helper to output refs and locking status of extent buffer.  Useful to debug
188  * race condition related problems.
189  */
190 static void print_eb_refs_lock(struct extent_buffer *eb)
191 {
192 #ifdef CONFIG_BTRFS_DEBUG
193         btrfs_info(eb->fs_info, "refs %u lock_owner %u current %u",
194                    atomic_read(&eb->refs), eb->lock_owner, current->pid);
195 #endif
196 }

The safety check added in b72c3aba09a53fc7c18 ("btrfs: locking: Add
extra check in btrfs_init_new_buffer() to avoid deadlock") and it seems
to be useful but I think it builds on the assumptions of the previous
tree locks. The mentioned warning uses the recursive locking which is
being removed.

For debugging we could keep the lock_owner in eb, but under the
CONFIG_BTRFS_DEBUG, so for the release build the eb size is reduced.

Re: [PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[Josef Bacik]

On 11/11/20 9:29 AM, David Sterba wrote:
> On Fri, Nov 06, 2020 at 04:27:32PM -0500, Josef Bacik wrote:
>> @@ -71,31 +47,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
>>   	if (trace_btrfs_tree_read_lock_enabled())
>>   		start_ns = ktime_get_ns();
>>   
>> -	if (unlikely(recurse)) {
>> -		/* First see if we can grab the lock outright */
>> -		if (down_read_trylock(&eb->lock))
>> -			goto out;
>> -
>> -		/*
>> -		 * Ok still doesn't necessarily mean we are already holding the
>> -		 * lock, check the owner.
>> -		 */
>> -		if (eb->lock_owner != current->pid) {
> 
> This
> 
>> -			down_read_nested(&eb->lock, nest);
>> -			goto out;
>> -		}
>> -
>> -		/*
>> -		 * Ok we have actually recursed, but we should only be recursing
>> -		 * once, so blow up if we're already recursed, otherwise set
>> -		 * ->lock_recursed and carry on.
>> -		 */
>> -		BUG_ON(eb->lock_recursed);
>> -		eb->lock_recursed = true;
>> -		goto out;
>> -	}
>>   	down_read_nested(&eb->lock, nest);
>> -out:
>>   	eb->lock_owner = current->pid;
>>   	trace_btrfs_tree_read_lock(eb, start_ns);
>>   }
>> @@ -136,22 +88,11 @@ int btrfs_try_tree_write_lock(struct extent_buffer *eb)
>>   }
>>   
>>   /*
>> - * Release read lock.  If the read lock was recursed then the lock stays in the
>> - * original state that it was before it was recursively locked.
>> + * Release read lock.
>>    */
>>   void btrfs_tree_read_unlock(struct extent_buffer *eb)
>>   {
>>   	trace_btrfs_tree_read_unlock(eb);
>> -	/*
>> -	 * if we're nested, we have the write lock.  No new locking
>> -	 * is needed as long as we are the lock owner.
>> -	 * The write unlock will do a barrier for us, and the lock_recursed
>> -	 * field only matters to the lock owner.
>> -	 */
>> -	if (eb->lock_recursed && current->pid == eb->lock_owner) {
> 
> And this were the last uses of the lock_owner inside locks, so when the
> recursion is gone, the remainig use:
> 
> btrfs_init_new_buffer:
> 
> 4624         /*
> 4625          * Extra safety check in case the extent tree is corrupted and extent
> 4626          * allocator chooses to use a tree block which is already used and
> 4627          * locked.
> 4628          */
> 4629         if (buf->lock_owner == current->pid) {
> 4630                 btrfs_err_rl(fs_info,
> 4631 "tree block %llu owner %llu already locked by pid=%d, extent tree corruption detected",
> 4632                         buf->start, btrfs_header_owner(buf), current->pid);
> 4633                 free_extent_buffer(buf);
> 4634                 return ERR_PTR(-EUCLEAN);
> 4635         }
> 
> And
> 
> 185
> 186 /*
> 187  * Helper to output refs and locking status of extent buffer.  Useful to debug
> 188  * race condition related problems.
> 189  */
> 190 static void print_eb_refs_lock(struct extent_buffer *eb)
> 191 {
> 192 #ifdef CONFIG_BTRFS_DEBUG
> 193         btrfs_info(eb->fs_info, "refs %u lock_owner %u current %u",
> 194                    atomic_read(&eb->refs), eb->lock_owner, current->pid);
> 195 #endif
> 196 }
> 
> The safety check added in b72c3aba09a53fc7c18 ("btrfs: locking: Add
> extra check in btrfs_init_new_buffer() to avoid deadlock") and it seems
> to be useful but I think it builds on the assumptions of the previous
> tree locks. The mentioned warning uses the recursive locking which is
> being removed.

Sorry I should have explained why I was leaving this in my cover letter.  The 
safety check is for the case that the free space cache is corrupted and we try 
to allocate a block that we are currently using and have locked in the path.  I 
would have preferred to move it under CONFIG_BTRFS_DEBUG, but it does actually 
help in the case of a bad free space cache, so I think we have to keep it.  Thanks,

Josef

Re: [PATCH 4/8] btrfs: remove the recursion handling code in locking.c

[David Sterba]

On Wed, Nov 11, 2020 at 09:43:50AM -0500, Josef Bacik wrote:
> > 185
> > 186 /*
> > 187  * Helper to output refs and locking status of extent buffer.  Useful to debug
> > 188  * race condition related problems.
> > 189  */
> > 190 static void print_eb_refs_lock(struct extent_buffer *eb)
> > 191 {
> > 192 #ifdef CONFIG_BTRFS_DEBUG
> > 193         btrfs_info(eb->fs_info, "refs %u lock_owner %u current %u",
> > 194                    atomic_read(&eb->refs), eb->lock_owner, current->pid);
> > 195 #endif
> > 196 }
> > 
> > The safety check added in b72c3aba09a53fc7c18 ("btrfs: locking: Add
> > extra check in btrfs_init_new_buffer() to avoid deadlock") and it seems
> > to be useful but I think it builds on the assumptions of the previous
> > tree locks. The mentioned warning uses the recursive locking which is
> > being removed.
> 
> Sorry I should have explained why I was leaving this in my cover letter.  The 
> safety check is for the case that the free space cache is corrupted and we try 
> to allocate a block that we are currently using and have locked in the path.  I 
> would have preferred to move it under CONFIG_BTRFS_DEBUG, but it does actually 
> help in the case of a bad free space cache, so I think we have to keep it.  Thanks,

Yeah that's a valid reason to keep it, I'll update the changelog.

[PATCH 5/8] btrfs: remove __btrfs_read_lock_root_node

[Josef Bacik]

We no longer have recursive locking, so remove this variant.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c   | 2 +-
 fs/btrfs/locking.c | 5 ++---
 fs/btrfs/locking.h | 8 +-------
 3 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index cdd86ced917a..ef7389e6be3d 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2588,7 +2588,7 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
 		 * We don't know the level of the root node until we actually
 		 * have it read locked
 		 */
-		b = __btrfs_read_lock_root_node(root, 0);
+		b = btrfs_read_lock_root_node(root);
 		level = btrfs_header_level(b);
 		if (level > write_lock_level)
 			goto out;
diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index 9b66154803a7..c8b239376fb4 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -185,14 +185,13 @@ struct extent_buffer *btrfs_lock_root_node(struct btrfs_root *root)
  *
  * Return: root extent buffer with read lock held
  */
-struct extent_buffer *__btrfs_read_lock_root_node(struct btrfs_root *root,
-						  bool recurse)
+struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
 {
 	struct extent_buffer *eb;
 
 	while (1) {
 		eb = btrfs_root_node(root);
-		__btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL, recurse);
+		btrfs_tree_read_lock(eb);
 		if (eb == root->node)
 			break;
 		btrfs_tree_read_unlock(eb);
diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
index f8f2fd835582..91441e31db18 100644
--- a/fs/btrfs/locking.h
+++ b/fs/btrfs/locking.h
@@ -94,13 +94,7 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb);
 int btrfs_try_tree_read_lock(struct extent_buffer *eb);
 int btrfs_try_tree_write_lock(struct extent_buffer *eb);
 struct extent_buffer *btrfs_lock_root_node(struct btrfs_root *root);
-struct extent_buffer *__btrfs_read_lock_root_node(struct btrfs_root *root,
-						  bool recurse);
-
-static inline struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
-{
-	return __btrfs_read_lock_root_node(root, false);
-}
+struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root);
 
 #ifdef CONFIG_BTRFS_DEBUG
 static inline void btrfs_assert_tree_locked(struct extent_buffer *eb) {
-- 
2.26.2

Re: [PATCH 5/8] btrfs: remove __btrfs_read_lock_root_node

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:30 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> We no longer have recursive locking, so remove this variant.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/ctree.c   | 2 +-
>  fs/btrfs/locking.c | 5 ++---
>  fs/btrfs/locking.h | 8 +-------
>  3 files changed, 4 insertions(+), 11 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index cdd86ced917a..ef7389e6be3d 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2588,7 +2588,7 @@ static struct extent_buffer *btrfs_search_slot_get_root(struct btrfs_root *root,
>                  * We don't know the level of the root node until we actually
>                  * have it read locked
>                  */
> -               b = __btrfs_read_lock_root_node(root, 0);
> +               b = btrfs_read_lock_root_node(root);
>                 level = btrfs_header_level(b);
>                 if (level > write_lock_level)
>                         goto out;
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index 9b66154803a7..c8b239376fb4 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -185,14 +185,13 @@ struct extent_buffer *btrfs_lock_root_node(struct btrfs_root *root)
>   *
>   * Return: root extent buffer with read lock held
>   */
> -struct extent_buffer *__btrfs_read_lock_root_node(struct btrfs_root *root,
> -                                                 bool recurse)
> +struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
>  {
>         struct extent_buffer *eb;
>
>         while (1) {
>                 eb = btrfs_root_node(root);
> -               __btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL, recurse);
> +               btrfs_tree_read_lock(eb);
>                 if (eb == root->node)
>                         break;
>                 btrfs_tree_read_unlock(eb);
> diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
> index f8f2fd835582..91441e31db18 100644
> --- a/fs/btrfs/locking.h
> +++ b/fs/btrfs/locking.h
> @@ -94,13 +94,7 @@ void btrfs_tree_read_unlock(struct extent_buffer *eb);
>  int btrfs_try_tree_read_lock(struct extent_buffer *eb);
>  int btrfs_try_tree_write_lock(struct extent_buffer *eb);
>  struct extent_buffer *btrfs_lock_root_node(struct btrfs_root *root);
> -struct extent_buffer *__btrfs_read_lock_root_node(struct btrfs_root *root,
> -                                                 bool recurse);
> -
> -static inline struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
> -{
> -       return __btrfs_read_lock_root_node(root, false);
> -}
> +struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root);
>
>  #ifdef CONFIG_BTRFS_DEBUG
>  static inline void btrfs_assert_tree_locked(struct extent_buffer *eb) {
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 6/8] btrfs: use btrfs_tree_read_lock in btrfs_search_slot

[Josef Bacik]

We no longer use recursion, so
__btrfs_tree_read_lock(BTRFS_NESTING_NORMAL) == btrfs_tree_read_lock.
Replace this call with the simple helper.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/ctree.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index ef7389e6be3d..2cdfaf7298ab 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -2857,8 +2857,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 				btrfs_tree_lock(b);
 				p->locks[level] = BTRFS_WRITE_LOCK;
 			} else {
-				__btrfs_tree_read_lock(b, BTRFS_NESTING_NORMAL,
-						       0);
+				btrfs_tree_read_lock(b);
 				p->locks[level] = BTRFS_READ_LOCK;
 			}
 			p->nodes[level] = b;
-- 
2.26.2

Re: [PATCH 6/8] btrfs: use btrfs_tree_read_lock in btrfs_search_slot

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> We no longer use recursion, so
> __btrfs_tree_read_lock(BTRFS_NESTING_NORMAL) == btrfs_tree_read_lock.
> Replace this call with the simple helper.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/ctree.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
> index ef7389e6be3d..2cdfaf7298ab 100644
> --- a/fs/btrfs/ctree.c
> +++ b/fs/btrfs/ctree.c
> @@ -2857,8 +2857,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>                                 btrfs_tree_lock(b);
>                                 p->locks[level] = BTRFS_WRITE_LOCK;
>                         } else {
> -                               __btrfs_tree_read_lock(b, BTRFS_NESTING_NORMAL,
> -                                                      0);
> +                               btrfs_tree_read_lock(b);
>                                 p->locks[level] = BTRFS_READ_LOCK;
>                         }
>                         p->nodes[level] = b;
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 7/8] btrfs: remove the recurse parameter from __btrfs_tree_read_lock

[Josef Bacik]

It is completely unused now, remove it.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/locking.c | 6 ++----
 fs/btrfs/locking.h | 3 +--
 2 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
index c8b239376fb4..e6fa53dddbb6 100644
--- a/fs/btrfs/locking.c
+++ b/fs/btrfs/locking.c
@@ -34,13 +34,11 @@
  * __btrfs_tree_read_lock: Lock the extent buffer for read.
  * @eb:  the eb to be locked
  * @nest: the nesting level to be used for lockdep
- * @recurse: unused.
  *
  * This takes the read lock on the extent buffer, using the specified nesting
  * level for lockdep purposes.
  */
-void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
-			    bool recurse)
+void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest)
 {
 	u64 start_ns = 0;
 
@@ -54,7 +52,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
 
 void btrfs_tree_read_lock(struct extent_buffer *eb)
 {
-	__btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL, false);
+	__btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL);
 }
 
 /*
diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
index 91441e31db18..a2e1f1f5c6e3 100644
--- a/fs/btrfs/locking.h
+++ b/fs/btrfs/locking.h
@@ -87,8 +87,7 @@ void __btrfs_tree_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest);
 void btrfs_tree_lock(struct extent_buffer *eb);
 void btrfs_tree_unlock(struct extent_buffer *eb);
 
-void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
-			    bool recurse);
+void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest);
 void btrfs_tree_read_lock(struct extent_buffer *eb);
 void btrfs_tree_read_unlock(struct extent_buffer *eb);
 int btrfs_try_tree_read_lock(struct extent_buffer *eb);
-- 
2.26.2

Re: [PATCH 7/8] btrfs: remove the recurse parameter from __btrfs_tree_read_lock

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> It is completely unused now, remove it.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/locking.c | 6 ++----
>  fs/btrfs/locking.h | 3 +--
>  2 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/btrfs/locking.c b/fs/btrfs/locking.c
> index c8b239376fb4..e6fa53dddbb6 100644
> --- a/fs/btrfs/locking.c
> +++ b/fs/btrfs/locking.c
> @@ -34,13 +34,11 @@
>   * __btrfs_tree_read_lock: Lock the extent buffer for read.
>   * @eb:  the eb to be locked
>   * @nest: the nesting level to be used for lockdep
> - * @recurse: unused.
>   *
>   * This takes the read lock on the extent buffer, using the specified nesting
>   * level for lockdep purposes.
>   */
> -void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
> -                           bool recurse)
> +void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest)
>  {
>         u64 start_ns = 0;
>
> @@ -54,7 +52,7 @@ void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting ne
>
>  void btrfs_tree_read_lock(struct extent_buffer *eb)
>  {
> -       __btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL, false);
> +       __btrfs_tree_read_lock(eb, BTRFS_NESTING_NORMAL);
>  }
>
>  /*
> diff --git a/fs/btrfs/locking.h b/fs/btrfs/locking.h
> index 91441e31db18..a2e1f1f5c6e3 100644
> --- a/fs/btrfs/locking.h
> +++ b/fs/btrfs/locking.h
> @@ -87,8 +87,7 @@ void __btrfs_tree_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest);
>  void btrfs_tree_lock(struct extent_buffer *eb);
>  void btrfs_tree_unlock(struct extent_buffer *eb);
>
> -void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest,
> -                           bool recurse);
> +void __btrfs_tree_read_lock(struct extent_buffer *eb, enum btrfs_lock_nesting nest);
>  void btrfs_tree_read_lock(struct extent_buffer *eb);
>  void btrfs_tree_read_unlock(struct extent_buffer *eb);
>  int btrfs_try_tree_read_lock(struct extent_buffer *eb);
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

[PATCH 8/8] btrfs: remove ->recursed from extent_buffer

[Josef Bacik]

It is unused everywhere now, it can be removed.

Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
 fs/btrfs/extent_io.c | 1 -
 fs/btrfs/extent_io.h | 1 -
 2 files changed, 2 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 119ced4a501b..32c2000a2269 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -4955,7 +4955,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
 	eb->fs_info = fs_info;
 	eb->bflags = 0;
 	init_rwsem(&eb->lock);
-	eb->lock_recursed = false;
 
 	btrfs_leak_debug_add(&fs_info->eb_leak_lock, &eb->leak_list,
 			     &fs_info->allocated_ebs);
diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
index 3c2bf21c54eb..b0bf7f75b113 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -87,7 +87,6 @@ struct extent_buffer {
 	int read_mirror;
 	struct rcu_head rcu_head;
 	pid_t lock_owner;
-	bool lock_recursed;
 	/* >= 0 if eb belongs to a log tree, -1 otherwise */
 	s8 log_index;
 
-- 
2.26.2

Re: [PATCH 8/8] btrfs: remove ->recursed from extent_buffer

[Filipe Manana]

On Fri, Nov 6, 2020 at 9:29 PM Josef Bacik <josef@toxicpanda.com> wrote:
>
> It is unused everywhere now, it can be removed.
>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Looks good, thanks.

> ---
>  fs/btrfs/extent_io.c | 1 -
>  fs/btrfs/extent_io.h | 1 -
>  2 files changed, 2 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 119ced4a501b..32c2000a2269 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -4955,7 +4955,6 @@ __alloc_extent_buffer(struct btrfs_fs_info *fs_info, u64 start,
>         eb->fs_info = fs_info;
>         eb->bflags = 0;
>         init_rwsem(&eb->lock);
> -       eb->lock_recursed = false;
>
>         btrfs_leak_debug_add(&fs_info->eb_leak_lock, &eb->leak_list,
>                              &fs_info->allocated_ebs);
> diff --git a/fs/btrfs/extent_io.h b/fs/btrfs/extent_io.h
> index 3c2bf21c54eb..b0bf7f75b113 100644
> --- a/fs/btrfs/extent_io.h
> +++ b/fs/btrfs/extent_io.h
> @@ -87,7 +87,6 @@ struct extent_buffer {
>         int read_mirror;
>         struct rcu_head rcu_head;
>         pid_t lock_owner;
> -       bool lock_recursed;
>         /* >= 0 if eb belongs to a log tree, -1 otherwise */
>         s8 log_index;
>
> --
> 2.26.2
>


-- 
Filipe David Manana,

“Whether you think you can, or you think you can't — you're right.”

Re: [PATCH 0/8] Locking cleanups and lockdep fix

[David Sterba]

On Fri, Nov 06, 2020 at 04:27:28PM -0500, Josef Bacik wrote:
> Hello,
> 
> Filipe reported a lockdep splat that he hit on btrfs/187, but honestly could be
> hit anywhere we do readdir on a sufficiently large fs.  Fixing this is fairly
> straightforward, but enabled me to do a lot of extra cleanups, especially now
> that my other locking fixes have been merged.  The first two patches are to
> address the lockdep problem.  The followup patches are cleaning out the
> recursive locking support, which we no longer require.  I would have separated
> this work, but btrfs_next_old_leaf was a heavy user of this, so it would be
> annoying to take separately, hence putting it all together.  Thanks,
> 
> Josef
> 
> Josef Bacik (8):
>   btrfs: cleanup the locking in btrfs_next_old_leaf
>   btrfs: unlock to current level in btrfs_next_old_leaf
>   btrfs: kill path->recurse
>   btrfs: remove the recursion handling code in locking.c
>   btrfs: remove __btrfs_read_lock_root_node
>   btrfs: use btrfs_tree_read_lock in btrfs_search_slot
>   btrfs: remove the recurse parameter from __btrfs_tree_read_lock
>   btrfs: remove ->recursed from extent_buffer

Added to misc-next, with some minor updates in changelogs. Seems that
this should remove all leftovers after the locking switch.