Skip to contentSkip to conversation

btrfs: use monotonic time for transaction handling

PATCH Superseded Arnd Bergmann 3 participants 9 messages Jun 20, 2018 – Jun 21, 2018 Btrfs file system

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Arnd Bergmann

author
#0 lore raw

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

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 <[email protected]>
---
 fs/btrfs/disk-io.c     | 4 ++--
 fs/btrfs/transaction.c | 2 +-
 fs/btrfs/transaction.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
fs/btrfs/disk-io.c +2 -2
index 205092dc9390..bf0717f2824d 100644
@@ -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 ||
fs/btrfs/transaction.c +1 -1
index ff5f6c719976..ebe50dfb8947 100644
@@ -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));
fs/btrfs/transaction.h +1 -1
index 94439482a0ec..4cbb1b55387d 100644
@@ -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;
Signature · 1 line
2.9.0
author
#1 lore raw

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

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 <[email protected]>
---
 fs/btrfs/dev-replace.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
fs/btrfs/dev-replace.c +4 -4
index e2ba0419297a..1b30c38d05c9 100644
@@ -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;
Signature · 1 line
2.9.0
#2 lore raw

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

Quoted 8 lines · 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 <[email protected]>

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.

author
#3 lore raw

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

Quoted 8 lines · David Sterba
Quoted 7 lines · 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.

Quoted 8 lines
Quoted 1 line
Signed-off-by Arnd Bergmann <[email protected]>

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

author
#4 lore raw

PATCH 3/3 btrfs: use timespec64 for i_otime

In reply to 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 <[email protected]>
---
 fs/btrfs/btrfs_inode.h | 2 +-
 fs/btrfs/inode.c       | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)
fs/btrfs/btrfs_inode.h +1 -1
index 7e075343daa5..1343ac57b438 100644
@@ -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;
fs/btrfs/inode.c +2 -2
index e9482f0db9d0..22dcc8afd38f 100644
@@ -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);
Signature · 1 line
2.9.0
#5 lore raw

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

Quoted 5 lines · 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 <[email protected]>

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

#6 lore raw

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

Quoted 10 lines · David Sterba
Quoted 5 lines · 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 <[email protected]>

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 .

author
#7 lore raw

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

Quoted 10 lines · Nikolay Borisov
Quoted 7 lines · David Sterba
Quoted 5 lines · 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 <[email protected]>
Quoted 8 lines
Quoted 3 lines

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

#8 lore raw

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

Quoted 20 lines · Arnd Bergmann
Quoted 10 lines · Nikolay Borisov
Quoted 7 lines · David Sterba
Quoted 5 lines · 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 <[email protected]>
Quoted 8 lines
Quoted 3 lines

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 .

Quoted 6 lines

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.

Quoted 3 lines

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

Please do.

Quoted 3 lines

Arnd