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

Thank you for the positive feedback and your review.

Regarding testing, the 24-hour fio test is currently underway on a 6TB SSD
with an iodepth set to 128. I will provide an update with the findings upon
its conclusion.

On Wed, Apr 16, 2025 at 2:44 AM Coly Li <[email protected]> wrote:
>
>
>
> > 2025年4月16日 01:25，Robert Pang <[email protected]> 写道：
> >
> > 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);
> >
> >>
>
> Thanks. So the code itself I don’t have more comment, just look forward to more testing and benchmark results.
>
> Thanks.
>
> Coly Li
>
>
>
>
> >>>      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
>
>