[PATCH 0/3] bcache-6.10 20240528

[PATCH 0/3] bcache-6.10 20240528

[Coly Li]

Hi Jens,

This series is just for the patch from Dongsheng Yang, due to more
testing needed, it comes after the first wave.

Dongsheng's patch helps to improve the latency of cache space
allocation. This patch has been shipped in product more than one year
by his team. Robert Pang from Google introduces this patch has been
tested inside Google with quite extended hardware configurations. Eric
Wheeler also deploys this patch in his production for 1+ months.

So far there is no issue reported for this patch, except for a bug in
existed code and easier to be trigger by Dongsheng's patch. This is why
my first patch is composed. Another patch from me is just a code
cleanup cought my eyes during the fix.

Please consider to take them, and thank you in advance.

Coly Li
---

Coly Li (2):
  bcache: call force_wake_up_gc() if necessary in check_should_bypass()
  bcache: code cleanup in __bch_bucket_alloc_set()

Dongsheng Yang (1):
  bcache: allow allocator to invalidate bucket in gc

 drivers/md/bcache/alloc.c   | 21 +++++++--------------
 drivers/md/bcache/bcache.h  |  1 +
 drivers/md/bcache/btree.c   |  7 ++++++-
 drivers/md/bcache/request.c | 16 +++++++++++++++-
 4 files changed, 29 insertions(+), 16 deletions(-)

-- 
2.35.3

[PATCH 1/3] bcache: allow allocator to invalidate bucket in gc

[Coly Li]

From: Dongsheng Yang <dongsheng.yang@easystack.cn>

Currently, if the gc is running, when the allocator found free_inc
is empty, allocator has to wait the gc finish. Before that, the
IO is blocked.

But actually, there would be some buckets is reclaimable before gc,
and gc will never mark this kind of bucket to be unreclaimable.

So we can put these buckets into free_inc in gc running to avoid
IO being blocked.

Signed-off-by: Dongsheng Yang <dongsheng.yang@easystack.cn>
Signed-off-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/alloc.c  | 13 +++++--------
 drivers/md/bcache/bcache.h |  1 +
 drivers/md/bcache/btree.c  |  7 ++++++-
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index ce13c272c387..32a46343097d 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -129,12 +129,9 @@ static inline bool can_inc_bucket_gen(struct bucket *b)
 
 bool bch_can_invalidate_bucket(struct cache *ca, struct bucket *b)
 {
-	BUG_ON(!ca->set->gc_mark_valid);
-
-	return (!GC_MARK(b) ||
-		GC_MARK(b) == GC_MARK_RECLAIMABLE) &&
-		!atomic_read(&b->pin) &&
-		can_inc_bucket_gen(b);
+	return (ca->set->gc_mark_valid || b->reclaimable_in_gc) &&
+	       ((!GC_MARK(b) || GC_MARK(b) == GC_MARK_RECLAIMABLE) &&
+	       !atomic_read(&b->pin) && can_inc_bucket_gen(b));
 }
 
 void __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
@@ -148,6 +145,7 @@ void __bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
 	bch_inc_gen(ca, b);
 	b->prio = INITIAL_PRIO;
 	atomic_inc(&b->pin);
+	b->reclaimable_in_gc = 0;
 }
 
 static void bch_invalidate_one_bucket(struct cache *ca, struct bucket *b)
@@ -352,8 +350,7 @@ static int bch_allocator_thread(void *arg)
 		 */
 
 retry_invalidate:
-		allocator_wait(ca, ca->set->gc_mark_valid &&
-			       !ca->invalidate_needs_gc);
+		allocator_wait(ca, !ca->invalidate_needs_gc);
 		invalidate_buckets(ca);
 
 		/*
diff --git a/drivers/md/bcache/bcache.h b/drivers/md/bcache/bcache.h
index 4e6afa89921f..1d33e40d26ea 100644
--- a/drivers/md/bcache/bcache.h
+++ b/drivers/md/bcache/bcache.h
@@ -200,6 +200,7 @@ struct bucket {
 	uint8_t		gen;
 	uint8_t		last_gc; /* Most out of date gen in the btree */
 	uint16_t	gc_mark; /* Bitfield used by GC. See below for field */
+	uint16_t	reclaimable_in_gc:1;
 };
 
 /*
diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index d011a7154d33..4e6ccf2c8a0b 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -1741,18 +1741,20 @@ static void btree_gc_start(struct cache_set *c)
 
 	mutex_lock(&c->bucket_lock);
 
-	c->gc_mark_valid = 0;
 	c->gc_done = ZERO_KEY;
 
 	ca = c->cache;
 	for_each_bucket(b, ca) {
 		b->last_gc = b->gen;
+		if (bch_can_invalidate_bucket(ca, b))
+			b->reclaimable_in_gc = 1;
 		if (!atomic_read(&b->pin)) {
 			SET_GC_MARK(b, 0);
 			SET_GC_SECTORS_USED(b, 0);
 		}
 	}
 
+	c->gc_mark_valid = 0;
 	mutex_unlock(&c->bucket_lock);
 }
 
@@ -1809,6 +1811,9 @@ static void bch_btree_gc_finish(struct cache_set *c)
 	for_each_bucket(b, ca) {
 		c->need_gc	= max(c->need_gc, bucket_gc_gen(b));
 
+		if (b->reclaimable_in_gc)
+			b->reclaimable_in_gc = 0;
+
 		if (atomic_read(&b->pin))
 			continue;
 
-- 
2.35.3

[PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Coly Li]

If there are extreme heavy write I/O continuously hit on relative small
cache device (512GB in my testing), it is possible to make counter
c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.

If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
requests will bypass the cache device because check_should_bypass()
returns 'true'. Because all writes bypass the cache device, counter
c->sectors_to_gc has no chance to be negative value, and garbage
collection thread won't be waken up even the whole cache becomes clean
after writeback accomplished. The aftermath is that all write I/Os go
directly into backing device even the cache device is clean.

To avoid the above situation, this patch uses a quite conservative way
to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
up garbage collection thread when the whole cache device is clean.

Before the fix, the writes-always-bypass situation happens after 10+
hours write I/O pressure on 512GB Intel optane memory which acts as
cache device. After this fix, such situation doesn't happen after 36+
hours testing.

Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/request.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 83d112bd2b1c..af345dc6fde1 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -369,10 +369,24 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	struct io *i;
 
 	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
-	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
 	    (bio_op(bio) == REQ_OP_DISCARD))
 		goto skip;
 
+	if (c->gc_stats.in_use > CUTOFF_CACHE_ADD) {
+		/*
+		 * If cached buckets are all clean now, 'true' will be
+		 * returned and all requests will bypass the cache device.
+		 * Then c->sectors_to_gc has no chance to be negative, and
+		 * gc thread won't wake up and caching won't work forever.
+		 * Here call force_wake_up_gc() to avoid such aftermath.
+		 */
+		if (BDEV_STATE(&dc->sb) == BDEV_STATE_CLEAN &&
+		    c->gc_mark_valid)
+			force_wake_up_gc(c);
+
+		goto skip;
+	}
+
 	if (mode == CACHE_MODE_NONE ||
 	    (mode == CACHE_MODE_WRITEAROUND &&
 	     op_is_write(bio_op(bio))))
-- 
2.35.3

[PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set()

[Coly Li]

In __bch_bucket_alloc_set() the lines after lable 'err:' indeed do
nothing useful after multiple cache devices are removed from bcache
code. This cleanup patch drops the useless code to save a bit CPU
cycles.

Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/alloc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/md/bcache/alloc.c b/drivers/md/bcache/alloc.c
index 32a46343097d..48ce750bf70a 100644
--- a/drivers/md/bcache/alloc.c
+++ b/drivers/md/bcache/alloc.c
@@ -498,8 +498,8 @@ int __bch_bucket_alloc_set(struct cache_set *c, unsigned int reserve,
 
 	ca = c->cache;
 	b = bch_bucket_alloc(ca, reserve, wait);
-	if (b == -1)
-		goto err;
+	if (b < 0)
+		return -1;
 
 	k->ptr[0] = MAKE_PTR(ca->buckets[b].gen,
 			     bucket_to_sector(c, b),
@@ -508,10 +508,6 @@ int __bch_bucket_alloc_set(struct cache_set *c, unsigned int reserve,
 	SET_KEY_PTRS(k, 1);
 
 	return 0;
-err:
-	bch_bucket_free(c, k);
-	bkey_put(c, k);
-	return -1;
 }
 
 int bch_bucket_alloc_set(struct cache_set *c, unsigned int reserve,
-- 
2.35.3

Re: [PATCH 0/3] bcache-6.10 20240528

[Jens Axboe]

On Tue, 28 May 2024 20:09:11 +0800, Coly Li wrote:
> This series is just for the patch from Dongsheng Yang, due to more
> testing needed, it comes after the first wave.
> 
> Dongsheng's patch helps to improve the latency of cache space
> allocation. This patch has been shipped in product more than one year
> by his team. Robert Pang from Google introduces this patch has been
> tested inside Google with quite extended hardware configurations. Eric
> Wheeler also deploys this patch in his production for 1+ months.
> 
> [...]

Applied, thanks!

[1/3] bcache: allow allocator to invalidate bucket in gc
      commit: a14a68b76954e73031ca6399abace17dcb77c17a
[2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
      commit: 05356938a4be356adde4eab4425c6822f3c7d706
[3/3] bcache: code cleanup in __bch_bucket_alloc_set()
      commit: 74d4ce92e08d5669d66fd890403724faa4286c21

Best regards,
-- 
Jens Axboe

[PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Coly Li]

If there are extreme heavy write I/O continuously hit on relative small
cache device (512GB in my testing), it is possible to make counter
c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.

If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
requests will bypass the cache device because check_should_bypass()
returns 'true'. Because all writes bypass the cache device, counter
c->sectors_to_gc has no chance to be negative value, and garbage
collection thread won't be waken up even the whole cache becomes clean
after writeback accomplished. The aftermath is that all write I/Os go
directly into backing device even the cache device is clean.

To avoid the above situation, this patch uses a quite conservative way
to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
up garbage collection thread when the whole cache device is clean.

Before the fix, the writes-always-bypass situation happens after 10+
hours write I/O pressure on 512GB Intel optane memory which acts as
cache device. After this fix, such situation doesn't happen after 36+
hours testing.

Signed-off-by: Coly Li <colyli@suse.de>
---
 drivers/md/bcache/request.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
index 83d112bd2b1c..af345dc6fde1 100644
--- a/drivers/md/bcache/request.c
+++ b/drivers/md/bcache/request.c
@@ -369,10 +369,24 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
 	struct io *i;
 
 	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
-	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
 	    (bio_op(bio) == REQ_OP_DISCARD))
 		goto skip;
 
+	if (c->gc_stats.in_use > CUTOFF_CACHE_ADD) {
+		/*
+		 * If cached buckets are all clean now, 'true' will be
+		 * returned and all requests will bypass the cache device.
+		 * Then c->sectors_to_gc has no chance to be negative, and
+		 * gc thread won't wake up and caching won't work forever.
+		 * Here call force_wake_up_gc() to avoid such aftermath.
+		 */
+		if (BDEV_STATE(&dc->sb) == BDEV_STATE_CLEAN &&
+		    c->gc_mark_valid)
+			force_wake_up_gc(c);
+
+		goto skip;
+	}
+
 	if (mode == CACHE_MODE_NONE ||
 	    (mode == CACHE_MODE_WRITEAROUND &&
 	     op_is_write(bio_op(bio))))
-- 
2.35.3

Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Eric Wheeler]

On Tue, 28 May 2024, Coly Li wrote:

> If there are extreme heavy write I/O continuously hit on relative small
> cache device (512GB in my testing), it is possible to make counter
> c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.
> 
> If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
> requests will bypass the cache device because check_should_bypass()
> returns 'true'. Because all writes bypass the cache device, counter
> c->sectors_to_gc has no chance to be negative value, and garbage
> collection thread won't be waken up even the whole cache becomes clean
> after writeback accomplished. The aftermath is that all write I/Os go
> directly into backing device even the cache device is clean.
> 
> To avoid the above situation, this patch uses a quite conservative way
> to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
> up garbage collection thread when the whole cache device is clean.

Nice fix.

If I understand correctly, even with this fix, bcache can reach a point 
where it must wait until garbage collection frees a bucket (via 
force_wake_up_gc) before buckets can be used again.  Waiting to call 
force_wake_up_gc until `c->gc_stats.in_use` exceeds CUTOFF_CACHE_ADD may 
not respond as fast as it could, and IO latency is important.

It may be a good idea to do `c->gc_stats.in_use > CUTOFF_CACHE_ADD/2` to
start garbage collection when it is half-way "full".

Reaching 50% is still quite conservative, but if you want to wait longer, 
then even 80% or 90% would be fine; however, I think 100% is too far.  We 
want to avoid the case where bcache is completely "out" of buckets and we 
have to wait for garbage collection latency before a cache bucket can 
fill, since buckets should be available.

For example on our system we have 736824 buckets available:
	# cat /sys/devices/virtual/block/dm-9/bcache/nbuckets
	736824

There should be no reason to wait until all buckets are exhausted. Forcing 
garbage collection at 50% (368412 buckets "in use") would be good house 
keeping.

You know this code very well so if I have misinterpreted something here, 
then please fill me in on the details.

--
Eric Wheeler


> 
> Before the fix, the writes-always-bypass situation happens after 10+
> hours write I/O pressure on 512GB Intel optane memory which acts as
> cache device. After this fix, such situation doesn't happen after 36+
> hours testing.
> 
> Signed-off-by: Coly Li <colyli@suse.de>
> ---
>  drivers/md/bcache/request.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/request.c b/drivers/md/bcache/request.c
> index 83d112bd2b1c..af345dc6fde1 100644
> --- a/drivers/md/bcache/request.c
> +++ b/drivers/md/bcache/request.c
> @@ -369,10 +369,24 @@ static bool check_should_bypass(struct cached_dev *dc, struct bio *bio)
>  	struct io *i;
>  
>  	if (test_bit(BCACHE_DEV_DETACHING, &dc->disk.flags) ||
> -	    c->gc_stats.in_use > CUTOFF_CACHE_ADD ||
>  	    (bio_op(bio) == REQ_OP_DISCARD))
>  		goto skip;
>  
> +	if (c->gc_stats.in_use > CUTOFF_CACHE_ADD) {
> +		/*
> +		 * If cached buckets are all clean now, 'true' will be
> +		 * returned and all requests will bypass the cache device.
> +		 * Then c->sectors_to_gc has no chance to be negative, and
> +		 * gc thread won't wake up and caching won't work forever.
> +		 * Here call force_wake_up_gc() to avoid such aftermath.
> +		 */
> +		if (BDEV_STATE(&dc->sb) == BDEV_STATE_CLEAN &&
> +		    c->gc_mark_valid)
> +			force_wake_up_gc(c);
> +
> +		goto skip;
> +	}
> +
>  	if (mode == CACHE_MODE_NONE ||
>  	    (mode == CACHE_MODE_WRITEAROUND &&
>  	     op_is_write(bio_op(bio))))
> -- 
> 2.35.3
> 
> 
> 

Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Coly Li]

> 2024年5月28日 06:31，Eric Wheeler <bcache@lists.ewheeler.net> 写道：
> 
> On Tue, 28 May 2024, Coly Li wrote:
> 
>> If there are extreme heavy write I/O continuously hit on relative small
>> cache device (512GB in my testing), it is possible to make counter
>> c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.
>> 
>> If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
>> requests will bypass the cache device because check_should_bypass()
>> returns 'true'. Because all writes bypass the cache device, counter
>> c->sectors_to_gc has no chance to be negative value, and garbage
>> collection thread won't be waken up even the whole cache becomes clean
>> after writeback accomplished. The aftermath is that all write I/Os go
>> directly into backing device even the cache device is clean.
>> 
>> To avoid the above situation, this patch uses a quite conservative way
>> to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
>> up garbage collection thread when the whole cache device is clean.
> 
> Nice fix.
> 
> If I understand correctly, even with this fix, bcache can reach a point 
> where it must wait until garbage collection frees a bucket (via 
> force_wake_up_gc) before buckets can be used again.  Waiting to call 
> force_wake_up_gc until `c->gc_stats.in_use` exceeds CUTOFF_CACHE_ADD may 
> not respond as fast as it could, and IO latency is important.
> 

CUTOFF_CACHE_ADD is not for this purpose.
GC is triggered by c->sectors_to_gc, it works as
- initialized as 1/16 size of cache device.
- every allocation decreases cached size from it.
- once c->sectors_go_gc is negative value, wakeup gc thread and reset the value to 1/16 size of cache device.

CUTOFF_CACHE_ADD is to avoid something like no-space deadlock in cache space. If cache space is allocated more than CUTOFF_CACHE_ADD (95%), cache space will not be allocated out anymore and all read/write will bypass and go directly into backing device. In my testing, after 10+ hours I can see c->gc_stats.in_use is 96%. Which is a bit more than 95%, but c->sectors_go_gc is still larger than 0. This is how the forever-bypass happens. It has nothing to do with the latency of neither I/O nor gc.


> It may be a good idea to do `c->gc_stats.in_use > CUTOFF_CACHE_ADD/2` to
> start garbage collection when it is half-way "full".
> 

No, it is not designed to work in this way. By the above change, all I/O will bypass the cache device and go directly into backing device when cache device is occupied only 50% space.



> Reaching 50% is still quite conservative, but if you want to wait longer, 
> then even 80% or 90% would be fine; however, I think 100% is too far.  We 
> want to avoid the case where bcache is completely "out" of buckets and we 
> have to wait for garbage collection latency before a cache bucket can 
> fill, since buckets should be available.
> 
> For example on our system we have 736824 buckets available:
> # cat /sys/devices/virtual/block/dm-9/bcache/nbuckets
> 736824
> 
> There should be no reason to wait until all buckets are exhausted. Forcing 
> garbage collection at 50% (368412 buckets "in use") would be good house 
> keeping.
> 
> You know this code very well so if I have misinterpreted something here, 
> then please fill me in on the details.

As I said, this patch is just to avoid a forever-bypass condition, and this is an extreme condition which is rare to happen for normal workload.

Thanks.

Coly Li

Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Eric Wheeler]

[-- Attachment #1: Type: text/plain, Size: 3956 bytes --]

On Tue, 28 May 2024, Coly Li wrote:
> > 2024年5月28日 06:31，Eric Wheeler <bcache@lists.ewheeler.net> 写道：
> > 
> > On Tue, 28 May 2024, Coly Li wrote:
> > 
> >> If there are extreme heavy write I/O continuously hit on relative small
> >> cache device (512GB in my testing), it is possible to make counter
> >> c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.
> >> 
> >> If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
> >> requests will bypass the cache device because check_should_bypass()
> >> returns 'true'. Because all writes bypass the cache device, counter
> >> c->sectors_to_gc has no chance to be negative value, and garbage
> >> collection thread won't be waken up even the whole cache becomes clean
> >> after writeback accomplished. The aftermath is that all write I/Os go
> >> directly into backing device even the cache device is clean.
> >> 
> >> To avoid the above situation, this patch uses a quite conservative way
> >> to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
> >> up garbage collection thread when the whole cache device is clean.
> > 
> > Nice fix.
> > 
> > If I understand correctly, even with this fix, bcache can reach a point 
> > where it must wait until garbage collection frees a bucket (via 
> > force_wake_up_gc) before buckets can be used again.  Waiting to call 
> > force_wake_up_gc until `c->gc_stats.in_use` exceeds CUTOFF_CACHE_ADD may 
> > not respond as fast as it could, and IO latency is important.
> > 
> 
> CUTOFF_CACHE_ADD is not for this purpose.
> GC is triggered by c->sectors_to_gc, it works as
> - initialized as 1/16 size of cache device.
> - every allocation decreases cached size from it.
> - once c->sectors_go_gc is negative value, wakeup gc thread and reset the value to 1/16 size of cache device.
> 
> CUTOFF_CACHE_ADD is to avoid something like no-space deadlock in cache 
> space. If cache space is allocated more than CUTOFF_CACHE_ADD (95%), 
> cache space will not be allocated out anymore and all read/write will 
> bypass and go directly into backing device. In my testing, after 10+ 
> hours I can see c->gc_stats.in_use is 96%. Which is a bit more than 95%, 
> but c->sectors_go_gc is still larger than 0. This is how the 
> forever-bypass happens. It has nothing to do with the latency of neither 
> I/O nor gc.

Understood, thank you for the explanation!

You said that this bug exists an older version even though it is difficult 
trigger. Perhaps it is a good idea to CC stable:

	Cc: stable@vger.kernel.org

Also, 
	Reviewed-by: "Eric Wheeler" <bcache@linux.ewheeler.net>

--
Eric Wheeler

> 
> 
> > It may be a good idea to do `c->gc_stats.in_use > CUTOFF_CACHE_ADD/2` to
> > start garbage collection when it is half-way "full".
> > 
> 
> No, it is not designed to work in this way. By the above change, all I/O will bypass the cache device and go directly into backing device when cache device is occupied only 50% space.
> 
> 
> 
> > Reaching 50% is still quite conservative, but if you want to wait longer, 
> > then even 80% or 90% would be fine; however, I think 100% is too far.  We 
> > want to avoid the case where bcache is completely "out" of buckets and we 
> > have to wait for garbage collection latency before a cache bucket can 
> > fill, since buckets should be available.
> > 
> > For example on our system we have 736824 buckets available:
> > # cat /sys/devices/virtual/block/dm-9/bcache/nbuckets
> > 736824
> > 
> > There should be no reason to wait until all buckets are exhausted. Forcing 
> > garbage collection at 50% (368412 buckets "in use") would be good house 
> > keeping.
> > 
> > You know this code very well so if I have misinterpreted something here, 
> > then please fill me in on the details.
> 
> As I said, this patch is just to avoid a forever-bypass condition, and this is an extreme condition which is rare to happen for normal workload.
> 
> Thanks.
> 
> Coly Li

Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()

[Coly Li]

> 2024年5月29日 08:16，Eric Wheeler <bcache@lists.ewheeler.net> 写道：
> 
> On Tue, 28 May 2024, Coly Li wrote:
>>> 2024年5月28日 06:31，Eric Wheeler <bcache@lists.ewheeler.net> 写道：
>>> 
>>> On Tue, 28 May 2024, Coly Li wrote:
>>> 
>>>> If there are extreme heavy write I/O continuously hit on relative small
>>>> cache device (512GB in my testing), it is possible to make counter
>>>> c->gc_stats.in_use continue to increase and exceed CUTOFF_CACHE_ADD.
>>>> 
>>>> If 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, all following write
>>>> requests will bypass the cache device because check_should_bypass()
>>>> returns 'true'. Because all writes bypass the cache device, counter
>>>> c->sectors_to_gc has no chance to be negative value, and garbage
>>>> collection thread won't be waken up even the whole cache becomes clean
>>>> after writeback accomplished. The aftermath is that all write I/Os go
>>>> directly into backing device even the cache device is clean.
>>>> 
>>>> To avoid the above situation, this patch uses a quite conservative way
>>>> to fix: if 'c->gc_stats.in_use > CUTOFF_CACHE_ADD' happens, only wakes
>>>> up garbage collection thread when the whole cache device is clean.
>>> 
>>> Nice fix.
>>> 
>>> If I understand correctly, even with this fix, bcache can reach a point 
>>> where it must wait until garbage collection frees a bucket (via 
>>> force_wake_up_gc) before buckets can be used again.  Waiting to call 
>>> force_wake_up_gc until `c->gc_stats.in_use` exceeds CUTOFF_CACHE_ADD may 
>>> not respond as fast as it could, and IO latency is important.
>>> 
>> 
>> CUTOFF_CACHE_ADD is not for this purpose.
>> GC is triggered by c->sectors_to_gc, it works as
>> - initialized as 1/16 size of cache device.
>> - every allocation decreases cached size from it.
>> - once c->sectors_go_gc is negative value, wakeup gc thread and reset the value to 1/16 size of cache device.
>> 
>> CUTOFF_CACHE_ADD is to avoid something like no-space deadlock in cache 
>> space. If cache space is allocated more than CUTOFF_CACHE_ADD (95%), 
>> cache space will not be allocated out anymore and all read/write will 
>> bypass and go directly into backing device. In my testing, after 10+ 
>> hours I can see c->gc_stats.in_use is 96%. Which is a bit more than 95%, 
>> but c->sectors_go_gc is still larger than 0. This is how the 
>> forever-bypass happens. It has nothing to do with the latency of neither 
>> I/O nor gc.
> 
> Understood, thank you for the explanation!
> 
> You said that this bug exists an older version even though it is difficult 
> trigger. Perhaps it is a good idea to CC stable:
> 
> Cc: stable@vger.kernel.org
> 

It is unnecessary to Cc stable, manually writing sysfs file trigger_gc can avoid the always-bypass situation.
This patch is to make the default configuration works without extra operation.



> Also, 
> Reviewed-by: "Eric Wheeler" <bcache@linux.ewheeler.net>

Thanks for the review. Maybe it is a bit late to add the tag, I just found this series was in linux-block tree already and will show up in next -rc quite soon.

Thanks.

Coly Li


[snipped]