Re: [PATCH 4/4] btrfs: Be smarter when sleeping in transaction_kthread

[Nikolay Borisov <nborisov@suse.com>]

On 16.10.20 г. 19:26 ч., Nikolay Borisov wrote:
> 
> 
> On 16.10.20 г. 17:20 ч., David Sterba wrote:
>> On Thu, Oct 08, 2020 at 03:24:30PM +0300, Nikolay Borisov wrote:
>>> If transaction_kthread is woken up before
>>> btrfs_fs_info::commit_interval seconds have elapsed it will sleep for a
>>> fixed period of 5 seconds. This is not a problem per-se but is not
>>> accuaret, instead the code should sleep for an interval which guarantees
>>> on next wakeup commit_interval would have passed. Since time tracking is
>>> not accurate add 1 second to ensure next wake up would be after
>>> commit_interval.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>>  fs/btrfs/disk-io.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>>> index c5d3e7f75066..a1fe99cf0831 100644
>>> --- a/fs/btrfs/disk-io.c
>>> +++ b/fs/btrfs/disk-io.c
>>> @@ -1735,7 +1735,7 @@ static int transaction_kthread(void *arg)
>>>  		if (cur->state < TRANS_STATE_COMMIT_START &&
>>>  		    delta < fs_info->commit_interval) {
>>>  			spin_unlock(&fs_info->trans_lock);
>>> -			delay = msecs_to_jiffies(5000);
>>> +			delay = msecs_to_jiffies((1+delta) * 1000);
>>
>> This does not seem right. Delta is number of seconds since the
>> transaction started, so we need to sleep for the remaining time. Which
>> is commit_interval - delta.
> 
> Doh, you are right. The correct line should be :
> 
> delay -= msecs_to_jiffies((1+delta) * 1000);

Correction it should be :

delay -= msecs_to_jiffies((delta-1) * 1000);

> 
> The + 1  is needed so that upon next wake up we are guaranteed next
> delta is  >= commit_interval. Shall I send a fixed patch or are you
> going to modify it and merge it?
> 
>>
>