Re: [PATCH 1/4] btrfs: discard: speed up discard up to iops_limit

[Amy Parker <enbyamy@gmail.com>]

On Wed, Nov 4, 2020 at 1:50 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> Instead of using iops_limit only for cutting off extremes, calculate the
> discard delay directly from it, so it closely follows iops_limit and
> doesn't under-discarding even though quotas are not saturated.

This sounds like it potentially be a great performance boost, do you
have any performance metrics regarding this patch?

>
> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> ---
>  fs/btrfs/discard.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
> index 741c7e19c32f..76796a90e88d 100644
> --- a/fs/btrfs/discard.c
> +++ b/fs/btrfs/discard.c
> @@ -519,7 +519,6 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
>         s64 discardable_bytes;
>         u32 iops_limit;
>         unsigned long delay;
> -       unsigned long lower_limit = BTRFS_DISCARD_MIN_DELAY_MSEC;
>
>         discardable_extents = atomic_read(&discard_ctl->discardable_extents);
>         if (!discardable_extents)
> @@ -550,11 +549,12 @@ void btrfs_discard_calc_delay(struct btrfs_discard_ctl *discard_ctl)
>
>         iops_limit = READ_ONCE(discard_ctl->iops_limit);
>         if (iops_limit)
> -               lower_limit = max_t(unsigned long, lower_limit,
> -                                   MSEC_PER_SEC / iops_limit);
> +               delay = MSEC_PER_SEC / iops_limit;
> +       else
> +               delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;

Looks good to me. I wonder why there wasn't handling of if iops_limit
was unfindable
before?

>
> -       delay = BTRFS_DISCARD_TARGET_MSEC / discardable_extents;
> -       delay = clamp(delay, lower_limit, BTRFS_DISCARD_MAX_DELAY_MSEC);
> +       delay = clamp(delay, BTRFS_DISCARD_MIN_DELAY_MSEC,
> +                     BTRFS_DISCARD_MAX_DELAY_MSEC);
>         discard_ctl->delay = msecs_to_jiffies(delay);
>
>         spin_unlock(&discard_ctl->lock);
> --
> 2.24.0
>

This patch looks all great to me.

Best regards,
Amy Parker
(they/them)