Re: [PATCH 5/9] bcache: reduce gc latency by processing less nodes and sleep less time

Hi Coly

Thank you so much for taking the time to look into this and share your
insights and ease my concerns. Your explanation in this thread will
provide sufficient context for future reference, so I think we can
save the additional code comments.

Best regards
Robert Pang

On Thu, Aug 6, 2026 at 1:36 AM Coly Li <[email protected]> wrote:
>
> On Tue, Jul 21, 2026 at 11:55:53AM +0800, Robert Pang wrote:
> > On Fri, Jul 17, 2026 at 9:20 PM Coly Li <[email protected]> wrote:
> > >
> > > > 2026年7月17日 08:50，Robert Pang <[email protected]> 写道：
> > > >
> > > > Hi Coly,
> > > >
> > > > I was reviewing this patch again and noticed that btree_gc_min_nodes() reads the
> > > > atomic counter 'search_inflight'. This counter is incremented and decremented by
> > > > clients issuing front-side I/O. In this scenario, should we add a memory barrier
> > > > (such as `smp_mb__before_atomic`) prior to `atomic_read()`?
> > > >
> > > > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > > > btree_gc_recurse(), the compiler might hoist the `atomic_read()` outside of the
> > > > while loop where btree_gc_min_nodes() is inlined, preventing us from fetching the
> > > > latest counter value. Adding the barrier would guarantee we read the updated
> > > > value.
> > > >
> > > > What are your thoughts on this?
> > >
> > > Hi Robert,
> > >
> > > At the first glance I feel the code was fine. But, it was almost 8 months ago, to
> > > make sure I understand you correctly, can you place your comments with the exact
> > > code together, then let me response you more accurately.
> > >
> > > Thanks for the review.
> > >
> > > Coly Li
> >
> > Hi Coly
> >
> > My apology for missing the code context in the earlier email. Here are
> > the code snippets where my comments relate:
> >
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 210b59007d98..5d922d301ab6 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> >
> > @@ -1578,29 +1579,29 @@ static unsigned int btree_gc_count_keys(struct btree *b)
> >
> >  static size_t btree_gc_min_nodes(struct cache_set *c)
> >  {
> > - size_t min_nodes;
> > + size_t min_nodes = GC_NODES_MIN;
> >
> > - /*
> > - * Since incremental GC would stop 100ms when front
> > - * side I/O comes, so when there are many btree nodes,
> > - * if GC only processes constant (100) nodes each time,
> > - * GC would last a long time, and the front side I/Os
> > - * would run out of the buckets (since no new bucket
> > - * can be allocated during GC), and be blocked again.
> > - * So GC should not process constant nodes, but varied
> > - * nodes according to the number of btree nodes, which
> > - * realized by dividing GC into constant(100) times,
> > - * so when there are many btree nodes, GC can process
> > - * more nodes each time, otherwise, GC will process less
> > - * nodes each time (but no less than MIN_GC_NODES)
> > - */
> > - min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
> > - if (min_nodes < MIN_GC_NODES)
> > - min_nodes = MIN_GC_NODES;
> > + if (atomic_read(&c->search_inflight) == 0) {
> > + size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
> > +
>
> If atomic_read(&c->search_inflight) == 0, it means currently
> no front end I/O. Then the gc nodes can be a bit more for a
> more aggressive garbage collection.
>
> Because the front end I/Os compete btree locks with gc threads,
> I set a more aggresive garbage collection only when there is no
> front I/O coming.
>
>
> > + if (min_nodes < n)
> > + min_nodes = n;
> > + }
> >
> >   return min_nodes;
> >  }
> >
> > In the above code change of this patch, btree_gc_min_nodes() reads the
> > atomic counter 'search_inflight'. And this counter is incremented and
> > decremented by clients issuing front-side I/O below:
> >
> > drivers/md/bcache/request.c:
> >
> > static CLOSURE_CALLBACK(search_free)
> > {
> >   closure_type(s, struct search, cl);
> >
> >   atomic_dec(&s->iop.c->search_inflight);
> >
> >   if (s->iop.bio)
> >     bio_put(s->iop.bio);
> >
> >   bio_complete(s);
> >   closure_debug_destroy(cl);
> >   mempool_free(s, &s->iop.c->search);
> > }
> >
> > static inline struct search *search_alloc(struct bio *bio,
> > struct bcache_device *d, struct block_device *orig_bdev,
> > unsigned long start_time)
> > {
> >   struct search *s;
> >
> >   s = mempool_alloc(&d->c->search, GFP_NOIO);
> >
> >   closure_init(&s->cl, NULL);
> >   do_bio_hook(s, bio, request_endio);
> >   atomic_inc(&d->c->search_inflight);
> >   ...
> > }
> >
> > My concern is that if btree_gc_min_nodes() is inlined in the caller
> > btree_gc_recurse() below, the compiler might hoist the `atomic_read()`
> > outside of the while loop where btree_gc_min_nodes() is inlined,
> > preventing us from fetching the latest counter value after each btree
> > node is traversed.
> >
>
> It is possible, but I do this on purpose. Because accurately catching
> zero inflight counter is unncessary. Let me explain in next text block.
>
> > drivers/md/bcache/btree.c:
> >
> > static int btree_gc_recurse(struct btree *b, struct btree_op *op,
> >     struct closure *writes, struct gc_stat *gc)
> > {
> >   int ret = 0;
> >   bool should_rewrite;
> >   struct bkey *k;
> >   struct btree_iter_stack iter;
> >   struct gc_merge_info r[GC_MERGE_NODES];
> >   struct gc_merge_info *i, *last = r + ARRAY_SIZE(r) - 1;
> >
> >   bch_btree_iter_stack_init(&b->keys, &iter, &b->c->gc_done);
> >
> >   for (i = r; i < r + ARRAY_SIZE(r); i++)
> >     i->b = ERR_PTR(-EINTR);
> >
> >   while (1) {
> >     k = bch_btree_iter_next_filter(&iter.iter, &b->keys,
> >        bch_ptr_bad);
> >     ...
> >     if (gc->nodes >= (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
> >       gc->nodes_pre =  gc->nodes;
> >       ret = -EAGAIN;
> >       break;
> >     }
> >     ..
> > }
> >
> > In this scenario, should we add a memory barrier (such as
> > `smp_mb__before_atomic`) prior to `atomic_read()` to guarantee we read
> > the updated value? I.e.
> >
>
> Such memory barrier hurts performance, and for hot I/O path, it might
> introduce obviouos negative performance impect. Fortunately for the
> condition you are concerned, it works well without memory barrier.
>
>
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index 377052cbde5c..659b8706031a 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> > @@ -1582,6 +1582,8 @@ static size_t btree_gc_min_nodes(struct cache_set *c)
> >  {
> >         size_t min_nodes = GC_NODES_MIN;
> >
> > +       /* Fetch latest search_inflight count */
> > +       smp_mb__before_atomic();
> >         if (atomic_read(&c->search_inflight) == 0) {
> >                 size_t n = c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
> >
> > What are your thoughts on this?
>
> The motivation is, when the bcache device is idle (no front end I/Os),
> make garbage collection be more aggressive by scanning more btree nodes
> in each iteration. This is a try-best effort, the inflight counter is
> unncessary to be accurate 0. If the check in btree_gc_min_nodes(),
>         atomic_read(&c->search_inflight) == 0
> fails, then just fails. And if the bcache device is really idle for a
> while, finally the above check will be true and a larger min_nodes will
> be returned from btree_gc_min_nodes().
>
> But if adding a single memory barrier only in btree_gc_min_nodes(), indeed
> it does't help too much. Memory barrier is also necessary for locations
> where the inflight counter are changed. Then this is why I name it as
> negative performance impact in previous text block.
>
> Maybe I need to add code comments to explain why memory barrier is
> unncessary in current btree_gc_min_nodes(). Does it help?
>
> Thanks for the quesiton.
>
> Coly Li