Re: [PATCH v2 1/3] bcache: avoid invalidating buckets in use

On Tue, Nov 19, 2024 at 03:40:29PM +0800, [email protected] wrote:
> From: Mingzhe Zou <[email protected]>
> 
> If the bucket was reused while our bio was in flight, we might
> have read the wrong data. Currently, we will reread the data from
> the backing device. This not only reduces performance, but also
> makes the process more complex.
> 
> When the bucket is in use, we hope not to reclaim it.
>

No please don't do this.

This is the essential designment of bcache buckets, concurrent access
is expected and permitted. Locked the bucket and prohibit detectable
race is a huge change to the whole code logic, may introduce potential
issue which I am not able to tell immediately.

I know although the upper layer code or application should retry with
an I/O error code received, but most of the cases it is very hard to
modify the deployed software. Maybe a feasible fix is to set the error
code to AGAIN, and before returning to upper layer code if the error
code is AGAIN other than ERROR, re-submit the bio again from a worker.

Anyway, this is just a quite rough idea, I don't think over it
carefully.

Thanks.

Coly Li


 
> Signed-off-by: Mingzhe Zou <[email protected]>
> ---
>  drivers/md/bcache/alloc.c  | 32 +++++++++++++++++++++++---------
>  drivers/md/bcache/bcache.h |  3 ++-
>  2 files changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
> index da50f6661bae..18441aa74229 100644
> --- a/drivers/md/bcache/alloc.c
> +++ b/drivers/md/bcache/alloc.c
> @@ -134,25 +134,41 @@ bool bch_can_invalidate_bucket(struct cache *ca, struct bucket *b)
>  	       !atomic_read(&b->pin) && can_inc_bucket_gen(b));
>  }
>  
> -void __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
> +bool __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
>  {
>  	lockdep_assert_held(&ca->set->bucket_lock);
>  	BUG_ON(GC_MARK(b) && GC_MARK(b) != GC_MARK_RECLAIMABLE);
>  
> +	if (!spin_trylock(&b->lock))
> +		return false;
> +
> +	/*
> +	 * If the bucket was reused while read bio was in flight, it will
> +	 * reread the data from the backing device. This will increase latency
> +	 * and cause other errors. When b->pin is not 0, do not invalidate
> +	 * the bucket.
> +	 */
> +
> +	if (atomic_inc_return(&b->pin) > 1) {
> +		atomic_dec(&b->pin);
> +		spin_unlock(&b->lock);
> +		return false;
> +	}
> +
>  	if (GC_SECTORS_USED(b))
>  		trace_bcache_invalidate(ca, b - ca->buckets);
>  
>  	bch_inc_gen(ca, b);
>  	b->prio = INITIAL_PRIO;
> -	atomic_inc(&b->pin);
>  	b->reclaimable_in_gc = 0;
> +	spin_unlock(&b->lock);
> +	return true;
>  }
>  
>  static void bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
>  {
> -	__bch_invalidate_one_bucket(ca, b);
> -
> -	fifo_push(&ca->free_inc, b - ca->buckets);
> +	if (bch_can_invalidate_bucket(ca, b) && __bch_invalidate_one_bucket(ca, b))
> +		fifo_push(&ca->free_inc, b - ca->buckets);
>  }
>  
>  /*
> @@ -253,8 +269,7 @@ static void invalidate_buckets_fifo(struct cache *ca)
>  
>  		b = ca->buckets + ca->fifo_last_bucket++;
>  
> -		if (bch_can_invalidate_bucket(ca, b))
> -			bch_invalidate_one_bucket(ca, b);
> +		bch_invalidate_one_bucket(ca, b);
>  
>  		if (++checked >= ca->sb.nbuckets) {
>  			ca->invalidate_needs_gc = 1;
> @@ -279,8 +294,7 @@ static void invalidate_buckets_random(struct cache *ca)
>  
>  		b = ca->buckets + n;
>  
> -		if (bch_can_invalidate_bucket(ca, b))
> -			bch_invalidate_one_bucket(ca, b);
> +		bch_invalidate_one_bucket(ca, b);
>  
>  		if (++checked >= ca->sb.nbuckets / 2) {
>  			ca->invalidate_needs_gc = 1;
> diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
> index 785b0d9008fa..fc7f10c5f222 100644
> --- a/drivers/md/bcache/bcache.h
> +++ b/drivers/md/bcache/bcache.h
> @@ -196,6 +196,7 @@
>  
>  struct bucket {
>  	atomic_t	pin;
> +	spinlock_t	lock;
>  	uint16_t	prio;
>  	uint8_t		gen;
>  	uint8_t		last_gc; /* Most out of date gen in the btree */
> @@ -981,7 +982,7 @@ uint8_t bch_inc_gen(struct cache *ca, struct bucket *b);
>  void bch_rescale_priorities(struct cache_set *c, int sectors);
>  
>  bool bch_can_invalidate_bucket(struct cache *ca, struct bucket *b);
> -void __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b);
> +bool __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b);
>  
>  void __bch_bucket_free(struct cache *ca, struct bucket *b);
>  void bch_bucket_free(struct cache_set *c, struct bkey *k);
> -- 
> 2.34.1
> 
> 

-- 
Coly Li