Re: [PATCH v2] bcache: fix set_at_max_writeback_rate() for multiple attached devices

[mingzhe <mingzhe.zou@easystack.cn>]

在 2022/9/19 12:38, Coly Li 写道:
> 
> 
>> 2022年9月19日 11:29，mingzhe <mingzhe.zou@easystack.cn> 写道：
>>
>>
>>
>> 在 2022/9/18 20:16, Coly Li 写道:
>>> Inside set_at_max_writeback_rate() the calculation in following if()
>>> check is wrong,
>>> 	if (atomic_inc_return(&c->idle_counter) <
>>> 	    atomic_read(&c->attached_dev_nr) * 6)
>>> Because each attached backing device has its own writeback thread
>>> running and increasing c->idle_counter, the counter increates much
>>> faster than expected. The correct calculation should be,
>>> 	(counter / dev_nr) < dev_nr * 6
>>> which equals to,
>>> 	counter < dev_nr * dev_nr * 6
>>> This patch fixes the above mistake with correct calculation, and helper
>>> routine idle_counter_exceeded() is added to make code be more clear.
>>> Reported-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
>>> Signed-off-by: Coly Li <colyli@suse.de>
>>> ---
>>> Changelog:
>>> v2: Add the missing "!atomic_read(&c->at_max_writeback_rate)" part
>>>      back.
>>> v1: Original verison.
>>>   drivers/md/bcache/writeback.c | 73 +++++++++++++++++++++++++----------
>>>   1 file changed, 52 insertions(+), 21 deletions(-)
>>> diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
>>> index 647661005176..c186bf55fe61 100644
>>> --- a/drivers/md/bcache/writeback.c
>>> +++ b/drivers/md/bcache/writeback.c
>>> @@ -157,6 +157,53 @@ static void __update_writeback_rate(struct cached_dev *dc)
>>>   	dc->writeback_rate_target = target;
>>>   }
>>>   +static bool idle_counter_exceeded(struct cache_set *c)
>>> +{
>>> +	int counter, dev_nr;
>>> +
>>> +	/*
>>> +	 * If c->idle_counter is overflow (idel for really long time),
>>> +	 * reset as 0 and not set maximum rate this time for code
>>> +	 * simplicity.
>>> +	 */
>>> +	counter = atomic_inc_return(&c->idle_counter);
>>> +	if (counter <= 0) {
>>> +		atomic_set(&c->idle_counter, 0);
>>> +		return false;
>>> +	}
>>> +
>>> +	dev_nr = atomic_read(&c->attached_dev_nr);
>>> +	if (dev_nr == 0)
>>> +		return false;
>>> +
>>> +	/*
>>> +	 * c->idle_counter is increased by writeback thread of all
>>> +	 * attached backing devices, in order to represent a rough
>>> +	 * time period, counter should be divided by dev_nr.
>>> +	 * Otherwise the idle time cannot be larger with more backing
>>> +	 * device attached.
>>> +	 * The following calculation equals to checking
>>> +	 *	(counter / dev_nr) < (dev_nr * 6)
>>> +	 */
>>> +	if (counter < (dev_nr * dev_nr * 6))
>>> +		return false;
>> Hi, Coly

> 
> BTW, if the patch looks fine to you, could you please to response a Reviewed-by or Acked-by for it?
> 
> Thanks.
> 
> Coly Li
> 
Acked-by: Mingzhe Zou <mingzhe.zou@easystack.cn>

Thanks

mingzhe
>