diff options
author | Kent Overstreet <kent.overstreet@gmail.com> | 2020-06-02 19:41:47 -0400 |
---|---|---|
committer | Kent Overstreet <kent.overstreet@gmail.com> | 2020-06-09 21:32:46 -0400 |
commit | f38a26d52f4c07922329d6679ef0f7d1155da6b2 (patch) | |
tree | a34be30a4f8fae2d41f62ffd2f0e8c5d75257a6a | |
parent | 2cbe7fa7412000db32c84a83f01edcad49f3f398 (diff) |
bcachefs: Fix a deadlock in bch2_btree_node_get_sibling()
There was a bad interaction with bch2_btree_iter_set_pos_same_leaf(),
which can leave a btree node locked that is just outside iter->pos,
breaking the lock ordering checks in __bch2_btree_node_lock(). Ideally
we should get rid of this corner case, but for now fix it locally with
verbose comments.
Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
-rw-r--r-- | fs/bcachefs/btree_cache.c | 12 | ||||
-rw-r--r-- | fs/bcachefs/btree_iter.c | 18 | ||||
-rw-r--r-- | fs/bcachefs/btree_iter.h | 9 | ||||
-rw-r--r-- | fs/bcachefs/btree_types.h | 1 |
4 files changed, 29 insertions, 11 deletions
diff --git a/fs/bcachefs/btree_cache.c b/fs/bcachefs/btree_cache.c index 732bbd377e12..fc69f6855d29 100644 --- a/fs/bcachefs/btree_cache.c +++ b/fs/bcachefs/btree_cache.c @@ -850,6 +850,18 @@ struct btree *bch2_btree_node_get_sibling(struct bch_fs *c, if (!parent) return NULL; + /* + * There's a corner case where a btree_iter might have a node locked + * that is just outside its current pos - when + * bch2_btree_iter_set_pos_same_leaf() gets to the end of the node. + * + * But the lock ordering checks in __bch2_btree_node_lock() go off of + * iter->pos, not the node's key: so if the iterator is marked as + * needing to be traversed, we risk deadlock if we don't bail out here: + */ + if (iter->uptodate >= BTREE_ITER_NEED_TRAVERSE) + return ERR_PTR(-EINTR); + if (!bch2_btree_node_relock(iter, level + 1)) { ret = ERR_PTR(-EINTR); goto out; diff --git a/fs/bcachefs/btree_iter.c b/fs/bcachefs/btree_iter.c index 29929298a1a9..6abcbe3debe5 100644 --- a/fs/bcachefs/btree_iter.c +++ b/fs/bcachefs/btree_iter.c @@ -205,8 +205,9 @@ bool __bch2_btree_node_lock(struct btree *b, struct bpos pos, if (!linked->nodes_locked) continue; - /* * Must lock btree nodes in key order: */ - if (__btree_iter_cmp(iter->btree_id, pos, linked) < 0) + /* Must lock btree nodes in key order: */ + if ((cmp_int(iter->btree_id, linked->btree_id) ?: + bkey_cmp(pos, linked->pos)) < 0) ret = false; /* @@ -1320,6 +1321,16 @@ void bch2_btree_iter_set_pos_same_leaf(struct btree_iter *iter, struct bpos new_ btree_iter_advance_to_pos(iter, l, -1); + /* + * XXX: + * keeping a node locked that's outside (even just outside) iter->pos + * breaks __bch2_btree_node_lock(). This seems to only affect + * bch2_btree_node_get_sibling so for now it's fixed there, but we + * should try to get rid of this corner case. + * + * (this behaviour is currently needed for BTREE_INSERT_NOUNLOCK) + */ + if (bch2_btree_node_iter_end(&l->iter) && btree_iter_pos_after_node(iter, l->b)) btree_iter_set_dirty(iter, BTREE_ITER_NEED_TRAVERSE); @@ -2194,6 +2205,7 @@ void bch2_trans_init(struct btree_trans *trans, struct bch_fs *c, bch2_trans_preload_mem(trans, expected_mem_bytes); #ifdef CONFIG_BCACHEFS_DEBUG + trans->pid = current->pid; mutex_lock(&c->btree_trans_lock); list_add(&trans->list, &c->btree_trans_list); mutex_unlock(&c->btree_trans_lock); @@ -2232,7 +2244,7 @@ void bch2_btree_trans_to_text(struct printbuf *out, struct bch_fs *c) mutex_lock(&c->btree_trans_lock); list_for_each_entry(trans, &c->btree_trans_list, list) { - pr_buf(out, "%ps\n", (void *) trans->ip); + pr_buf(out, "%i %ps\n", trans->pid, (void *) trans->ip); trans_for_each_iter(trans, iter) { if (!iter->nodes_locked) diff --git a/fs/bcachefs/btree_iter.h b/fs/bcachefs/btree_iter.h index 841a5834f1a8..ab35fcd8b8b4 100644 --- a/fs/bcachefs/btree_iter.h +++ b/fs/bcachefs/btree_iter.h @@ -172,17 +172,10 @@ void bch2_btree_iter_set_pos_same_leaf(struct btree_iter *, struct bpos); void __bch2_btree_iter_set_pos(struct btree_iter *, struct bpos, bool); void bch2_btree_iter_set_pos(struct btree_iter *, struct bpos); -static inline int __btree_iter_cmp(enum btree_id id, - struct bpos pos, - const struct btree_iter *r) -{ - return cmp_int(id, r->btree_id) ?: bkey_cmp(pos, r->pos); -} - static inline int btree_iter_cmp(const struct btree_iter *l, const struct btree_iter *r) { - return __btree_iter_cmp(l->btree_id, l->pos, r); + return cmp_int(l->btree_id, r->btree_id) ?: bkey_cmp(l->pos, r->pos); } /* diff --git a/fs/bcachefs/btree_types.h b/fs/bcachefs/btree_types.h index b86d7369eb2d..e97248ca3aa2 100644 --- a/fs/bcachefs/btree_types.h +++ b/fs/bcachefs/btree_types.h @@ -284,6 +284,7 @@ struct btree_trans { #ifdef CONFIG_BCACHEFS_DEBUG struct list_head list; struct btree *locking; + pid_t pid; #endif unsigned long ip; |