Re: [PATCH 1/1] bcache: process fewer btree nodes in incremental GC cycles

Thank you for your prompt feedback, Coly.

On Mon, Apr 14, 2025 at 7:08 PM Coly Li <[email protected]> wrote:
>
> On Mon, Apr 14, 2025 at 03:44:04PM +0800, Robert Pang wrote:
> > Current incremental GC processes a minimum of 100 btree nodes per cycle,
> > followed by a 100ms sleep. For NVMe cache devices, where the average node
> > processing time is ~1ms, this leads to front-side I/O latency potentially
> > reaching tens or hundreds of milliseconds during GC execution.
> >
> > This commit resolves this latency issue by reducing the minimum node processing
> > count per cycle to 10 and the inter-cycle sleep duration to 10ms. It also
> > integrates a check of existing GC statistics to re-scale the number of nodes
> > processed per sleep interval when needed, ensuring GC finishes well before the
> > next GC is due.
> >
> > Signed-off-by: Robert Pang <[email protected]>
> > ---
> >  drivers/md/bcache/btree.c | 38 +++++++++++++++++---------------------
> >  drivers/md/bcache/util.h  |  3 +++
> >  2 files changed, 20 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> > index ed40d8600656..093e1edcaa53 100644
> > --- a/drivers/md/bcache/btree.c
> > +++ b/drivers/md/bcache/btree.c
> > @@ -88,11 +88,8 @@
> >   * Test module load/unload
> >   */
> >
> > -#define MAX_NEED_GC          64
> > -#define MAX_SAVE_PRIO                72
>
> You may compose another patch for the above changes, to separte them
> from main idea of this patch.

Certainly, Just sent this as a separate patch.

> > -#define MAX_GC_TIMES         100
> > -#define MIN_GC_NODES         100
> > -#define GC_SLEEP_MS          100
> > +#define GC_MIN_NODES         10
> > +#define GC_SLEEP_MS          10
> >
> >  #define PTR_DIRTY_BIT                (((uint64_t) 1 << 36))
> >
> > @@ -1585,25 +1582,24 @@ 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_MIN_NODES;
> > +     uint64_t gc_max_ms = time_stat_average(&c->btree_gc_time, frequency, ms) / 2;
> >
> >       /*
> > -      * 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)
> > +      * The incremental garbage collector operates by processing
> > +      * GC_MIN_NODES at a time, pausing for GC_SLEEP_MS between
> > +      * each interval. If historical garbage collection statistics
> > +      * (btree_gc_time) is available, the maximum allowable GC
> > +      * duration is set to half of this observed frequency. To
> > +      * prevent exceeding this maximum duration, the number of
> > +      * nodes processed in the current step may be increased if
> > +      * the projected completion time based on the current pace
> > +      * extends beyond the allowed limit. This ensures timely GC
> > +      * completion before the next GC is due.
> >        */
> > -     min_nodes = c->gc_stats.nodes / MAX_GC_TIMES;
> > -     if (min_nodes < MIN_GC_NODES)
> > -             min_nodes = MIN_GC_NODES;
> > +     if ((gc_max_ms >= GC_SLEEP_MS) &&
> > +         (GC_SLEEP_MS * (c->gc_stats.nodes / min_nodes)) > gc_max_ms)
> > +             min_nodes = c->gc_stats.nodes / (gc_max_ms / GC_SLEEP_MS);
> >
>
> Is it possible that gc_max_ms becomes 0?

Yes, gc_max_ms can be 0 initially when the cache is set up and stats are not
collected yet. In that case, the check "gc_max_ms >= GC_SLEEP_MS" fails and
we process at the default rate of 10 nodes per cycle.

Importantly, this same check also serves to prevent a division-by-zero error
when the number of nodes is re-scaled using the following calculation:

  min_nodes = c->gc_stats.nodes / (gc_max_ms / GC_SLEEP_MS);

>
> >       return min_nodes;
> >  }
> > diff --git a/drivers/md/bcache/util.h b/drivers/md/bcache/util.h
> > index 539454d8e2d0..21a370f444b7 100644
> > --- a/drivers/md/bcache/util.h
> > +++ b/drivers/md/bcache/util.h
> > @@ -305,6 +305,9 @@ static inline unsigned int local_clock_us(void)
> >  #define NSEC_PER_ms                  NSEC_PER_MSEC
> >  #define NSEC_PER_sec                 NSEC_PER_SEC
> >
> > +#define time_stat_average(stats, stat, units)                                \
> > +     div_u64((stats)->average_ ## stat >> 8, NSEC_PER_ ## units)
> > +
>
> Could you please add a few code comments here to explain what does
> time_stat_average() do?

Will add the code comments with explanation in a new version promptly.

>
> Thanks in advance.
>
> >  #define __print_time_stat(stats, name, stat, units)                  \
> >       sysfs_print(name ## _ ## stat ## _ ## units,                    \
> >                   div_u64((stats)->stat >> 8, NSEC_PER_ ## units))
> > --
> > 2.49.0.604.gff1f9ca942-goog
> >
>
> --
> Coly Li