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;
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;
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);
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.
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
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.
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 .
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
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
-
Arnd Bergmann -
David Sterba -
Nikolay Borisov