[PATCH] bcache: fix error count in memory shrink

[PATCH] bcache: fix error count in memory shrink

[tang.junhui]

From: Tang Junhui <tang.junhui@zte.com.cn>

In bch_mca_scan(), nr btree nodes are expected to shrink, so the for(;;)
loop need to satisfy the condition freed < nr.
And since c->btree_cache_used always decrease after mca_data_free() calling
in for(;;) loop,  so we need a auto variable to record c->btree_cache_used
before the for(;;) loop.

Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
---
 drivers/md/bcache/btree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
index 81e8dc3..7e9ef3d 100644
--- a/drivers/md/bcache/btree.c
+++ b/drivers/md/bcache/btree.c
@@ -664,6 +664,7 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
 	struct btree *b, *t;
 	unsigned long i, nr = sc->nr_to_scan;
 	unsigned long freed = 0;
+	unsigned int btree_cache_used;
 
 	if (c->shrinker_disabled)
 		return SHRINK_STOP;
@@ -700,7 +701,8 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
 		}
 	}
 
-	for (i = 0; (nr--) && i < c->btree_cache_used; i++) {
+	btree_cache_used = c->btree_cache_used;
+	for (i = 0; freed < nr && i < btree_cache_used; i++) {
 		if (list_empty(&c->btree_cache))
 			goto out;
 
-- 
1.8.3.1

Re: [PATCH] bcache: fix error count in memory shrink

[Michael Lyle]

Hi Tang Junhui---

I'm not really sure about this one.  It changes the semantics of the
amount of work done-- nr_to_scan now means number of things to free
instead of the number to check.

If the system is under severe memory pressure, and most of the cache is
essential/actively used, this could greatly increase the amount of work
done.

As to the i < btree_cache_used, it's arguably a bug, but it only reduces
the amount of work done if the cache is very, very small.

Mike

On 01/30/2018 06:46 PM, tang.junhui@zte.com.cn wrote:
> From: Tang Junhui <tang.junhui@zte.com.cn>
> 
> In bch_mca_scan(), nr btree nodes are expected to shrink, so the for(;;)
> loop need to satisfy the condition freed < nr.
> And since c->btree_cache_used always decrease after mca_data_free() calling
> in for(;;) loop,  so we need a auto variable to record c->btree_cache_used
> before the for(;;) loop.
> 
> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
> ---
>  drivers/md/bcache/btree.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
> index 81e8dc3..7e9ef3d 100644
> --- a/drivers/md/bcache/btree.c
> +++ b/drivers/md/bcache/btree.c
> @@ -664,6 +664,7 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
>  	struct btree *b, *t;
>  	unsigned long i, nr = sc->nr_to_scan;
>  	unsigned long freed = 0;
> +	unsigned int btree_cache_used;
>  
>  	if (c->shrinker_disabled)
>  		return SHRINK_STOP;
> @@ -700,7 +701,8 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
>  		}
>  	}
>  
> -	for (i = 0; (nr--) && i < c->btree_cache_used; i++) {
> +	btree_cache_used = c->btree_cache_used;
> +	for (i = 0; freed < nr && i < btree_cache_used; i++) {
>  		if (list_empty(&c->btree_cache))
>  			goto out;
>  
> 

Re: [PATCH] bcache: fix error count in memory shrink

[tang.junhui]

Hi Mike---

>Hi Tang Junhui---
>
>I'm not really sure about this one.  It changes the semantics of the
>amount of work done-- nr_to_scan now means number of things to free
>instead of the number to check.
>
The code seems to be designed as that, sc->nr_to_scan marks how much btree
nodes to scan in c->btree_cache_used, and it doesn't include the btree
nodes in c->btree_cache_freeable.

>If the system is under severe memory pressure, and most of the cache is
>essential/actively used, this could greatly increase the amount of work
>done.
>
Yes, I didn't relize this.
>As to the i < btree_cache_used, it's arguably a bug, but it only reduces
>the amount of work done if the cache is very, very small.
>
I think it is a bug, I'll send a new patch to add variable to record 
c->btree_cache_used befor for(;;) loop.

>Mike
>
>On 01/30/2018 06:46 PM, tang.junhui@zte.com.cn wrote:
>> From: Tang Junhui <tang.junhui@zte.com.cn>
>> 
>> In bch_mca_scan(), nr btree nodes are expected to shrink, so the for(;;)
>> loop need to satisfy the condition freed < nr.
>> And since c->btree_cache_used always decrease after mca_data_free() calling
>> in for(;;) loop,  so we need a auto variable to record c->btree_cache_used
>> before the for(;;) loop.
>> 
>> Signed-off-by: Tang Junhui <tang.junhui@zte.com.cn>
>> ---
>>  drivers/md/bcache/btree.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/md/bcache/btree.c b/drivers/md/bcache/btree.c
>> index 81e8dc3..7e9ef3d 100644
>> --- a/drivers/md/bcache/btree.c
>> +++ b/drivers/md/bcache/btree.c
>> @@ -664,6 +664,7 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
>>      struct btree *b, *t;
>>      unsigned long i, nr = sc->nr_to_scan;
>>      unsigned long freed = 0;
>> +    unsigned int btree_cache_used;
>>  
>>      if (c->shrinker_disabled)
>>          return SHRINK_STOP;
>> @@ -700,7 +701,8 @@ static unsigned long bch_mca_scan(struct shrinker *shrink,
>>          }
>>      }
>>  
>> -    for (i = 0; (nr--) && i < c->btree_cache_used; i++) {
>> +    btree_cache_used = c->btree_cache_used;
>> +    for (i = 0; freed < nr && i < btree_cache_used; i++) {
>>          if (list_empty(&c->btree_cache))
>>              goto out;
>>  
>> 
Thanks,
Tang Junhui