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

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