Linux bcache driver list
 help / color / mirror / Atom feed
* [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
@ 2024-05-27 17:47 Coly Li
  2024-05-27 17:47 ` [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set() Coly Li
  2024-05-27 22:31 ` [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Eric Wheeler
  0 siblings, 2 replies; 9+ messages in thread
From: Coly Li @ 2024-05-27 17:47 UTC (permalink / raw)
  To: linux-bcache; +Cc: 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set()
  2024-05-27 17:47 [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Coly Li
@ 2024-05-27 17:47 ` Coly Li
  2024-05-27 22:33   ` Eric Wheeler
  2024-05-27 22:31 ` [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Eric Wheeler
  1 sibling, 1 reply; 9+ messages in thread
From: Coly Li @ 2024-05-27 17:47 UTC (permalink / raw)
  To: linux-bcache; +Cc: 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
  2024-05-27 17:47 [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Coly Li
  2024-05-27 17:47 ` [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set() Coly Li
@ 2024-05-27 22:31 ` Eric Wheeler
  2024-05-28  3:24   ` Coly Li
  1 sibling, 1 reply; 9+ messages in thread
From: Eric Wheeler @ 2024-05-27 22:31 UTC (permalink / raw)
  To: Coly Li; +Cc: linux-bcache

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set()
  2024-05-27 17:47 ` [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set() Coly Li
@ 2024-05-27 22:33   ` Eric Wheeler
  2024-05-28  2:18     ` Coly Li
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Wheeler @ 2024-05-27 22:33 UTC (permalink / raw)
  To: Coly Li; +Cc: linux-bcache

On Tue, 28 May 2024, Coly Li wrote:
> 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);


Is there a matching "get" somewhere that should be removed, too?

--
Eric Wheeler



> -	return -1;
>  }
>  
>  int bch_bucket_alloc_set(struct cache_set *c, unsigned int reserve,
> -- 
> 2.35.3
> 
> 
> 

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set()
  2024-05-27 22:33   ` Eric Wheeler
@ 2024-05-28  2:18     ` Coly Li
  0 siblings, 0 replies; 9+ messages in thread
From: Coly Li @ 2024-05-28  2:18 UTC (permalink / raw)
  To: Eric Wheeler; +Cc: Bcache Linux



> 2024年5月28日 06:33,Eric Wheeler <bcache@lists.ewheeler.net> 写道:
> 
> On Tue, 28 May 2024, Coly Li wrote:
>> 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);
> 
> 
> Is there a matching "get" somewhere that should be removed, too?

No, it is unnecessary and should be avoided. Because k is ZERO_KEY here, calling bch_bucket_free() and bkey_put() are NOOP indeed.

Coly Li


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
  2024-05-27 22:31 ` [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Eric Wheeler
@ 2024-05-28  3:24   ` Coly Li
  2024-05-29  0:16     ` Eric Wheeler
  0 siblings, 1 reply; 9+ messages in thread
From: Coly Li @ 2024-05-28  3:24 UTC (permalink / raw)
  To: Eric Wheeler; +Cc: Bcache Linux



> 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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set()
  2024-05-28 12:09 [PATCH 0/3] bcache-6.10 20240528 Coly Li
@ 2024-05-28 12:09 ` Coly Li
  0 siblings, 0 replies; 9+ messages in thread
From: Coly Li @ 2024-05-28 12:09 UTC (permalink / raw)
  To: axboe; +Cc: linux-block, linux-bcache, 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


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
  2024-05-28  3:24   ` Coly Li
@ 2024-05-29  0:16     ` Eric Wheeler
  2024-05-29 16:23       ` Coly Li
  0 siblings, 1 reply; 9+ messages in thread
From: Eric Wheeler @ 2024-05-29  0:16 UTC (permalink / raw)
  To: Coly Li; +Cc: Bcache Linux

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass()
  2024-05-29  0:16     ` Eric Wheeler
@ 2024-05-29 16:23       ` Coly Li
  0 siblings, 0 replies; 9+ messages in thread
From: Coly Li @ 2024-05-29 16:23 UTC (permalink / raw)
  To: Eric Wheeler; +Cc: Bcache Linux



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


^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2024-05-29 16:24 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-27 17:47 [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Coly Li
2024-05-27 17:47 ` [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set() Coly Li
2024-05-27 22:33   ` Eric Wheeler
2024-05-28  2:18     ` Coly Li
2024-05-27 22:31 ` [PATCH 2/3] bcache: call force_wake_up_gc() if necessary in check_should_bypass() Eric Wheeler
2024-05-28  3:24   ` Coly Li
2024-05-29  0:16     ` Eric Wheeler
2024-05-29 16:23       ` Coly Li
  -- strict thread matches above, loose matches on Subject: below --
2024-05-28 12:09 [PATCH 0/3] bcache-6.10 20240528 Coly Li
2024-05-28 12:09 ` [PATCH 3/3] bcache: code cleanup in __bch_bucket_alloc_set() Coly Li

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox