bcache: fix set_at_max_writeback_rate() for multiple attached devices

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>
Acked-by: Mingzhe Zou <mingzhe.zou@easystack.cn>
Link: https://lore.kernel.org/r/20220919161647.81238-6-colyli@suse.de
Signed-off-by: Jens Axboe <axboe@kernel.dk>

diff --git a/drivers/md/bcache/writeback.c b/drivers/md/bcache/writeback.c
index 6476610..0285b67 100644 (file)
--- 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;
+
+       return true;
+}
+
+/*
+ * Idle_counter is increased every time 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;