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

[Brian Foster <bfoster@redhat.com>]

On Thu, Oct 19, 2023 at 11:30:19AM -0400, Kent Overstreet wrote:
> On Thu, Oct 19, 2023 at 09:27:46AM -0400, Brian Foster wrote:
> > A recent bug report uncovered a scenario where a filesystem never
> > runs with freespace_initialized, and therefore the user observes
> > significantly degraded write performance by virtue of running the
> > early bucket allocator. The associated bug aside, the primary cause
> > of the performance drop in this particular instance is that the
> > early bucket allocator does not update the allocation cursor. This
> > means that every allocation walks the alloc btree from the first
> > bucket of the associated device looking for a bucket marked as free
> > space.
> > 
> > Update the early allocator code to set the alloc cursor to the
> > prospectively allocated bucket, similar to how the freelist
> > allocator behaves. This improves performance of the early bucket
> > allocator dramatically (even though it should be bypassed in favor
> > of the freelist allocator in most cases).
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> > 
> > cshepherd on #bcache originally reported the early bucket allocator
> > problem and helped chase it down to what looks like a members_v2
> > regression. I believe he was planning to post a patch for that one.
> > 
> > Brian
> > 
> >  fs/bcachefs/alloc_foreground.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/bcachefs/alloc_foreground.c b/fs/bcachefs/alloc_foreground.c
> > index 3bc4abd3d7d5..be3fc0f38c79 100644
> > --- a/fs/bcachefs/alloc_foreground.c
> > +++ b/fs/bcachefs/alloc_foreground.c
> > @@ -431,7 +431,7 @@ bch2_bucket_alloc_early(struct btree_trans *trans,
> >  	}
> >  	bch2_trans_iter_exit(trans, &iter);
> >  
> > -	ca->alloc_cursor = alloc_cursor;
> > +	ca->alloc_cursor = IS_ERR_OR_NULL(ob) ? alloc_cursor : ob->bucket;
> 
> Oh, this code is broken. The local alloc_cursor never gets updated, and
> it needs to for where we check if we need to loop around.
> 

Ah, good point.

> The proper fix would be to add
> 
>   alloc_cursor = iter.pos.offset;
> 
> before the line you changed
> 

Thanks. I'll give that a whirl.

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.

Brian