[PATCH 0/4] Small QOI fixes for transaction_kthread

[PATCH 0/4] Small QOI fixes for transaction_kthread

[Nikolay Borisov]

Following 4 patches make transaction_kthread code slitghly more user friendly.
Namely, patch 1 convert from open-coded multiplicaiton by HZ to using
msecs_to_jiffies helper. Patch 2 relies on the observation that the running
transaction is obtained under trans_lock so an extra check can be removed.
Patch 3/4 could possibly be squashed into 1 but the net effect is that the code
is more intuitive when sleeping in case a lower interval than commit_trans has
elapsed.


This has survived full xfstest run.

Nikolay Borisov (4):
  btrfs: Use helpers to convert from seconds to jiffies in
    transaction_kthread
  btrfs: Remove redundant check
  btrfs: Record delta directly in transaction_kthread
  btrfs: Be smarter when sleeping in transaction_kthread

 fs/btrfs/disk-io.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

--
2.17.1

[PATCH 1/4] btrfs: Use helpers to convert from seconds to jiffies in transaction_kthread

[Nikolay Borisov]

The kernel provides easy to understand helpers to convert from human
understandable units to the kernel-friendly 'jiffies'. So let's use
those to make the code easier to understand. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 764001609a15..77b52b724733 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1721,7 +1721,7 @@ static int transaction_kthread(void *arg)
 
 	do {
 		cannot_commit = false;
-		delay = HZ * fs_info->commit_interval;
+		delay = msecs_to_jiffies(fs_info->commit_interval * 1000);
 		mutex_lock(&fs_info->transaction_kthread_mutex);
 
 		spin_lock(&fs_info->trans_lock);
@@ -1736,7 +1736,7 @@ static int transaction_kthread(void *arg)
 		    (now < cur->start_time ||
 		     now - cur->start_time < fs_info->commit_interval)) {
 			spin_unlock(&fs_info->trans_lock);
-			delay = HZ * 5;
+			delay = msecs_to_jiffies(5000);
 			goto sleep;
 		}
 		transid = cur->transid;
-- 
2.17.1

[PATCH 2/4] btrfs: Remove redundant check

[Nikolay Borisov]

The value obtained from ktime_get_seconds() is guaranteed to be
monotonically increasing since it's taken from CLOCK_MONOTONIC. As
transaction_kthread obtains a reference to the currently running
transaction under holding btrfs_fs_info::trans_lock it's guaranteed to:

a) See an initialized 'cur', whose start_tim is guaranteed to be smaller
than 'now'
or
b) Not obtain a 'cur' and simply go to sleep.

Given this remove the unnecessary check, if it sees now <
cur->start_time this would imply there are far greated problems on the
machine.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 77b52b724733..b9fbbf66ccee 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1733,8 +1733,7 @@ static int transaction_kthread(void *arg)
 
 		now = ktime_get_seconds();
 		if (cur->state < TRANS_STATE_COMMIT_START &&
-		    (now < cur->start_time ||
-		     now - cur->start_time < fs_info->commit_interval)) {
+		    (now - cur->start_time < fs_info->commit_interval)) {
 			spin_unlock(&fs_info->trans_lock);
 			delay = msecs_to_jiffies(5000);
 			goto sleep;
-- 
2.17.1

[PATCH 3/4] btrfs: Record delta directly in transaction_kthread

[Nikolay Borisov]

Rename 'now' to 'delta' and store there the delta between transaction
start time and current time. This is in preparation for optimising the
sleep logic in the next patch. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/disk-io.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b9fbbf66ccee..c5d3e7f75066 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1715,7 +1715,7 @@ static int transaction_kthread(void *arg)
 	struct btrfs_trans_handle *trans;
 	struct btrfs_transaction *cur;
 	u64 transid;
-	time64_t now;
+	time64_t delta;
 	unsigned long delay;
 	bool cannot_commit;
 
@@ -1731,9 +1731,9 @@ static int transaction_kthread(void *arg)
 			goto sleep;
 		}
 
-		now = ktime_get_seconds();
+		delta = ktime_get_seconds() - cur->start_time;
 		if (cur->state < TRANS_STATE_COMMIT_START &&
-		    (now - cur->start_time < fs_info->commit_interval)) {
+		    delta < fs_info->commit_interval) {
 			spin_unlock(&fs_info->trans_lock);
 			delay = msecs_to_jiffies(5000);
 			goto sleep;
-- 
2.17.1

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

[Nikolay Borisov]

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);
 			goto sleep;
 		}
 		transid = cur->transid;
-- 
2.17.1

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

[David Sterba]

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.

Re: [PATCH 0/4] Small QOI fixes for transaction_kthread

[David Sterba]

On Thu, Oct 08, 2020 at 03:24:26PM +0300, Nikolay Borisov wrote:
> Following 4 patches make transaction_kthread code slitghly more user friendly.
> Namely, patch 1 convert from open-coded multiplicaiton by HZ to using
> msecs_to_jiffies helper. Patch 2 relies on the observation that the running
> transaction is obtained under trans_lock so an extra check can be removed.
> Patch 3/4 could possibly be squashed into 1 but the net effect is that the code
> is more intuitive when sleeping in case a lower interval than commit_trans has
> elapsed.
> 
> 
> This has survived full xfstest run.
> 
> Nikolay Borisov (4):
>   btrfs: Use helpers to convert from seconds to jiffies in
>     transaction_kthread
>   btrfs: Remove redundant check
>   btrfs: Record delta directly in transaction_kthread

1-3 added to misc-next, with some fixups.

>   btrfs: Be smarter when sleeping in transaction_kthread

There's a comment regarding correctness.

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

[Nikolay Borisov]

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);

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?

> 

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

[Nikolay Borisov]

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

[PATCH v2] btrfs: Be smarter when sleeping in transaction_kthread

[Nikolay Borisov]

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 precise substract 1 second from delta to ensure the delay we end up waiting
will be longer than than the wake up period.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---

This one survived my testing and debugging confirmed that sometimes delta can
indeed be 0 so utilising min is the right thing to do.

 fs/btrfs/disk-io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b7deb3e9dd9e..2632b5833f64 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1735,7 +1735,8 @@ 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((delta-1) * 1000);
+			delay = min(delay, msecs_to_jiffies(fs_info->commit_interval * 1000));
 			goto sleep;
 		}
 		transid = cur->transid;
--
2.25.1

Re: [PATCH 1/4] btrfs: Use helpers to convert from seconds to jiffies in transaction_kthread

[Josef Bacik]

On 10/8/20 8:24 AM, Nikolay Borisov wrote:
> The kernel provides easy to understand helpers to convert from human
> understandable units to the kernel-friendly 'jiffies'. So let's use
> those to make the code easier to understand. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>   fs/btrfs/disk-io.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 764001609a15..77b52b724733 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -1721,7 +1721,7 @@ static int transaction_kthread(void *arg)
>   
>   	do {
>   		cannot_commit = false;
> -		delay = HZ * fs_info->commit_interval;
> +		delay = msecs_to_jiffies(fs_info->commit_interval * 1000);

Since we're now doing everything in msecs, why don't we just make sure 
->commit_interval is set to msecs, that way we don't have to carry around the * 
1000?  If we still need the multiplication we should be using MSEC_PER_SEC.  Thanks,

Josef

Re: [PATCH 1/4] btrfs: Use helpers to convert from seconds to jiffies in transaction_kthread

[Nikolay Borisov]

On 21.10.20 г. 17:51 ч., Josef Bacik wrote:
> On 10/8/20 8:24 AM, Nikolay Borisov wrote:
>> The kernel provides easy to understand helpers to convert from human
>> understandable units to the kernel-friendly 'jiffies'. So let's use
>> those to make the code easier to understand. No functional changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>   fs/btrfs/disk-io.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 764001609a15..77b52b724733 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -1721,7 +1721,7 @@ static int transaction_kthread(void *arg)
>>         do {
>>           cannot_commit = false;
>> -        delay = HZ * fs_info->commit_interval;
>> +        delay = msecs_to_jiffies(fs_info->commit_interval * 1000);
> 
> Since we're now doing everything in msecs, why don't we just make sure
> ->commit_interval is set to msecs, that way we don't have to carry
> around the * 1000?  If we still need the multiplication we should be
> using MSEC_PER_SEC.  Thanks,

Yes, as a matter of fact I intend on making commit_interval into
jiffies, to completely eliminate the msecs_to_jiffies helpers here.

But that will be a follow on work once David merges the v2 of "Be
smarter" patch.

> 
> Josef
> 

Re: [PATCH 1/4] btrfs: Use helpers to convert from seconds to jiffies in transaction_kthread

[David Sterba]

On Wed, Oct 21, 2020 at 06:03:00PM +0300, Nikolay Borisov wrote:
> 
> 
> On 21.10.20 г. 17:51 ч., Josef Bacik wrote:
> > On 10/8/20 8:24 AM, Nikolay Borisov wrote:
> >> The kernel provides easy to understand helpers to convert from human
> >> understandable units to the kernel-friendly 'jiffies'. So let's use
> >> those to make the code easier to understand. No functional changes.
> >>
> >> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> >> ---
> >>   fs/btrfs/disk-io.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> >> index 764001609a15..77b52b724733 100644
> >> --- a/fs/btrfs/disk-io.c
> >> +++ b/fs/btrfs/disk-io.c
> >> @@ -1721,7 +1721,7 @@ static int transaction_kthread(void *arg)
> >>         do {
> >>           cannot_commit = false;
> >> -        delay = HZ * fs_info->commit_interval;
> >> +        delay = msecs_to_jiffies(fs_info->commit_interval * 1000);
> > 
> > Since we're now doing everything in msecs, why don't we just make sure
> > ->commit_interval is set to msecs, that way we don't have to carry
> > around the * 1000?  If we still need the multiplication we should be
> > using MSEC_PER_SEC.  Thanks,
> 
> Yes, as a matter of fact I intend on making commit_interval into
> jiffies, to completely eliminate the msecs_to_jiffies helpers here.
> 
> But that will be a follow on work once David merges the v2 of "Be
> smarter" patch.

As the commit_interval is an internal value, jiffies is ok, the
conversions happen happen for mount option set and print only. So ok
from me.