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

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

On 04/11/2020 17:55, Amy Parker wrote:
> On Wed, Nov 4, 2020 at 9:50 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>>
>> 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.
> 
> How often is iops_limit unfindable?

I don't know, but the default is 10, so shouldn't be too ubiquitous.
Maybe someone here knows statistics.

> 
>>
>> 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.
> 
> Thanks for clarifying.
> 
>>
>>> 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
> 
> Thanks for finding the commit.
> 
>>
>>>
>>>>
>>>>>
>>>>>>
>>>>>> -       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
> 
> Best regards,
> Amy Parker
> (they/them)
> 

-- 
Pavel Begunkov