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 <[email protected]> > > --- > > > > 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
