lkml.org 
[lkml]   [2022]   [Oct]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 6.0 809/862] bcache: fix set_at_max_writeback_rate() for multiple attached devices
    Date
    From: Coly Li <colyli@suse.de>

    [ Upstream commit d2d05b88035d2d51a5bb6c5afec88a0880c73df4 ]

    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>
    Signed-off-by: Sasha Levin <sashal@kernel.org>
    ---
    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 3f0ff3aab6f2..9c227e4a8465 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;
    +
    + 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;

    --
    2.35.1


    \
     
     \ /
      Last update: 2022-10-19 14:33    [W:2.423 / U:0.296 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site