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

On Fri, Jul 17, 2026 at 9:20=E2=80=AFPM Coly Li <[email protected]> wrote:
>
> > 2026=E5=B9=B47=E6=9C=8817=E6=97=A5 08:50=EF=BC=8CRobert Pang <robertpan=
[email protected]> =E5=86=99=E9=81=93=EF=BC=9A
> >
> > 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 decre=
mented by
> > clients issuing front-side I/O. In this scenario, should we add a memor=
y 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()` outsid=
e of the
> > while loop where btree_gc_min_nodes() is inlined, preventing us from fe=
tching the
> > latest counter value. Adding the barrier would guarantee we read the up=
dated
> > 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 th=
e 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 btre=
e *b)

 static size_t btree_gc_min_nodes(struct cache_set *c)
 {
- size_t min_nodes;
+ size_t min_nodes =3D 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 =3D c->gc_stats.nodes / MAX_GC_TIMES;
- if (min_nodes < MIN_GC_NODES)
- min_nodes =3D MIN_GC_NODES;
+ if (atomic_read(&c->search_inflight) =3D=3D 0) {
+ size_t n =3D c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;
+
+ if (min_nodes < n)
+ min_nodes =3D 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 =3D 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.

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 =3D 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 =3D r + ARRAY_SIZE(r) - 1;

  bch_btree_iter_stack_init(&b->keys, &iter, &b->c->gc_done);

  for (i =3D r; i < r + ARRAY_SIZE(r); i++)
    i->b =3D ERR_PTR(-EINTR);

  while (1) {
    k =3D bch_btree_iter_next_filter(&iter.iter, &b->keys,
       bch_ptr_bad);
    ...
    if (gc->nodes >=3D (gc->nodes_pre + btree_gc_min_nodes(b->c))) {
      gc->nodes_pre =3D  gc->nodes;
      ret =3D -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.

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 =3D GC_NODES_MIN;

+       /* Fetch latest search_inflight count */
+       smp_mb__before_atomic();
        if (atomic_read(&c->search_inflight) =3D=3D 0) {
                size_t n =3D c->gc_stats.nodes >> MAX_GC_TIMES_SHIFT;

What are your thoughts on this?

Best regards
Robert Pang