[PATCH 1/3] btrfs: use monotonic time for transaction handling

[PATCH 1/3] btrfs: use monotonic time for transaction handling

[Arnd Bergmann]

get_seconds() is deprecated because of the overflow of 32-bit times,
and it's not the best choice for measuring time intervals because it
can go backwards or jump due to settimeofday() or leap seconds.

This changes the transaction handling to instead use ktime_get_seconds(),
which returns a CLOCK_MONOTONIC timestamp that has neither of those
problems.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/btrfs/disk-io.c     | 4 ++--
 fs/btrfs/transaction.c | 2 +-
 fs/btrfs/transaction.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 205092dc9390..bf0717f2824d 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1803,7 +1803,7 @@ static int transaction_kthread(void *arg)
 	struct btrfs_trans_handle *trans;
 	struct btrfs_transaction *cur;
 	u64 transid;
-	unsigned long now;
+	time64_t now;
 	unsigned long delay;
 	bool cannot_commit;
 
@@ -1819,7 +1819,7 @@ static int transaction_kthread(void *arg)
 			goto sleep;
 		}
 
-		now = get_seconds();
+		now = ktime_get_seconds();
 		if (cur->state < TRANS_STATE_BLOCKED &&
 		    !test_bit(BTRFS_FS_NEED_ASYNC_COMMIT, &fs_info->flags) &&
 		    (now < cur->start_time ||
diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c
index ff5f6c719976..ebe50dfb8947 100644
--- a/fs/btrfs/transaction.c
+++ b/fs/btrfs/transaction.c
@@ -241,7 +241,7 @@ static noinline int join_transaction(struct btrfs_fs_info *fs_info,
 	refcount_set(&cur_trans->use_count, 2);
 	atomic_set(&cur_trans->pending_ordered, 0);
 	cur_trans->flags = 0;
-	cur_trans->start_time = get_seconds();
+	cur_trans->start_time = ktime_get_seconds();
 
 	memset(&cur_trans->delayed_refs, 0, sizeof(cur_trans->delayed_refs));
 
diff --git a/fs/btrfs/transaction.h b/fs/btrfs/transaction.h
index 94439482a0ec..4cbb1b55387d 100644
--- a/fs/btrfs/transaction.h
+++ b/fs/btrfs/transaction.h
@@ -48,7 +48,7 @@ struct btrfs_transaction {
 	int aborted;
 	struct list_head list;
 	struct extent_io_tree dirty_pages;
-	unsigned long start_time;
+	time64_t start_time;
 	wait_queue_head_t writer_wait;
 	wait_queue_head_t commit_wait;
 	wait_queue_head_t pending_wait;
-- 
2.9.0

[PATCH 2/3] btrfs: use 64-bit timestamps for struct btrfs_dev_replace_item

[Arnd Bergmann]

The structure already has 64-bit fields for the timestamps, but
calling get_seconds() may truncate and risk overflow on 32-bit
architectures.

This changes the dev-replace code to use ktime_get_real_seconds()
instead, which always returns 64-bit timestamps.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/btrfs/dev-replace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c
index e2ba0419297a..1b30c38d05c9 100644
--- a/fs/btrfs/dev-replace.c
+++ b/fs/btrfs/dev-replace.c
@@ -465,7 +465,7 @@ int btrfs_dev_replace_start(struct btrfs_fs_info *fs_info,
 	 * go to the tgtdev as well (refer to btrfs_map_block()).
 	 */
 	dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED;
-	dev_replace->time_started = get_seconds();
+	dev_replace->time_started = ktime_get_real_seconds();
 	dev_replace->cursor_left = 0;
 	dev_replace->committed_cursor_left = 0;
 	dev_replace->cursor_left_last_write_of_item = 0;
@@ -618,7 +618,7 @@ static int btrfs_dev_replace_finishing(struct btrfs_fs_info *fs_info,
 			  : BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED;
 	dev_replace->tgtdev = NULL;
 	dev_replace->srcdev = NULL;
-	dev_replace->time_stopped = get_seconds();
+	dev_replace->time_stopped = ktime_get_real_seconds();
 	dev_replace->item_needs_writeback = 1;
 
 	/* replace old device with new one in mapping tree */
@@ -807,7 +807,7 @@ int btrfs_dev_replace_cancel(struct btrfs_fs_info *fs_info)
 		break;
 	}
 	dev_replace->replace_state = BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED;
-	dev_replace->time_stopped = get_seconds();
+	dev_replace->time_stopped = ktime_get_real_seconds();
 	dev_replace->item_needs_writeback = 1;
 	btrfs_dev_replace_write_unlock(dev_replace);
 	btrfs_scrub_cancel(fs_info);
@@ -848,7 +848,7 @@ void btrfs_dev_replace_suspend_for_unmount(struct btrfs_fs_info *fs_info)
 	case BTRFS_IOCTL_DEV_REPLACE_STATE_STARTED:
 		dev_replace->replace_state =
 			BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED;
-		dev_replace->time_stopped = get_seconds();
+		dev_replace->time_stopped = ktime_get_real_seconds();
 		dev_replace->item_needs_writeback = 1;
 		btrfs_info(fs_info, "suspending dev_replace for unmount");
 		break;
-- 
2.9.0

[PATCH 3/3] btrfs: use timespec64 for i_otime

[Arnd Bergmann]

While the regular inode timestamps all use timespec64 now, the
i_otime field is btrfs specific and still needs to be converted
to correctly represent times beyond 2038.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/btrfs/btrfs_inode.h | 2 +-
 fs/btrfs/inode.c       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index 7e075343daa5..1343ac57b438 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -178,7 +178,7 @@ struct btrfs_inode {
 	struct btrfs_delayed_node *delayed_node;
 
 	/* File creation time. */
-	struct timespec i_otime;
+	struct timespec64 i_otime;
 
 	/* Hook into fs_info->delayed_iputs */
 	struct list_head delayed_iput;
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index e9482f0db9d0..22dcc8afd38f 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5745,7 +5745,7 @@ static struct inode *new_simple_dir(struct super_block *s,
 	inode->i_mtime = current_time(inode);
 	inode->i_atime = inode->i_mtime;
 	inode->i_ctime = inode->i_mtime;
-	BTRFS_I(inode)->i_otime = timespec64_to_timespec(inode->i_mtime);
+	BTRFS_I(inode)->i_otime = inode->i_mtime;
 
 	return inode;
 }
@@ -6349,7 +6349,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	inode->i_mtime = current_time(inode);
 	inode->i_atime = inode->i_mtime;
 	inode->i_ctime = inode->i_mtime;
-	BTRFS_I(inode)->i_otime = timespec64_to_timespec(inode->i_mtime);
+	BTRFS_I(inode)->i_otime = inode->i_mtime;
 
 	inode_item = btrfs_item_ptr(path->nodes[0], path->slots[0],
 				  struct btrfs_inode_item);
-- 
2.9.0

Re: [PATCH 2/3] btrfs: use 64-bit timestamps for struct btrfs_dev_replace_item

[David Sterba]

On Wed, Jun 20, 2018 at 04:34:33PM +0200, Arnd Bergmann wrote:
> The structure already has 64-bit fields for the timestamps, but
> calling get_seconds() may truncate and risk overflow on 32-bit
> architectures.
> 
> This changes the dev-replace code to use ktime_get_real_seconds()
> instead, which always returns 64-bit timestamps.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks but there's a patch already fixng that, sent a few days ago

https://patchwork.kernel.org/patch/10473195/

and added to patch queue for the next dev cycle as it does not appear to
urgent for 4.18.

Re: [PATCH 2/3] btrfs: use 64-bit timestamps for struct btrfs_dev_replace_item

[Arnd Bergmann]

On Wed, Jun 20, 2018 at 4:36 PM, David Sterba <dsterba@suse.cz> wrote:
> On Wed, Jun 20, 2018 at 04:34:33PM +0200, Arnd Bergmann wrote:
>> The structure already has 64-bit fields for the timestamps, but
>> calling get_seconds() may truncate and risk overflow on 32-bit
>> architectures.
>>
>> This changes the dev-replace code to use ktime_get_real_seconds()
>> instead, which always returns 64-bit timestamps.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks but there's a patch already fixng that, sent a few days ago
>
> https://patchwork.kernel.org/patch/10473195/
>
> and added to patch queue for the next dev cycle as it does not appear to
> urgent for 4.18.

Ok, sounds good.
I had missed that Allen has independently sent out some of the
same patches that I created in the last weeks.

Allen, do you have more patches pending? I have sent out most of what
I did (around 80 patches I think), with just ext4, ceph, nfs and xfs pending
at the moment. It seems we also sent identical patches for procfs and
I have something pending for ceph that duplicates another patch you did.

      Arnd

Re: [PATCH 3/3] btrfs: use timespec64 for i_otime

[David Sterba]

On Wed, Jun 20, 2018 at 04:34:34PM +0200, Arnd Bergmann wrote:
> While the regular inode timestamps all use timespec64 now, the
> i_otime field is btrfs specific and still needs to be converted
> to correctly represent times beyond 2038.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

This patch addresses the remaining type conversions, so I'm going to
merge it, thanks.

Re: [PATCH 3/3] btrfs: use timespec64 for i_otime

[Nikolay Borisov]

On 20.06.2018 19:38, David Sterba wrote:
> On Wed, Jun 20, 2018 at 04:34:34PM +0200, Arnd Bergmann wrote:
>> While the regular inode timestamps all use timespec64 now, the
>> i_otime field is btrfs specific and still needs to be converted
>> to correctly represent times beyond 2038.
>>
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> 
> This patch addresses the remaining type conversions, so I'm going to
> merge it, thanks.
> 

Actually for the sake of consistency we might want to merge this series
altogether. As it stands we now use ktime_get_seconds and
ktime_get_real_seconds (from Allen's patch). I haven't dug to see what's
the difference (if any) between the two .

Re: [PATCH 3/3] btrfs: use timespec64 for i_otime

[Arnd Bergmann]

On Wed, Jun 20, 2018 at 9:34 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>
>
> On 20.06.2018 19:38, David Sterba wrote:
>> On Wed, Jun 20, 2018 at 04:34:34PM +0200, Arnd Bergmann wrote:
>>> While the regular inode timestamps all use timespec64 now, the
>>> i_otime field is btrfs specific and still needs to be converted
>>> to correctly represent times beyond 2038.
>>>
>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>
>> This patch addresses the remaining type conversions, so I'm going to
>> merge it, thanks.
>>
>
> Actually for the sake of consistency we might want to merge this series
> altogether. As it stands we now use ktime_get_seconds and
> ktime_get_real_seconds (from Allen's patch). I haven't dug to see what's
> the difference (if any) between the two .

I just checked again and see that Allen's patch addresses the first two
of my three patches, but he picked a different approach for
transaction_kthread(): My patch moved to CLOCK_MONOTONIC,
while his version only changed the to time64_t but kept the
CLOCK_REALTIME behavior. It's a small difference, but I think
my version is slightly better. My patch 2/3 is identical to his version.

If you like, I can also rebase my patch 1/3 on top of his patch and
change it to CLOCK_MONOTONIC.

         Arnd

Re: [PATCH 3/3] btrfs: use timespec64 for i_otime

[Nikolay Borisov]

On 20.06.2018 22:41, Arnd Bergmann wrote:
> On Wed, Jun 20, 2018 at 9:34 PM, Nikolay Borisov <nborisov@suse.com> wrote:
>>
>>
>> On 20.06.2018 19:38, David Sterba wrote:
>>> On Wed, Jun 20, 2018 at 04:34:34PM +0200, Arnd Bergmann wrote:
>>>> While the regular inode timestamps all use timespec64 now, the
>>>> i_otime field is btrfs specific and still needs to be converted
>>>> to correctly represent times beyond 2038.
>>>>
>>>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>>>
>>> This patch addresses the remaining type conversions, so I'm going to
>>> merge it, thanks.
>>>
>>
>> Actually for the sake of consistency we might want to merge this series
>> altogether. As it stands we now use ktime_get_seconds and
>> ktime_get_real_seconds (from Allen's patch). I haven't dug to see what's
>> the difference (if any) between the two .
> 
> I just checked again and see that Allen's patch addresses the first two
> of my three patches, but he picked a different approach for
> transaction_kthread(): My patch moved to CLOCK_MONOTONIC,
> while his version only changed the to time64_t but kept the
> CLOCK_REALTIME behavior. It's a small difference, but I think
> my version is slightly better. My patch 2/3 is identical to his version.

I agree, in the transaction_kthread we are only interested in knowing
whether a fixed time windows (commit_internval) has passed. So monotonic
makes more sense here.
> 
> If you like, I can also rebase my patch 1/3 on top of his patch and
> change it to CLOCK_MONOTONIC.

Please do.

> 
>          Arnd
>