[6/6,RFC] ext4: super: extend timestamps to 40 bits

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 <arnd@arndb.de>

---
 fs/ext4/ext4.h  |  9 ++++++++-
 fs/ext4/super.c | 35 ++++++++++++++++++++++++++---------
 fs/ext4/sysfs.c | 18 ++++++++++++++++--
 3 files changed, 50 insertions(+), 12 deletions(-)

-- 
2.9.0

On Jun 20, 2018, at 9:33 AM, Arnd Bergmann <arnd@arndb.de> 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 <arnd@arndb.de>


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

On Thu, Jun 21, 2018 at 7:46 PM, Andreas Dilger <adilger@dilger.ca> wrote:

>> 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);


I generally try to avoid concatenating identifiers like this, as it makes
it much harder to grep for where a particular symbol or
struct member gets used.

>> +{

>> +     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);


Yes, good idea. I'm surprised we don't have a generic macro for that yet
(or maybe I just couldn't find it)


>> @@ -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.


Ok.

>> +{

>> +     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)


Ok. I'll integrate all of the above and post as a non-RFC patch then
after some testing.

      Arnd