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

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.

> -#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?


>  	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?


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