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

[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

Look good to me. However, do we need to specify a maximum value for 
idle_counter. If cache_set has 100 backing devices, there are dev_nr*6 
rounds of update_writeback_rate() on each backing device, which takes 
dc->writeback_rate_update_seconds(default is 5 seconds)*600 seconds.

In fact, any io request from the backing device will clear the 
idle_counter of cache_set, and at_max_writeback_rate will exit soon. 
Therefore, if cache_set waits for 5 minutes or 10 minutes without any io 
request to start at_max_writeback_rate, it will not have any effect on 
the possible front-end io.

In addition, cache_set only waits 6 rounds of update_writeback_rate() 
should not have much performance impact.

mingzhe
> +
> +	return true;
> +}
> +
> +/*
> + * Idle_counter is increased everytime when update_writeback_rate() is
> + * called. If all backing devices attached to the same cache set have
> + * identical dc->writeback_rate_update_seconds values, it is about 6
> + * rounds of update_writeback_rate() on each backing device before
> + * c->at_max_writeback_rate is set to 1, and then max wrteback rate set
> + * to each dc->writeback_rate.rate.
> + * In order to avoid extra locking cost for counting exact dirty cached
> + * devices number, c->attached_dev_nr is used to calculate the idle
> + * throushold. It might be bigger if not all cached device are in write-
> + * back mode, but it still works well with limited extra rounds of
> + * update_writeback_rate().
> + */
>   static bool set_at_max_writeback_rate(struct cache_set *c,
>   				       struct cached_dev *dc)
>   {
> @@ -167,21 +214,8 @@ static bool set_at_max_writeback_rate(struct cache_set *c,
>   	/* Don't set max writeback rate if gc is running */
>   	if (!c->gc_mark_valid)
>   		return false;
> -	/*
> -	 * Idle_counter is increased everytime when update_writeback_rate() is
> -	 * called. If all backing devices attached to the same cache set have
> -	 * identical dc->writeback_rate_update_seconds values, it is about 6
> -	 * rounds of update_writeback_rate() on each backing device before
> -	 * c->at_max_writeback_rate is set to 1, and then max wrteback rate set
> -	 * to each dc->writeback_rate.rate.
> -	 * In order to avoid extra locking cost for counting exact dirty cached
> -	 * devices number, c->attached_dev_nr is used to calculate the idle
> -	 * throushold. It might be bigger if not all cached device are in write-
> -	 * back mode, but it still works well with limited extra rounds of
> -	 * update_writeback_rate().
> -	 */
> -	if (atomic_inc_return(&c->idle_counter) <
> -	    atomic_read(&c->attached_dev_nr) * 6)
> +
> +	if (!idle_counter_exceeded(c))
>   		return false;
>   
>   	if (atomic_read(&c->at_max_writeback_rate) != 1)
> @@ -195,13 +229,10 @@ static bool set_at_max_writeback_rate(struct cache_set *c,
>   	dc->writeback_rate_change = 0;
>   
>   	/*
> -	 * Check c->idle_counter and c->at_max_writeback_rate agagain in case
> -	 * new I/O arrives during before set_at_max_writeback_rate() returns.
> -	 * Then the writeback rate is set to 1, and its new value should be
> -	 * decided via __update_writeback_rate().
> +	 * In case new I/O arrives during before
> +	 * set_at_max_writeback_rate() returns.
>   	 */
> -	if ((atomic_read(&c->idle_counter) <
> -	     atomic_read(&c->attached_dev_nr) * 6) ||
> +	if (!idle_counter_exceeded(c) ||
>   	    !atomic_read(&c->at_max_writeback_rate))
>   		return false;
>