On Tue, Nov 19, 2024 at 03:40:29PM +0800, mingzhe.zou@xxxxxxxxxxxx wrote:
> From: Mingzhe Zou <mingzhe.zou@xxxxxxxxxxxx>
>
> 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 <mingzhe.zou@xxxxxxxxxxxx>
> ---
> 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
