| Message ID | 20201008122430.93433-5-nborisov@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small QOI fixes for transaction_kthread | expand |
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.
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? >
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? > >> >
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;
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(-)