On Jun 20, 2018, at 9:33 AM, Arnd Bergmann wrote: > > The inode timestamps use 34 bits in ext4, but the various timestamps in > the superblock are limited to 32 bits. If every user accesses these as > 'unsigned', then this is good until year 2106, but it seems better to > extend this a bit further in the process of removing the deprecated > get_seconds() function. > > This adds another byte for each timestamp in the superblock, making > them long enough to store timestamps beyond what is in the inodes, > which seems good enough here (in ocfs2, they are already 64-bit wide, > which is appropriate for a new layout). > > I did not modify e2fsprogs, which obviously needs the same change to > actually interpret future timestamps correctly. > > Signed-off-by: Arnd Bergmann Patch looks functionally correct, some minor comments below that I think would improve it. > --- > fs/ext4/ext4.h | 9 ++++++++- > fs/ext4/super.c | 35 ++++++++++++++++++++++++++--------- > fs/ext4/sysfs.c | 18 ++++++++++++++++-- > 3 files changed, 50 insertions(+), 12 deletions(-) > > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 6b4f4369a08c..cac1464383e4 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -1294,7 +1294,14 @@ struct ext4_super_block { > __le32 s_lpf_ino; /* Location of the lost+found inode */ > __le32 s_prj_quota_inum; /* inode for tracking project quota */ > __le32 s_checksum_seed; /* crc32c(uuid) if csum_seed set */ > - __le32 s_reserved[98]; /* Padding to the end of the block */ > + __u8 s_wtime_hi; > + __u8 s_mtime_hi; > + __u8 s_mkfs_time_hi; > + __u8 s_lastcheck_hi; > + __u8 s_first_error_time_hi; > + __u8 s_last_error_time_hi; > + __u8 s_pad[2]; > + __le32 s_reserved[96]; /* Padding to the end of the block */ > __le32 s_checksum; /* crc32c(superblock) */ > }; > > diff --git a/fs/ext4/super.c b/fs/ext4/super.c > index 0c4c2201b3aa..2063d4e5ed08 100644 > --- a/fs/ext4/super.c > +++ b/fs/ext4/super.c > @@ -312,6 +312,20 @@ void ext4_itable_unused_set(struct super_block *sb, > bg->bg_itable_unused_hi = cpu_to_le16(count >> 16); > } > > +static void ext4_update_tstamp(__le32 *lo, __u8 *hi) Would it be better to wrap this in a macro, something like: #define ext4_update_tstamp(es, tstamp) \ __ext4_update_tstamp(&(es)->tstamp, &(es)->tstamp ## _hi) #define ext4_get_tstamp(es, tstamp) \ __ext4_get_tstamp(&(es)->tstamp, &(es)->tstamp ## _hi) So that it can be used in the callers more easily: ext4_update_tstamp(es, s_last_error_time); time = ext4_get_tstamp(es, s_last_error_time); > +{ > + time64_t now = ktime_get_real_seconds(); > + > + now = clamp_val(now, 0, 0xffffffffffull); Long strings of "0xfff..." are hard to get correct. This looks right, but it may be easier to be sure it is correct with something like: /* timestamps have a 32-bit low field and 8-bit high field */ now = clamp_val(now, 0, (1ULL << 40) - 1); > + > + *lo = cpu_to_le32(lower_32_bits(now)); > + *hi = upper_32_bits(now); > +} > + > +static time64_t ext4_get_tstamp(__le32 *lo, __u8 *hi) > +{ > + return ((time64_t)(*hi) << 32) + le32_to_cpu(*lo); > +} > > static void __save_error_info(struct super_block *sb, const char *func, > unsigned int line) > @@ -322,11 +336,12 @@ static void __save_error_info(struct super_block *sb, const char *func, > if (bdev_read_only(sb->s_bdev)) > return; > es->s_state |= cpu_to_le16(EXT4_ERROR_FS); > - es->s_last_error_time = cpu_to_le32(get_seconds()); > + ext4_update_tstamp(&es->s_last_error_time, &es->s_last_error_time_hi); > strncpy(es->s_last_error_func, func, sizeof(es->s_last_error_func)); > es->s_last_error_line = cpu_to_le32(line); > if (!es->s_first_error_time) { > es->s_first_error_time = es->s_last_error_time; > + es->s_first_error_time_hi = es->s_last_error_time_hi; > strncpy(es->s_first_error_func, func, > sizeof(es->s_first_error_func)); > es->s_first_error_line = cpu_to_le32(line); > @@ -2163,8 +2178,8 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es, > "warning: maximal mount count reached, " > "running e2fsck is recommended"); > else if (le32_to_cpu(es->s_checkinterval) && > - (le32_to_cpu(es->s_lastcheck) + > - le32_to_cpu(es->s_checkinterval) <= get_seconds())) > + (ext4_get_tstamp(&es->s_lastcheck, &es->s_lastcheck_hi) + > + le32_to_cpu(es->s_checkinterval) <= ktime_get_real_seconds())) > ext4_msg(sb, KERN_WARNING, > "warning: checktime reached, " > "running e2fsck is recommended"); > @@ -2173,7 +2188,7 @@ static int ext4_setup_super(struct super_block *sb, struct ext4_super_block *es, > if (!(__s16) le16_to_cpu(es->s_max_mnt_count)) > es->s_max_mnt_count = cpu_to_le16(EXT4_DFL_MAX_MNT_COUNT); > le16_add_cpu(&es->s_mnt_count, 1); > - es->s_mtime = cpu_to_le32(get_seconds()); > + ext4_update_tstamp(&es->s_mtime, &es->s_mtime_hi); > ext4_update_dynamic_rev(sb); > if (sbi->s_journal) > ext4_set_feature_journal_needs_recovery(sb); > @@ -2839,8 +2854,9 @@ static void print_daily_error_info(struct timer_list *t) > ext4_msg(sb, KERN_NOTICE, "error count since last fsck: %u", > le32_to_cpu(es->s_error_count)); > if (es->s_first_error_time) { > - printk(KERN_NOTICE "EXT4-fs (%s): initial error at time %u: %.*s:%d", > - sb->s_id, le32_to_cpu(es->s_first_error_time), > + printk(KERN_NOTICE "EXT4-fs (%s): initial error at time %llu: %.*s:%d", > + sb->s_id, > + ext4_get_tstamp(&es->s_first_error_time, &es->s_first_error_time_hi), > (int) sizeof(es->s_first_error_func), > es->s_first_error_func, > le32_to_cpu(es->s_first_error_line)); > @@ -2853,8 +2869,9 @@ static void print_daily_error_info(struct timer_list *t) > printk(KERN_CONT "\n"); > } > if (es->s_last_error_time) { > - printk(KERN_NOTICE "EXT4-fs (%s): last error at time %u: %.*s:%d", > - sb->s_id, le32_to_cpu(es->s_last_error_time), > + printk(KERN_NOTICE "EXT4-fs (%s): last error at time %llu: %.*s:%d", > + sb->s_id, > + ext4_get_tstamp(&es->s_last_error_time, &es->s_last_error_time_hi), > (int) sizeof(es->s_last_error_func), > es->s_last_error_func, > le32_to_cpu(es->s_last_error_line)); > @@ -4747,7 +4764,7 @@ static int ext4_commit_super(struct super_block *sb, int sync) > * to complain and force a full file system check. > */ > if (!(sb->s_flags & SB_RDONLY)) > - es->s_wtime = cpu_to_le32(get_seconds()); > + ext4_update_tstamp(&es->s_wtime, &es->s_wtime_hi); > if (sb->s_bdev->bd_part) > es->s_kbytes_written = > cpu_to_le64(EXT4_SB(sb)->s_kbytes_written + > diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c > index b970a200f20c..fe58aa905cbe 100644 > --- a/fs/ext4/sysfs.c > +++ b/fs/ext4/sysfs.c > @@ -25,6 +25,8 @@ typedef enum { > attr_reserved_clusters, > attr_inode_readahead, > attr_trigger_test_error, > + attr_first_error_time, > + attr_last_error_time, > attr_feature, > attr_pointer_ui, > attr_pointer_atomic, > @@ -182,8 +184,8 @@ EXT4_RW_ATTR_SBI_UI(warning_ratelimit_burst, s_warning_ratelimit_state.burst); > EXT4_RW_ATTR_SBI_UI(msg_ratelimit_interval_ms, s_msg_ratelimit_state.interval); > EXT4_RW_ATTR_SBI_UI(msg_ratelimit_burst, s_msg_ratelimit_state.burst); > EXT4_RO_ATTR_ES_UI(errors_count, s_error_count); > -EXT4_RO_ATTR_ES_UI(first_error_time, s_first_error_time); > -EXT4_RO_ATTR_ES_UI(last_error_time, s_last_error_time); > +EXT4_ATTR(first_error_time, 0444, first_error_time); > +EXT4_ATTR(last_error_time, 0444, last_error_time); > > static unsigned int old_bump_val = 128; > EXT4_ATTR_PTR(max_writeback_mb_bump, 0444, pointer_ui, &old_bump_val); > @@ -249,6 +251,12 @@ static void *calc_ptr(struct ext4_attr *a, struct ext4_sb_info *sbi) > return NULL; > } > > +static ssize_t print_time(char *buf, __le32 lo, __u8 hi) It would probably be more consistent to name this "print_tstamp()" since it isn't strictly a "time" as one would expect. > +{ > + return snprintf(buf, PAGE_SIZE, "%lld", > + ((time64_t)hi << 32) + le32_to_cpu(lo)); > +} Similarly, wrap this with: #define print_tstamp(buf, es, tstamp) \ __print_tstamp(buf, &(es)->tstamp, &(es)->tstamp ## _hi) > @@ -287,6 +295,12 @@ static ssize_t ext4_attr_show(struct kobject *kobj, > atomic_read((atomic_t *) ptr)); > case attr_feature: > return snprintf(buf, PAGE_SIZE, "supported\n"); > + case attr_first_error_time: > + return print_time(buf, sbi->s_es->s_first_error_time, > + sbi->s_es->s_first_error_time_hi); > + case attr_last_error_time: > + return print_time(buf, sbi->s_es->s_last_error_time, > + sbi->s_es->s_last_error_time_hi); > } > > return 0; > -- > 2.9.0 > Cheers, Andreas