bcachefs: iter->real_pos

We need to differentiate between the search position of a btree
iterator, vs. what it actually points at (what we found). This matters
for extents, where iter->pos will typically be the start of the key we
found and iter->real_pos will be the end of the key we found (which soon
won't necessarily be in the same btree node!) and it will also matter
for snapshots.
Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>
Signed-off-by: Kent Overstreet <kent.overstreet@linux.dev>

fs/bcachefs/btree_iter.c

@@ -34,13 +34,13 @@ static inline struct bpos btree_iter_search_key(struct btree_iter *iter)
 static inline bool btree_iter_pos_before_node(struct btree_iter *iter,
 					      struct btree *b)
 {
-	return bkey_cmp(btree_iter_search_key(iter), b->data->min_key) < 0;
+	return bkey_cmp(iter->real_pos, b->data->min_key) < 0;
 }
 
 static inline bool btree_iter_pos_after_node(struct btree_iter *iter,
 					     struct btree *b)
 {
-	return bkey_cmp(b->key.k.p, btree_iter_search_key(iter)) < 0;
+	return bkey_cmp(b->key.k.p, iter->real_pos) < 0;
 }
 
 static inline bool btree_iter_pos_in_node(struct btree_iter *iter,
@@ -491,7 +491,6 @@ static void bch2_btree_iter_verify_cached(struct btree_iter *iter)
 static void bch2_btree_iter_verify_level(struct btree_iter *iter,
 					 unsigned level)
 {
-	struct bpos pos = btree_iter_search_key(iter);
 	struct btree_iter_level *l = &iter->l[level];
 	struct btree_node_iter tmp = l->iter;
 	bool locked = btree_node_locked(iter, level);
@@ -539,12 +538,12 @@ static void bch2_btree_iter_verify_level(struct btree_iter *iter,
 		: bch2_btree_node_iter_prev_all(&tmp, l->b);
 	k = bch2_btree_node_iter_peek_all(&l->iter, l->b);
 
-	if (p && bkey_iter_pos_cmp(l->b, p, &pos) >= 0) {
+	if (p && bkey_iter_pos_cmp(l->b, p, &iter->real_pos) >= 0) {
 		msg = "before";
 		goto err;
 	}
 
-	if (k && bkey_iter_pos_cmp(l->b, k, &pos) < 0) {
+	if (k && bkey_iter_pos_cmp(l->b, k, &iter->real_pos) < 0) {
 		msg = "after";
 		goto err;
 	}
@@ -567,12 +566,11 @@ static void bch2_btree_iter_verify_level(struct btree_iter *iter,
 	}
 
 	panic("iterator should be %s key at level %u:\n"
-	      "iter pos %s %llu:%llu\n"
+	      "iter pos %llu:%llu\n"
 	      "prev key %s\n"
 	      "cur  key %s\n",
 	      msg, level,
-	      iter->flags & BTREE_ITER_IS_EXTENTS ? ">" : "=>",
-	      iter->pos.inode, iter->pos.offset,
+	      iter->real_pos.inode, iter->real_pos.offset,
 	      buf1, buf2);
 }
 
@@ -580,12 +578,24 @@ static void bch2_btree_iter_verify(struct btree_iter *iter)
 {
 	unsigned i;
 
-	bch2_btree_trans_verify_locks(iter->trans);
+	EBUG_ON(iter->btree_id >= BTREE_ID_NR);
+
+	bch2_btree_iter_verify_locks(iter);
 
 	for (i = 0; i < BTREE_MAX_DEPTH; i++)
 		bch2_btree_iter_verify_level(iter, i);
 }
 
+static void bch2_btree_iter_verify_entry_exit(struct btree_iter *iter)
+{
+	enum btree_iter_type type = btree_iter_type(iter);
+
+	BUG_ON((type == BTREE_ITER_KEYS ||
+		type == BTREE_ITER_CACHED) &&
+	       (bkey_cmp(iter->pos, bkey_start_pos(&iter->k)) < 0 ||
+		bkey_cmp(iter->pos, iter->k.p) > 0));
+}
+
 void bch2_btree_trans_verify_iters(struct btree_trans *trans, struct btree *b)
 {
 	struct btree_iter *iter;
@@ -601,6 +611,7 @@ void bch2_btree_trans_verify_iters(struct btree_trans *trans, struct btree *b)
 
 static inline void bch2_btree_iter_verify_level(struct btree_iter *iter, unsigned l) {}
 static inline void bch2_btree_iter_verify(struct btree_iter *iter) {}
+static inline void bch2_btree_iter_verify_entry_exit(struct btree_iter *iter) {}
 
 #endif
 
@@ -626,12 +637,11 @@ static void __bch2_btree_iter_fix_key_modified(struct btree_iter *iter,
 					       struct bkey_packed *where)
 {
 	struct btree_iter_level *l = &iter->l[b->c.level];
-	struct bpos pos = btree_iter_search_key(iter);
 
 	if (where != bch2_btree_node_iter_peek_all(&l->iter, l->b))
 		return;
 
-	if (bkey_iter_pos_cmp(l->b, where, &pos) < 0)
+	if (bkey_iter_pos_cmp(l->b, where, &iter->real_pos) < 0)
 		bch2_btree_node_iter_advance(&l->iter, l->b);
 
 	btree_iter_set_dirty(iter, BTREE_ITER_NEED_PEEK);
@@ -666,7 +676,6 @@ static void __bch2_btree_node_iter_fix(struct btree_iter *iter,
 	bool iter_current_key_modified =
 		orig_iter_pos >= offset &&
 		orig_iter_pos <= offset + clobber_u64s;
-	struct bpos iter_pos = btree_iter_search_key(iter);
 
 	btree_node_iter_for_each(node_iter, set)
 		if (set->end == old_end)
@@ -674,7 +683,7 @@ static void __bch2_btree_node_iter_fix(struct btree_iter *iter,
 
 	/* didn't find the bset in the iterator - might have to readd it: */
 	if (new_u64s &&
-	    bkey_iter_pos_cmp(b, where, &iter_pos) >= 0) {
+	    bkey_iter_pos_cmp(b, where, &iter->real_pos) >= 0) {
 		bch2_btree_node_iter_push(node_iter, b, where, end);
 		goto fixup_done;
 	} else {
@@ -689,7 +698,7 @@ static void __bch2_btree_node_iter_fix(struct btree_iter *iter,
 		return;
 
 	if (new_u64s &&
-	    bkey_iter_pos_cmp(b, where, &iter_pos) >= 0) {
+	    bkey_iter_pos_cmp(b, where, &iter->real_pos) >= 0) {
 		set->k = offset;
 	} else if (set->k < offset + clobber_u64s) {
 		set->k = offset + new_u64s;
@@ -825,12 +834,11 @@ static inline bool btree_iter_advance_to_pos(struct btree_iter *iter,
 					     struct btree_iter_level *l,
 					     int max_advance)
 {
-	struct bpos pos = btree_iter_search_key(iter);
 	struct bkey_packed *k;
 	int nr_advanced = 0;
 
 	while ((k = bch2_btree_node_iter_peek_all(&l->iter, l->b)) &&
-	       bkey_iter_pos_cmp(l->b, k, &pos) < 0) {
+	       bkey_iter_pos_cmp(l->b, k, &iter->real_pos) < 0) {
 		if (max_advance > 0 && nr_advanced >= max_advance)
 			return false;
 
@@ -893,10 +901,9 @@ static void btree_iter_verify_new_node(struct btree_iter *iter, struct btree *b)
 static inline void __btree_iter_init(struct btree_iter *iter,
 				     unsigned level)
 {
-	struct bpos pos = btree_iter_search_key(iter);
 	struct btree_iter_level *l = &iter->l[level];
 
-	bch2_btree_node_iter_init(&l->iter, l->b, &pos);
+	bch2_btree_node_iter_init(&l->iter, l->b, &iter->real_pos);
 
 	/*
 	 * Iterators to interior nodes should always be pointed at the first non
@@ -1342,21 +1349,6 @@ int __must_check __bch2_btree_iter_traverse(struct btree_iter *iter)
 	return ret;
 }
 
-static inline void bch2_btree_iter_checks(struct btree_iter *iter)
-{
-	enum btree_iter_type type = btree_iter_type(iter);
-
-	EBUG_ON(iter->btree_id >= BTREE_ID_NR);
-
-	BUG_ON((type == BTREE_ITER_KEYS ||
-		type == BTREE_ITER_CACHED) &&
-	       (bkey_cmp(iter->pos, bkey_start_pos(&iter->k)) < 0 ||
-		bkey_cmp(iter->pos, iter->k.p) > 0));
-
-	bch2_btree_iter_verify_locks(iter);
-	bch2_btree_iter_verify_level(iter, iter->level);
-}
-
 /* Iterate across nodes (leaf and interior nodes) */
 
 struct btree *bch2_btree_iter_peek_node(struct btree_iter *iter)
@@ -1365,7 +1357,7 @@ struct btree *bch2_btree_iter_peek_node(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_NODES);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
 
 	if (iter->uptodate == BTREE_ITER_UPTODATE)
 		return iter->l[iter->level].b;
@@ -1380,7 +1372,7 @@ struct btree *bch2_btree_iter_peek_node(struct btree_iter *iter)
 
 	BUG_ON(bkey_cmp(b->key.k.p, iter->pos) < 0);
 
-	iter->pos = b->key.k.p;
+	iter->pos = iter->real_pos = b->key.k.p;
 	iter->uptodate = BTREE_ITER_UPTODATE;
 
 	bch2_btree_iter_verify(iter);
@@ -1394,7 +1386,7 @@ struct btree *bch2_btree_iter_next_node(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_NODES);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
 
 	/* already got to end? */
 	if (!btree_iter_node(iter, iter->level))
@@ -1431,7 +1423,7 @@ struct btree *bch2_btree_iter_next_node(struct btree_iter *iter)
 		if (btree_node_read_locked(iter, iter->level))
 			btree_node_unlock(iter, iter->level);
 
-		iter->pos	= bkey_successor(iter->pos);
+		iter->pos = iter->real_pos = bkey_successor(iter->pos);
 		iter->level	= iter->min_depth;
 
 		btree_iter_set_dirty(iter, BTREE_ITER_NEED_TRAVERSE);
@@ -1442,7 +1434,7 @@ struct btree *bch2_btree_iter_next_node(struct btree_iter *iter)
 		b = iter->l[iter->level].b;
 	}
 
-	iter->pos = b->key.k.p;
+	iter->pos = iter->real_pos = b->key.k.p;
 	iter->uptodate = BTREE_ITER_UPTODATE;
 
 	bch2_btree_iter_verify(iter);
@@ -1490,57 +1482,55 @@ static void btree_iter_pos_changed(struct btree_iter *iter, int cmp)
 		btree_iter_set_dirty(iter, BTREE_ITER_NEED_PEEK);
 }
 
-void __bch2_btree_iter_set_pos(struct btree_iter *iter, struct bpos new_pos,
-			       bool strictly_greater)
+static void btree_iter_set_search_pos(struct btree_iter *iter, struct bpos new_pos)
 {
-	struct bpos old = btree_iter_search_key(iter);
-	int cmp;
+	int cmp = bkey_cmp(new_pos, iter->real_pos);
 
-	iter->flags &= ~BTREE_ITER_IS_EXTENTS;
-	iter->flags |= strictly_greater ? BTREE_ITER_IS_EXTENTS : 0;
+	iter->real_pos = new_pos;
+
+	btree_iter_pos_changed(iter, cmp);
 
+	bch2_btree_iter_verify(iter);
+}
+
+void __bch2_btree_iter_set_pos(struct btree_iter *iter, struct bpos new_pos,
+			       bool strictly_greater)
+{
 	bkey_init(&iter->k);
 	iter->k.p = iter->pos = new_pos;
 
-	cmp = bkey_cmp(btree_iter_search_key(iter), old);
+	iter->flags &= ~BTREE_ITER_IS_EXTENTS;
+	iter->flags |= strictly_greater ? BTREE_ITER_IS_EXTENTS : 0;
 
-	btree_iter_pos_changed(iter, cmp);
+	btree_iter_set_search_pos(iter, btree_iter_search_key(iter));
 }
 
 void bch2_btree_iter_set_pos(struct btree_iter *iter, struct bpos new_pos)
 {
-	int cmp = bkey_cmp(new_pos, iter->pos);
-
-	bkey_init(&iter->k);
-	iter->k.p = iter->pos = new_pos;
-
-	btree_iter_pos_changed(iter, cmp);
+	__bch2_btree_iter_set_pos(iter, new_pos,
+			(iter->flags & BTREE_ITER_IS_EXTENTS) != 0);
 }
 
 static inline bool bch2_btree_iter_advance_pos(struct btree_iter *iter)
 {
 	struct bpos pos = iter->k.p;
+	bool ret = bkey_cmp(pos, POS_MAX) != 0;
 
-	if (unlikely(!bkey_cmp(pos, POS_MAX)))
-		return false;
-
-	if (!(iter->flags & BTREE_ITER_IS_EXTENTS))
+	if (ret && !(iter->flags & BTREE_ITER_IS_EXTENTS))
 		pos = bkey_successor(pos);
 	bch2_btree_iter_set_pos(iter, pos);
-	return true;
+	return ret;
 }
 
 static inline bool bch2_btree_iter_rewind_pos(struct btree_iter *iter)
 {
 	struct bpos pos = bkey_start_pos(&iter->k);
+	bool ret = bkey_cmp(pos, POS_MIN) != 0;
 
-	if (unlikely(!bkey_cmp(pos, POS_MIN)))
-		return false;
-
-	if (!(iter->flags & BTREE_ITER_IS_EXTENTS))
+	if (ret && !(iter->flags & BTREE_ITER_IS_EXTENTS))
 		pos = bkey_predecessor(pos);
 	bch2_btree_iter_set_pos(iter, pos);
-	return true;
+	return ret;
 }
 
 static inline bool btree_iter_set_pos_to_next_leaf(struct btree_iter *iter)
@@ -1548,10 +1538,16 @@ static inline bool btree_iter_set_pos_to_next_leaf(struct btree_iter *iter)
 	struct bpos next_pos = iter->l[0].b->key.k.p;
 	bool ret = bkey_cmp(next_pos, POS_MAX) != 0;
 
-	if (ret && !(iter->flags & BTREE_ITER_IS_EXTENTS))
-		next_pos = bkey_successor(next_pos);
+	/*
+	 * Typically, we don't want to modify iter->pos here, since that
+	 * indicates where we searched from - unless we got to the end of the
+	 * btree, in that case we want iter->pos to reflect that:
+	 */
+	if (ret)
+		btree_iter_set_search_pos(iter, bkey_successor(next_pos));
+	else
+		bch2_btree_iter_set_pos(iter, POS_MAX);
 
-	bch2_btree_iter_set_pos(iter, next_pos);
 	return ret;
 }
 
@@ -1560,14 +1556,11 @@ static inline bool btree_iter_set_pos_to_prev_leaf(struct btree_iter *iter)
 	struct bpos next_pos = iter->l[0].b->data->min_key;
 	bool ret = bkey_cmp(next_pos, POS_MIN) != 0;
 
-	if (ret) {
-		next_pos = bkey_predecessor(next_pos);
-
-		if (iter->flags & BTREE_ITER_IS_EXTENTS)
-			next_pos = bkey_predecessor(next_pos);
-	}
+	if (ret)
+		btree_iter_set_search_pos(iter, bkey_predecessor(next_pos));
+	else
+		bch2_btree_iter_set_pos(iter, POS_MIN);
 
-	bch2_btree_iter_set_pos(iter, next_pos);
 	return ret;
 }
 
@@ -1610,7 +1603,10 @@ struct bkey_s_c bch2_btree_iter_peek(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_KEYS);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
+	bch2_btree_iter_verify_entry_exit(iter);
+
+	btree_iter_set_search_pos(iter, btree_iter_search_key(iter));
 
 	if (iter->uptodate == BTREE_ITER_UPTODATE &&
 	    !bkey_deleted(&iter->k))
@@ -1636,9 +1632,12 @@ struct bkey_s_c bch2_btree_iter_peek(struct btree_iter *iter)
 	if (bkey_cmp(bkey_start_pos(k.k), iter->pos) > 0)
 		iter->pos = bkey_start_pos(k.k);
 
+	iter->real_pos = k.k->p;
+
 	iter->uptodate = BTREE_ITER_UPTODATE;
 
-	bch2_btree_iter_verify_level(iter, 0);
+	bch2_btree_iter_verify_entry_exit(iter);
+	bch2_btree_iter_verify(iter);
 	return k;
 }
 
@@ -1692,7 +1691,7 @@ struct bkey_s_c bch2_btree_iter_peek_with_updates(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_KEYS);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
 
 	while (1) {
 		ret = bch2_btree_iter_traverse(iter);
@@ -1714,8 +1713,8 @@ struct bkey_s_c bch2_btree_iter_peek_with_updates(struct btree_iter *iter)
 	}
 
 	/*
-	 * iter->pos should always be equal to the key we just
-	 * returned - except extents can straddle iter->pos:
+	 * iter->pos should be mononotically increasing, and always be equal to
+	 * the key we just returned - except extents can straddle iter->pos:
 	 */
 	if (bkey_cmp(bkey_start_pos(k.k), iter->pos) > 0)
 		iter->pos = bkey_start_pos(k.k);
@@ -1744,7 +1743,10 @@ struct bkey_s_c bch2_btree_iter_peek_prev(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_KEYS);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
+	bch2_btree_iter_verify_entry_exit(iter);
+
+	btree_iter_set_search_pos(iter, iter->pos);
 
 	if (iter->uptodate == BTREE_ITER_UPTODATE &&
 	    !bkey_deleted(&iter->k))
@@ -1752,8 +1754,10 @@ struct bkey_s_c bch2_btree_iter_peek_prev(struct btree_iter *iter)
 
 	while (1) {
 		ret = bch2_btree_iter_traverse(iter);
-		if (unlikely(ret))
-			return bkey_s_c_err(ret);
+		if (unlikely(ret)) {
+			k = bkey_s_c_err(ret);
+			goto no_key;
+		}
 
 		k = __btree_iter_peek(iter, l);
 		if (!k.k ||
@@ -1765,8 +1769,10 @@ struct bkey_s_c bch2_btree_iter_peek_prev(struct btree_iter *iter)
 		if (likely(k.k))
 			break;
 
-		if (!btree_iter_set_pos_to_prev_leaf(iter))
-			return bkey_s_c_null;
+		if (!btree_iter_set_pos_to_prev_leaf(iter)) {
+			k = bkey_s_c_null;
+			goto no_key;
+		}
 	}
 
 	EBUG_ON(bkey_cmp(bkey_start_pos(k.k), pos) > 0);
@@ -1774,10 +1780,23 @@ struct bkey_s_c bch2_btree_iter_peek_prev(struct btree_iter *iter)
 	/* Extents can straddle iter->pos: */
 	if (bkey_cmp(k.k->p, pos) < 0)
 		iter->pos = k.k->p;
+	iter->real_pos = k.k->p;
 	iter->uptodate = BTREE_ITER_UPTODATE;
-
-	bch2_btree_iter_verify_level(iter, 0);
+out:
+	bch2_btree_iter_verify_entry_exit(iter);
+	bch2_btree_iter_verify(iter);
 	return k;
+no_key:
+	/*
+	 * __btree_iter_peek() may have set iter->k to a key we didn't want, and
+	 * then we errored going to the previous leaf - make sure it's
+	 * consistent with iter->pos:
+	 */
+	BUG_ON(bkey_cmp(pos, iter->pos) &&
+	       bkey_cmp(iter->pos, POS_MIN));
+	bkey_init(&iter->k);
+	iter->k.p = iter->pos;
+	goto out;
 }
 
 /**
@@ -1830,7 +1849,7 @@ __bch2_btree_iter_peek_slot_extents(struct btree_iter *iter)
 		 */
 
 		EBUG_ON(bkey_cmp(k.k->p, iter->pos) <= 0);
-		bch2_btree_iter_verify_level(iter, 0);
+		bch2_btree_iter_verify(iter);
 		return k;
 	}
 
@@ -1853,7 +1872,9 @@ __bch2_btree_iter_peek_slot_extents(struct btree_iter *iter)
 	iter->k	= n;
 	iter->uptodate = BTREE_ITER_UPTODATE;
 
-	bch2_btree_iter_verify_level(iter, 0);
+	bch2_btree_iter_verify_entry_exit(iter);
+	bch2_btree_iter_verify(iter);
+
 	return (struct bkey_s_c) { &iter->k, NULL };
 }
 
@@ -1864,7 +1885,10 @@ struct bkey_s_c bch2_btree_iter_peek_slot(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_KEYS);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
+	bch2_btree_iter_verify_entry_exit(iter);
+
+	btree_iter_set_search_pos(iter, btree_iter_search_key(iter));
 
 	if (iter->uptodate == BTREE_ITER_UPTODATE)
 		return btree_iter_peek_uptodate(iter);
@@ -1888,7 +1912,8 @@ struct bkey_s_c bch2_btree_iter_peek_slot(struct btree_iter *iter)
 	}
 
 	iter->uptodate = BTREE_ITER_UPTODATE;
-	bch2_btree_iter_verify_level(iter, 0);
+	bch2_btree_iter_verify_entry_exit(iter);
+	bch2_btree_iter_verify(iter);
 	return k;
 }
 
@@ -1906,7 +1931,7 @@ struct bkey_s_c bch2_btree_iter_peek_cached(struct btree_iter *iter)
 	int ret;
 
 	EBUG_ON(btree_iter_type(iter) != BTREE_ITER_CACHED);
-	bch2_btree_iter_checks(iter);
+	bch2_btree_iter_verify(iter);
 
 	ret = bch2_btree_iter_traverse(iter);
 	if (unlikely(ret))
@@ -1937,6 +1962,7 @@ static inline void bch2_btree_iter_init(struct btree_trans *trans,
 	bkey_init(&iter->k);
 	iter->k.p			= pos;
 	iter->flags			= flags;
+	iter->real_pos			= btree_iter_search_key(iter);
 	iter->uptodate			= BTREE_ITER_NEED_TRAVERSE;
 	iter->btree_id			= btree_id;
 	iter->level			= 0;
@@ -2076,7 +2102,7 @@ static struct btree_iter *__btree_trans_get_iter(struct btree_trans *trans,
 
 		if (best &&
 		    bkey_cmp(bpos_diff(best->pos, pos),
-			     bpos_diff(iter->pos, pos)) < 0)
+			     bpos_diff(iter->real_pos, pos)) < 0)
 			continue;
 
 		best = iter;

fs/bcachefs/btree_types.h

@@ -247,6 +247,8 @@ enum btree_iter_uptodate {
 struct btree_iter {
 	struct btree_trans	*trans;
 	struct bpos		pos;
+	/* what we're searching for/what the iterator actually points to: */
+	struct bpos		real_pos;
 	struct bpos		pos_after_commit;
 
 	u16			flags;

fs/bcachefs/btree_update_leaf.c

@@ -219,7 +219,7 @@ static inline void btree_insert_entry_checks(struct btree_trans *trans,
 {
 	struct bch_fs *c = trans->c;
 
-	BUG_ON(bkey_cmp(insert->k.p, iter->pos));
+	BUG_ON(bkey_cmp(insert->k.p, iter->real_pos));
 	BUG_ON(bch2_debug_check_bkeys &&
 	       bch2_bkey_invalid(c, bkey_i_to_s_c(insert),
 				 __btree_node_type(iter->level, iter->btree_id)));