Re: [PATCH 3/4] btrfs: don't miss discards after override-schedule

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

On 04/11/2020 20:59, Josef Bacik wrote:
> On 11/4/20 4:45 AM, Pavel Begunkov wrote:
>> If btrfs_discard_schedule_work() is called with override=true, it sets
>> delay anew regardless how much time left until the timer should have
>> fired. If delays are long (that can happen, for example, with low
>> kbps_limit), they might be constantly overriden without having a chance
>> to run the discard work.
>>
>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
>> ---
>>   fs/btrfs/ctree.h   |  1 +
>>   fs/btrfs/discard.c | 11 +++++++++++
>>   2 files changed, 12 insertions(+)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index d43a82dcdfc0..ad71c8c769de 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -469,6 +469,7 @@ struct btrfs_discard_ctl {
>>       struct btrfs_block_group *block_group;
>>       struct list_head discard_list[BTRFS_NR_DISCARD_LISTS];
>>       u64 prev_discard;
>> +    u64 prev_discard_time;
>>       atomic_t discardable_extents;
>>       atomic64_t discardable_bytes;
>>       u64 max_discard_size;
>> diff --git a/fs/btrfs/discard.c b/fs/btrfs/discard.c
>> index b6c68e5711f0..c9018b9ccf99 100644
>> --- a/fs/btrfs/discard.c
>> +++ b/fs/btrfs/discard.c
>> @@ -381,6 +381,15 @@ void btrfs_discard_schedule_work(struct btrfs_discard_ctl *discard_ctl,
>>               delay = max(delay, bg_timeout);
>>           }
>>   +        if (override && discard_ctl->prev_discard) {
>> +            u64 elapsed = now - discard_ctl->prev_discard_time;
>> +
>> +            if (delay > elapsed)
>> +                delay -= elapsed;
>> +            else
>> +                delay = 0;
>> +        }
>> +
>>           mod_delayed_work(discard_ctl->discard_workers,
>>                    &discard_ctl->work, nsecs_to_jiffies(delay));
>>       }
>> @@ -466,6 +475,7 @@ static void btrfs_discard_workfn(struct work_struct *work)
>>       }
>>         discard_ctl->prev_discard = trimmed;
>> +    discard_ctl->prev_discard_time = ktime_get_ns();
> 
> I noticed these weren't protected by the discard_ctl->lock, so I went to look at if that was ok.  It appears to be ok, since this is the workfn, and we only read them if there's no pending work, so we're protected there.  Just a note for anybody else who finds it weird, though I wouldn't argue with protecting it with a lock just to remove any ambiguity.

Agree, together with ->prev_discard. Or at least there should be
a comment.

> 
> Reviewed-by: Josef Bacik <josef@toxicpanda.com>
Thanks

-- 
Pavel Begunkov