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.
>
I am sorry, sc->nr_to_scan should include the btree nodes in
c->btree_cache_freeable. I have sended a new patch.
>>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@xxxxxxxxxx wrote:
>>> From: Tang Junhui <tang.junhui@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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
