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

[Pavel Begunkov <asml.silence@gmail.com>]

On 04/11/2020 17:33, Amy Parker wrote:
> On Wed, Nov 4, 2020 at 9:22 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> On 04/11/2020 15:29, Amy Parker wrote:
>>> 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?
>>
>> Boosting the discard rate and so reaping stalling blocks may be nice, but
>> unless it holds too much memory creating lack of space it shouldn't affect
>> throughput. Though, it's better to ask people with deeper understanding
>> of the fs.
> 
> Alright, thanks for the clarification.
> 
>> What I've seen is that in some cases there are extents staying queued for
>> discarding for _too_ long. E.g. reaping a small number of very fat extents
>> keeps delay at max and doesn't allow to reap them effectively. That could
>> be a problem with fast drives.
> 
> Ah, yep. Seen this personally to a smaller extent.
> 
>>
>>>
>>>>
>>>> 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?
>>
>> Not sure what you mean by unfindable, but async discard is relatively new,
>> might be that everyone just have their hands full.
> 
> By unfindable I mean if iops_limit turned up as null when reading it
> from discard_ctl.

Ahh, ok. It's handled and I left it as it was, that BTW is still a problem.

First it calculates a delay based on number of queued extents and than clamps
it to (BTRFS_DISCARD_MIN_DELAY_MSEC, BTRFS_DISCARD_MAX_DELAY_MSEC). Without
this patch it did the same but the lower bound was calculated from iops_limit.

> Async discard was added in 5.6, correct? So yeah, makes sense then that people
> just had their hands full. Thanks for adding it.

b0643e59cfa609c4b5f ("btrfs: add the beginning of async discard, discard
workqueue"). Dec 2019, so less than a year

> 
>>
>>>
>>>>
>>>> -       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.

-- 
Pavel Begunkov