linux-kernel - Re: [PATCH] blk-wbt: fix performance regression in wbt scale_up/scale

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAD+ocby9=Xyy=0cENHw2ndmnvKk-Eti3XFzJ6WzAE4tbqC7-hg@mail.gmail.com>
Date:   Fri, 4 Oct 2019 13:53:08 -0700
From:   harshad shirwadkar <harshadshirwadkar@...il.com>
To:     linux-kernel@...r.kernel.org
Cc:     Josef Bacik <jbacik@...com>, vaibhavrustagi@...gle.com
Subject: Re: [PATCH] blk-wbt: fix performance regression in wbt scale_up/scale_down

On Fri, Oct 4, 2019 at 11:09 AM Harshad Shirwadkar
<harshadshirwadkar@...il.com> wrote:
>
> scale_up wakes up waiters after scaling up. But after scaling max, it
> should not wake up more waiters as waiters will not have anything to
> do. This patch fixes this by making scale_up (and also scale_down)
> return when threshold is reached.
>
> This bug causes increased fdatasync latency when fdatasync and dd
> conv=sync are performed in parallel on 4.19 compared to 4.14. This
> bug was introduced during refactoring of blk-wbt code.
>
> Fixes: a79050434b45 ("blk-rq-qos: refactor out common elements of blk-wbt")
> Cc: Josef Bacik <jbacik@...com>
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>
> ---
>  block/blk-rq-qos.c | 14 +++++++++-----
>  block/blk-rq-qos.h |  4 ++--
>  block/blk-wbt.c    |  6 ++++--
>  3 files changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c
> index 3954c0dc1443..d92abb43000c 100644
> --- a/block/blk-rq-qos.c
> +++ b/block/blk-rq-qos.c
> @@ -142,24 +142,27 @@ bool rq_depth_calc_max_depth(struct rq_depth *rqd)
>         return ret;
>  }
>
> -void rq_depth_scale_up(struct rq_depth *rqd)
> +/* Returns true on success and false if scaling up wasn't possible */
> +bool rq_depth_scale_up(struct rq_depth *rqd)
>  {
>         /*
>          * Hit max in previous round, stop here
>          */
>         if (rqd->scaled_max)
> -               return;
> +               return false;
>
>         rqd->scale_step--;
>
>         rqd->scaled_max = rq_depth_calc_max_depth(rqd);
> +       return true;
>  }
>
>  /*
>   * Scale rwb down. If 'hard_throttle' is set, do it quicker, since we
> - * had a latency violation.
> + * had a latency violation. Returns true on success and returns false if
> + * scaling down wasn't possible.
>   */
> -void rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle)
> +bool rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle)
>  {
>         /*
>          * Stop scaling down when we've hit the limit. This also prevents
> @@ -167,7 +170,7 @@ void rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle)
>          * keep up.
>          */
>         if (rqd->max_depth == 1)
> -               return;
> +               return false;
>
>         if (rqd->scale_step < 0 && hard_throttle)
>                 rqd->scale_step = 0;
> @@ -176,6 +179,7 @@ void rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle)
>
>         rqd->scaled_max = false;
>         rq_depth_calc_max_depth(rqd);
> +       return 0;
Oops, I meant return true here, thanks Vaibhav (+cc) for pointing this
out. I'll fix this in V2.

>  }
>
>  struct rq_qos_wait_data {
> diff --git a/block/blk-rq-qos.h b/block/blk-rq-qos.h
> index 2300e038b9fa..c0f0778d5396 100644
> --- a/block/blk-rq-qos.h
> +++ b/block/blk-rq-qos.h
> @@ -125,8 +125,8 @@ void rq_qos_wait(struct rq_wait *rqw, void *private_data,
>                  acquire_inflight_cb_t *acquire_inflight_cb,
>                  cleanup_cb_t *cleanup_cb);
>  bool rq_wait_inc_below(struct rq_wait *rq_wait, unsigned int limit);
> -void rq_depth_scale_up(struct rq_depth *rqd);
> -void rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle);
> +bool rq_depth_scale_up(struct rq_depth *rqd);
> +bool rq_depth_scale_down(struct rq_depth *rqd, bool hard_throttle);
>  bool rq_depth_calc_max_depth(struct rq_depth *rqd);
>
>  void __rq_qos_cleanup(struct rq_qos *rqos, struct bio *bio);
> diff --git a/block/blk-wbt.c b/block/blk-wbt.c
> index 313f45a37e9d..5a96881e7a52 100644
> --- a/block/blk-wbt.c
> +++ b/block/blk-wbt.c
> @@ -308,7 +308,8 @@ static void calc_wb_limits(struct rq_wb *rwb)
>
>  static void scale_up(struct rq_wb *rwb)
>  {
> -       rq_depth_scale_up(&rwb->rq_depth);
> +       if (!rq_depth_scale_up(&rwb->rq_depth))
> +               return;
>         calc_wb_limits(rwb);
>         rwb->unknown_cnt = 0;
>         rwb_wake_all(rwb);
> @@ -317,7 +318,8 @@ static void scale_up(struct rq_wb *rwb)
>
>  static void scale_down(struct rq_wb *rwb, bool hard_throttle)
>  {
> -       rq_depth_scale_down(&rwb->rq_depth, hard_throttle);
> +       if (!rq_depth_scale_down(&rwb->rq_depth, hard_throttle))
> +               return;
>         calc_wb_limits(rwb);
>         rwb->unknown_cnt = 0;
>         rwb_trace_step(rwb, "scale down");
> --
> 2.23.0.581.g78d2f28ef7-goog
>