bcachefs: Tighten up btree locking invariants

New rule is: if a btree path holds any locks it should be holding
precisely the locks wanted (accoringing to path->level and
path->locks_want).

Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>

3 files changed, 29 insertions, 46 deletions

diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c
index d7537d0d1802..90ce312c57f9 100644
--- a/fs/bcachefs/btree_iter.c
+++ b/fs/bcachefs/btree_iter.c
@@ -219,7 +219,6 @@ static inline bool btree_path_get_locks(struct btree_trans *trans,
 					? path->l[l].b->c.lock.state.seq
 					: 0);
 			fail_idx = l;
-			btree_path_set_dirty(path, BTREE_ITER_NEED_TRAVERSE);
 		}
 
 		l++;
@@ -230,10 +229,14 @@ static inline bool btree_path_get_locks(struct btree_trans *trans,
 	 * can't be relocked so bch2_btree_path_traverse has to walk back up to
 	 * the node that we failed to relock:
 	 */
-	while (fail_idx >= 0) {
-		btree_node_unlock(path, fail_idx);
-		path->l[fail_idx].b = BTREE_ITER_NO_NODE_GET_LOCKS;
-		--fail_idx;
+	if (fail_idx >= 0) {
+		__bch2_btree_path_unlock(path);
+		btree_path_set_dirty(path, BTREE_ITER_NEED_TRAVERSE);
+
+		do {
+			path->l[fail_idx].b = BTREE_ITER_NO_NODE_GET_LOCKS;
+			--fail_idx;
+		} while (fail_idx >= 0);
 	}
 
 	if (path->uptodate == BTREE_ITER_NEED_RELOCK)
@@ -372,14 +375,14 @@ static void bch2_btree_path_verify_locks(struct btree_path *path)
 {
 	unsigned l;
 
-	for (l = 0; btree_path_node(path, l); l++) {
-		if (path->uptodate >= BTREE_ITER_NEED_RELOCK &&
-		    !btree_node_locked(path, l))
-			continue;
+	if (!path->nodes_locked) {
+		BUG_ON(path->uptodate == BTREE_ITER_UPTODATE);
+		return;
+	}
 
+	for (l = 0; btree_path_node(path, l); l++)
 		BUG_ON(btree_lock_want(path, l) !=
 		       btree_node_locked_type(path, l));
-	}
 }
 
 void bch2_trans_verify_locks(struct btree_trans *trans)
@@ -417,6 +420,7 @@ bool bch2_btree_path_relock_intent(struct btree_trans *trans,
 					is_btree_node(path, l)
 					? path->l[l].b->c.lock.state.seq
 					: 0);
+			__bch2_btree_path_unlock(path);
 			btree_path_set_dirty(path, BTREE_ITER_NEED_TRAVERSE);
 			btree_trans_restart(trans);
 			return false;
@@ -666,9 +670,6 @@ void bch2_trans_verify_paths(struct btree_trans *trans)
 {
 	struct btree_path *path;
 
-	if (!bch2_debug_check_iterators)
-		return;
-
 	trans_for_each_path(trans, path)
 		bch2_btree_path_verify(trans, path);
 }
@@ -1009,7 +1010,7 @@ static void btree_path_verify_new_node(struct btree_trans *trans,
 	}
 
 	if (!parent_locked)
-		btree_node_unlock(path, b->c.level + 1);
+		btree_node_unlock(path, plevel);
 }
 
 static inline void __btree_path_level_init(struct btree_path *path,
@@ -1051,21 +1052,17 @@ static inline void btree_path_level_init(struct btree_trans *trans,
  */
 void bch2_trans_node_add(struct btree_trans *trans, struct btree *b)
 {
-	enum btree_node_locked_type t;
 	struct btree_path *path;
 
 	trans_for_each_path(trans, path)
 		if (!path->cached &&
 		    btree_path_pos_in_node(path, b)) {
-			/*
-			 * bch2_btree_path_node_drop() has already been called -
-			 * the old node we're replacing has already been
-			 * unlocked and the pointer invalidated
-			 */
-			BUG_ON(btree_node_locked(path, b->c.level));
+			enum btree_node_locked_type t =
+				btree_lock_want(path, b->c.level);
 
-			t = btree_lock_want(path, b->c.level);
-			if (t != BTREE_NODE_UNLOCKED) {
+			if (path->nodes_locked &&
+			    t != BTREE_NODE_UNLOCKED) {
+				btree_node_unlock(path, b->c.level);
 				six_lock_increment(&b->c.lock, t);
 				mark_btree_node_locked(path, b->c.level, t);
 			}
@@ -1074,18 +1071,6 @@ void bch2_trans_node_add(struct btree_trans *trans, struct btree *b)
 		}
 }
 
-void bch2_trans_node_drop(struct btree_trans *trans, struct btree *b)
-{
-	struct btree_path *path;
-	unsigned level = b->c.level;
-
-	trans_for_each_path(trans, path)
-		if (path->l[level].b == b) {
-			btree_node_unlock(path, level);
-			path->l[level].b = BTREE_ITER_NO_NODE_DROP;
-		}
-}
-
 /*
  * A btree node has been modified in such a way as to invalidate iterators - fix
  * them:
@@ -1575,14 +1560,12 @@ btree_path_set_pos(struct btree_trans *trans,
 		if (cmp < 0 ||
 		    !btree_path_advance_to_pos(path, &path->l[l], 8))
 			__btree_path_level_init(path, l);
-
-		/* Don't leave it locked if we're not supposed to: */
-		if (btree_lock_want(path, l) == BTREE_NODE_UNLOCKED)
-			btree_node_unlock(path, l);
 	}
 
-	if (l != path->level)
+	if (l != path->level) {
 		btree_path_set_dirty(path, BTREE_ITER_NEED_TRAVERSE);
+		__bch2_btree_path_unlock(path);
+	}
 out:
 	bch2_btree_path_verify(trans, path);
 #ifdef CONFIG_BCACHEFS_DEBUG
@@ -2699,9 +2682,10 @@ void bch2_btree_trans_to_text(struct printbuf *out, struct bch_fs *c)
 			if (!path->nodes_locked)
 				continue;
 
-			pr_buf(out, "  path %u %c %s:",
+			pr_buf(out, "  path %u %c l=%u %s:",
 			       path->idx,
 			       path->cached ? 'c' : 'b',
+			       path->level,
 			       bch2_btree_ids[path->btree_id]);
 			bch2_bpos_to_text(out, path->pos);
 			pr_buf(out, "\n");
diff --git a/fs/bcachefs/btree_iter.h b/fs/bcachefs/btree_iter.h
index 273bc7f3a29b..ed41b52a5b5d 100644
--- a/fs/bcachefs/btree_iter.h
+++ b/fs/bcachefs/btree_iter.h
@@ -193,7 +193,6 @@ static inline void bch2_btree_path_downgrade(struct btree_path *path)
 void bch2_trans_downgrade(struct btree_trans *);
 
 void bch2_trans_node_add(struct btree_trans *trans, struct btree *);
-void bch2_trans_node_drop(struct btree_trans *, struct btree *);
 void bch2_trans_node_reinit_iter(struct btree_trans *, struct btree *);
 
 int __must_check bch2_btree_iter_traverse(struct btree_iter *);
diff --git a/fs/bcachefs/btree_update_interior.c b/fs/bcachefs/btree_update_interior.c
index 5534573af425..4e428d2c0eeb 100644
--- a/fs/bcachefs/btree_update_interior.c
+++ b/fs/bcachefs/btree_update_interior.c
@@ -1429,7 +1429,6 @@ static void btree_split(struct btree_update *as, struct btree_trans *trans,
 	/* Successful split, update the path to point to the new nodes: */
 
 	six_lock_increment(&b->c.lock, SIX_LOCK_intent);
-	bch2_trans_node_drop(trans, b);
 	if (n3)
 		bch2_trans_node_add(trans, n3);
 	if (n2)
@@ -1694,14 +1693,16 @@ retry:
 	bch2_keylist_add(&as->parent_keys, &delete);
 	bch2_keylist_add(&as->parent_keys, &n->key);
 
+	bch2_trans_verify_paths(trans);
+
 	bch2_btree_insert_node(as, trans, path, parent, &as->parent_keys, flags);
 
+	bch2_trans_verify_paths(trans);
+
 	bch2_btree_update_get_open_buckets(as, n);
 
 	six_lock_increment(&b->c.lock, SIX_LOCK_intent);
 	six_lock_increment(&m->c.lock, SIX_LOCK_intent);
-	bch2_trans_node_drop(trans, b);
-	bch2_trans_node_drop(trans, m);
 
 	bch2_trans_node_add(trans, n);
 
@@ -1798,7 +1799,6 @@ retry:
 	bch2_btree_update_get_open_buckets(as, n);
 
 	six_lock_increment(&b->c.lock, SIX_LOCK_intent);
-	bch2_trans_node_drop(trans, b);
 	bch2_trans_node_add(trans, n);
 	bch2_btree_node_free_inmem(trans, b);
 	six_unlock_intent(&n->c.lock);