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

[Brian Foster]

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;
        }