Re: [PATCH] bcachefs: update alloc cursor in early bucket allocator

[Brian Foster <bfoster@redhat.com>]

On Thu, Oct 19, 2023 at 02:38:03PM -0400, Kent Overstreet wrote:
> On Thu, Oct 19, 2023 at 01:22:38PM -0400, Brian Foster wrote:
> > BTW, should this code be protected from no free space situations at a
> > higher level, or should we consider a max retry count or something? I
> > want to be cautious about things like prospective livelocks
> > (particularly if this path is less common) if this retry was effectively
> > dead code due to not updating alloc_cursor.
> 
> It should be limiting itself to a single retry already: on retry, we set
> alloc_cursor = alloc_start, so we won't retry again. Just make sure that
> still works :)
> 

I don't think that actually works due to the whole alloc_cursor not
updating thing, but regardless the "single retry from the beginning"
logic makes a lot more sense to me. With that, here's what I'm currently
playing with:

diff --git a/fs/bcachefs/alloc_foreground.c b/fs/bcachefs/alloc_foreground.c
index 3bc4abd3d7d5..41a7bf24c440 100644
--- a/fs/bcachefs/alloc_foreground.c
+++ b/fs/bcachefs/alloc_foreground.c
@@ -402,8 +402,9 @@ bch2_bucket_alloc_early(struct btree_trans *trans,
 	struct btree_iter iter;
 	struct bkey_s_c k;
 	struct open_bucket *ob = NULL;
-	u64 alloc_start = max_t(u64, ca->mi.first_bucket, ca->new_fs_bucket_idx);
-	u64 alloc_cursor = max(alloc_start, READ_ONCE(ca->alloc_cursor));
+	u64 first_bucket = max_t(u64, ca->mi.first_bucket, ca->new_fs_bucket_idx);
+	u64 alloc_start = max(first_bucket, READ_ONCE(ca->alloc_cursor));
+	u64 alloc_cursor = alloc_start;
 	int ret;
 again:
 	for_each_btree_key_norestart(trans, iter, BTREE_ID_alloc, POS(ca->dev_idx, alloc_cursor),
@@ -431,13 +432,14 @@ bch2_bucket_alloc_early(struct btree_trans *trans,
 	}
 	bch2_trans_iter_exit(trans, &iter);
 
+	alloc_cursor = iter.pos.offset;
 	ca->alloc_cursor = alloc_cursor;
 
 	if (!ob && ret)
 		ob = ERR_PTR(ret);
 
-	if (!ob && alloc_cursor > alloc_start) {
-		alloc_cursor = alloc_start;
+	if (!ob && alloc_start > first_bucket) {
+		alloc_cursor = alloc_start = first_bucket;
 		goto again;
 	}