[08/11] xfs: widen ondisk timestamps to deal with y2038 problem

Commit Message

Darrick J. Wong Aug. 21, 2020, 2:12 a.m. UTC

From: Darrick J. Wong <[e-mail]>

Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
time epoch).  This enables us to handle dates up to 2486, which solves
the y2038 problem.

Signed-off-by: Darrick J. Wong <[e-mail]>
Reviewed-by: Amir Goldstein <[e-mail]>
---
 fs/xfs/libxfs/xfs_format.h      |   38 +++++++++++++++++++++++++++
 fs/xfs/libxfs/xfs_fs.h          |    1 +
 fs/xfs/libxfs/xfs_inode_buf.c   |   54 +++++++++++++++++++++++++++++++++------
 fs/xfs/libxfs/xfs_inode_buf.h   |    8 +++---
 fs/xfs/libxfs/xfs_log_format.h  |    3 ++
 fs/xfs/libxfs/xfs_sb.c          |    2 +
 fs/xfs/libxfs/xfs_trans_inode.c |   11 ++++++++
 fs/xfs/scrub/inode.c            |   16 ++++++++----
 fs/xfs/xfs_inode.c              |   15 ++++++++++-
 fs/xfs/xfs_inode_item.c         |   33 ++++++++++++++++++------
 fs/xfs/xfs_ioctl.c              |    3 +-
 fs/xfs/xfs_ondisk.h             |    6 ++++
 fs/xfs/xfs_super.c              |   13 ++++++++-
 13 files changed, 173 insertions(+), 30 deletions(-)

Comments

Christoph Hellwig Aug. 22, 2020, 7:33 a.m. UTC | #1

>   * in the AGI header so that we can skip the finobt walk at mount time when
> @@ -855,12 +862,18 @@ struct xfs_agfl {
>   *
>   * Inode timestamps consist of signed 32-bit counters for seconds and
>   * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
> + *
> + * When bigtime is enabled, timestamps become an unsigned 64-bit nanoseconds
> + * counter.  Time zero is the start of the classic timestamp range.
>   */
>  union xfs_timestamp {
>  	struct {
>  		__be32		t_sec;		/* timestamp seconds */
>  		__be32		t_nsec;		/* timestamp nanoseconds */
>  	};
> +
> +	/* Nanoseconds since the bigtime epoch. */
> +	__be64			t_bigtime;
>  };

So do we really need the union here?  What about:

 (1) keep the typedef instead of removing it
 (2) switch the typedef to be just a __be64, and use trivial helpers
     to extract the two separate legacy sec/nsec field
 (3) PROFIT!!!

> +/* Convert an ondisk timestamp into the 64-bit safe incore format. */
>  void
>  xfs_inode_from_disk_timestamp(
> +	struct xfs_dinode		*dip,
>  	struct timespec64		*tv,
>  	const union xfs_timestamp	*ts)

I think passing ts by value might lead to somewhat better code
generation on modern ABIs (and older ABIs just fall back to pass
by reference transparently).

>  {
> +	if (dip->di_version >= 3 &&
> +	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {

Do we want a helper for this condition?

> +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> +		uint64_t		s;
> +		uint32_t		n;
> +
> +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> +		tv->tv_nsec = n;
> +		return;
> +	}
> +
>  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
>  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);

Nit: for these kinds of symmetric conditions and if/else feels a little
more natural.

> +		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);

This adds a > 80 char line.

> +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> +		uint64_t		t;
> +
> +		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
> +		t *= NSEC_PER_SEC;
> +		its->t_bigtime = t + ts->tv_nsec;

This calculation is dupliated in two places, might be worth
adding a little helper (which will need to get the sec/nsec values
passed separately due to the different structures).

> +		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);

Another line over 8 characters here.

> +	if (xfs_sb_version_hasbigtime(&mp->m_sb)) {
> +		sb->s_time_min = XFS_INO_BIGTIME_MIN;
> +		sb->s_time_max = XFS_INO_BIGTIME_MAX;
> +	} else {
> +		sb->s_time_min = XFS_INO_TIME_MIN;
> +		sb->s_time_max = XFS_INO_TIME_MAX;
> +	}

This is really a comment on the earlier patch, but maybe we should
name the old constants with "OLD" or "LEGACY" or "SMALL" in the name?

> @@ -1494,6 +1499,10 @@ xfs_fc_fill_super(
>  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
>  		sb->s_flags |= SB_I_VERSION;
>  
> +	if (xfs_sb_version_hasbigtime(&mp->m_sb))
> +		xfs_warn(mp,
> + "EXPERIMENTAL big timestamp feature in use. Use at your own risk!");
> +

Is there any good reason to mark this experimental?

Dave Chinner Aug. 24, 2020, 1:25 a.m. UTC | #2

On Thu, Aug 20, 2020 at 07:12:21PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <[e-mail]>
> 
> Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
> nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
> time epoch).  This enables us to handle dates up to 2486, which solves
> the y2038 problem.
> 
> Signed-off-by: Darrick J. Wong <[e-mail]>
> Reviewed-by: Amir Goldstein <[e-mail]>

....

> @@ -875,6 +888,25 @@ union xfs_timestamp {
>   */
>  #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
>  
> +/*
> + * Number of seconds between the start of the bigtime timestamp range and the
> + * start of the Unix epoch.
> + */
> +#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)

This is confusing. It's taken me 15 minutes so far to get my head
around this because the reference frame for all these definitions is
not clear. I though these had something to do with nanosecond
timestamp limits because that's what BIGTIME records, but.....

The start of the epoch is a negative number based on the definition
of the on-disk format for the minimum number of seconds that the
"Unix" timestamp format can store?  Why is this not defined in
nanoseconds given that is what is stored on disk?

XFS_INO_BIGTIME_EPOCH = (-XFS_INO_TIME_MIN)
			= (-((int64_t)S32_MIN))
			= (-((int64_t)-2^31))
			= 2^31?

So the bigtime epoch is considered to be 2^31 *seconds* into the
range of the on-disk nanosecond timestamp? Huh?

> +
> +/*
> + * Smallest possible timestamp with big timestamps, which is
> + * Dec 13 20:45:52 UTC 1901.
> + */
> +#define XFS_INO_BIGTIME_MIN	(XFS_INO_TIME_MIN)

Same here. The reference here is "seconds before the Unix epoch",
not the minimum valid on disk nanosecond value.

Oh, these are defining the post-disk-to-in-memory-format conversion
limits? They have nothing to do with on-disk limits nor that on-disk
format is stored in nanosecond? i.e. the reference frame for these
limits is actually still the in-kernel Unix epoch definition?

> +/*
> + * Largest possible timestamp with big timestamps, which is
> + * Jul  2 20:20:25 UTC 2486.
> + */
> +#define XFS_INO_BIGTIME_MAX	((int64_t)((-1ULL / NSEC_PER_SEC) - \
> +					   XFS_INO_BIGTIME_EPOCH))

Urk. This makes my head -hurt-.

It's converting the maximum on-disk format nanosecond count to a
maximum second count then taking away the second count for the epoch
and then casting it to a signed int because the in-memory format for
seconds is signed? Oh, and the 64 bit division is via constants, so
the compiler does it and doesn't need to use something like
div_u64(), right?

Mind you, I'm just guessing here that the "-1ULL" is the
representation of the maximum on-disk nanosecond timestamp here,
because that doesn't seem to be defined anywhere....


> diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> index cc1316a5fe0c..c59ddb56bb90 100644
> --- a/fs/xfs/libxfs/xfs_inode_buf.c
> +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> @@ -157,11 +157,25 @@ xfs_imap_to_bp(
>  	return 0;
>  }
>  
> +/* Convert an ondisk timestamp into the 64-bit safe incore format. */
>  void
>  xfs_inode_from_disk_timestamp(
> +	struct xfs_dinode		*dip,
>  	struct timespec64		*tv,
>  	const union xfs_timestamp	*ts)
>  {
> +	if (dip->di_version >= 3 &&
> +	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {

Helper. xfs_dinode_has_bigtime() was what I suggested previously.

> +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> +		uint64_t		s;
> +		uint32_t		n;
> +
> +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> +		tv->tv_nsec = n;
> +		return;
> +	}
> +
>  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
>  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
>  }

I still don't really like the way this turned out :(

> @@ -220,9 +234,9 @@ xfs_inode_from_disk(
>  	 * a time before epoch is converted to a time long after epoch
>  	 * on 64 bit systems.
>  	 */
> -	xfs_inode_from_disk_timestamp(&inode->i_atime, &from->di_atime);
> -	xfs_inode_from_disk_timestamp(&inode->i_mtime, &from->di_mtime);
> -	xfs_inode_from_disk_timestamp(&inode->i_ctime, &from->di_ctime);
> +	xfs_inode_from_disk_timestamp(from, &inode->i_atime, &from->di_atime);
> +	xfs_inode_from_disk_timestamp(from, &inode->i_mtime, &from->di_mtime);
> +	xfs_inode_from_disk_timestamp(from, &inode->i_ctime, &from->di_ctime);
>  
>  	to->di_size = be64_to_cpu(from->di_size);
>  	to->di_nblocks = be64_to_cpu(from->di_nblocks);
> @@ -235,9 +249,17 @@ xfs_inode_from_disk(
>  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
>  		inode_set_iversion_queried(inode,
>  					   be64_to_cpu(from->di_changecount));
> -		xfs_inode_from_disk_timestamp(&to->di_crtime, &from->di_crtime);
> +		xfs_inode_from_disk_timestamp(from, &to->di_crtime,
> +				&from->di_crtime);
>  		to->di_flags2 = be64_to_cpu(from->di_flags2);
>  		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
> +		/*
> +		 * Set the bigtime flag incore so that we automatically convert
> +		 * this inode's ondisk timestamps to bigtime format the next
> +		 * time we write the inode core to disk.
> +		 */
> +		if (xfs_sb_version_hasbigtime(&ip->i_mount->m_sb))
> +			to->di_flags2 |= XFS_DIFLAG2_BIGTIME;
>  	}

iAs I said before, you can't set this flag here - it needs to be
done transactionally when the timestamp is next logged.


> @@ -259,9 +281,19 @@ xfs_inode_from_disk(
>  
>  void
>  xfs_inode_to_disk_timestamp(
> +	struct xfs_icdinode		*from,
>  	union xfs_timestamp		*ts,
>  	const struct timespec64		*tv)
>  {
> +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {

Shouldn't this also check the inode version number like the dinode
decoder? Perhaps a helper like xfs_inode_has_bigtime(ip)?

> +		uint64_t		t;
> +
> +		t = (uint64_t)(tv->tv_sec + XFS_INO_BIGTIME_EPOCH);

tv_sec can still overflow, right?

		t = (uint64_t)tv->tv_sec + XFS_INO_BIGTIME_EPOCH;

> +		t *= NSEC_PER_SEC;
> +		ts->t_bigtime = cpu_to_be64(t + tv->tv_nsec);
> +		return;
> +	}
> +
>  	ts->t_sec = cpu_to_be32(tv->tv_sec);
>  	ts->t_nsec = cpu_to_be32(tv->tv_nsec);
>  }
> @@ -285,9 +317,9 @@ xfs_inode_to_disk(
>  	to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
>  
>  	memset(to->di_pad, 0, sizeof(to->di_pad));
> -	xfs_inode_to_disk_timestamp(&to->di_atime, &inode->i_atime);
> -	xfs_inode_to_disk_timestamp(&to->di_mtime, &inode->i_mtime);
> -	xfs_inode_to_disk_timestamp(&to->di_ctime, &inode->i_ctime);
> +	xfs_inode_to_disk_timestamp(from, &to->di_atime, &inode->i_atime);
> +	xfs_inode_to_disk_timestamp(from, &to->di_mtime, &inode->i_mtime);
> +	xfs_inode_to_disk_timestamp(from, &to->di_ctime, &inode->i_ctime);
>  	to->di_nlink = cpu_to_be32(inode->i_nlink);
>  	to->di_gen = cpu_to_be32(inode->i_generation);
>  	to->di_mode = cpu_to_be16(inode->i_mode);
> @@ -306,7 +338,8 @@ xfs_inode_to_disk(
>  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
>  		to->di_version = 3;
>  		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
> -		xfs_inode_to_disk_timestamp(&to->di_crtime, &from->di_crtime);
> +		xfs_inode_to_disk_timestamp(from, &to->di_crtime,
> +				&from->di_crtime);
>  		to->di_flags2 = cpu_to_be64(from->di_flags2);
>  		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
>  		to->di_ino = cpu_to_be64(ip->i_ino);
> @@ -526,6 +559,11 @@ xfs_dinode_verify(
>  	if (fa)
>  		return fa;
>  
> +	/* bigtime iflag can only happen on bigtime filesystems */
> +	if ((flags2 & (XFS_DIFLAG2_BIGTIME)) &&
> +	    !xfs_sb_version_hasbigtime(&mp->m_sb))

	if (xfs_dinode_has_bigtime(dip) &&
	    !xfs_sb_version_hasbigtime(&mp->m_sb))

> +void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
> +		union xfs_timestamp *ts, const struct timespec64 *tv);
>  
>  #endif	/* __XFS_INODE_BUF_H__ */
> diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> index 17c83d29998c..569721f7f9e5 100644
> --- a/fs/xfs/libxfs/xfs_log_format.h
> +++ b/fs/xfs/libxfs/xfs_log_format.h
> @@ -373,6 +373,9 @@ union xfs_ictimestamp {
>  		int32_t		t_sec;		/* timestamp seconds */
>  		int32_t		t_nsec;		/* timestamp nanoseconds */
>  	};
> +
> +	/* Nanoseconds since the bigtime epoch. */
> +	uint64_t		t_bigtime;
>  };

Where are we using this again? Right now the timestamps are
converted directly into the VFS inode timestamp fields so we can get
rid of these incore timestamp fields. So shouldn't we be trying to
get rid of this structure rather than adding more functionality to
it?

> @@ -131,6 +131,17 @@ xfs_trans_log_inode(
>  			iversion_flags = XFS_ILOG_CORE;
>  	}
>  
> +	/*
> +	 * If we're updating the inode core or the timestamps and it's possible
> +	 * to upgrade this inode to bigtime format, do so now.
> +	 */
> +	if ((flags & (XFS_ILOG_CORE | XFS_ILOG_TIMESTAMP)) &&
> +	    xfs_sb_version_hasbigtime(&tp->t_mountp->m_sb) &&
> +	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {

The latter two checks could be wrapped up into a helper named
something obvious like xfs_inode_need_bigtime(ip)?

> +		ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> +		flags |= XFS_ILOG_CORE;
> +	}
> +
>  	/*
>  	 * Record the specific change for fdatasync optimisation. This allows
>  	 * fdatasync to skip log forces for inodes that are only timestamp
> diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> index 9f036053fdb7..6f00309de2d4 100644
> --- a/fs/xfs/scrub/inode.c
> +++ b/fs/xfs/scrub/inode.c
> @@ -190,6 +190,11 @@ xchk_inode_flags2(
>  	if ((flags2 & XFS_DIFLAG2_DAX) && (flags2 & XFS_DIFLAG2_REFLINK))
>  		goto bad;
>  
> +	/* no bigtime iflag without the bigtime feature */
> +	if (!xfs_sb_version_hasbigtime(&mp->m_sb) &&
> +	    (flags2 & XFS_DIFLAG2_BIGTIME))

Can we get all these open coded checks wrapped up into a single
helper?

> +		xchk_dinode_nsec(sc, ino, dip, &dip->di_crtime);
>  		xchk_inode_flags2(sc, dip, ino, mode, flags, flags2);
>  		xchk_inode_cowextsize(sc, dip, ino, mode, flags,
>  				flags2);
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index c06129cffba9..bbc309bc70c4 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -841,6 +841,8 @@ xfs_ialloc(
>  	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
>  		inode_set_iversion(inode, 1);
>  		ip->i_d.di_flags2 = 0;
> +		if (xfs_sb_version_hasbigtime(&mp->m_sb))
> +			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;

Rather than calculate the initial inode falgs on every allocation,
shouldn't we just have the defaults pre-calculated at mount time?

>  		ip->i_d.di_cowextsize = 0;
>  		ip->i_d.di_crtime = tv;
>  	}
> @@ -2717,7 +2719,11 @@ xfs_ifree(
>  
>  	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
>  	ip->i_d.di_flags = 0;
> -	ip->i_d.di_flags2 = 0;
> +	/*
> +	 * Preserve the bigtime flag so that di_ctime accurately stores the
> +	 * deletion time.
> +	 */
> +	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;

Oh, that's a nasty wart.

>  	ip->i_d.di_dmevmask = 0;
>  	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
>  	ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
> @@ -3503,6 +3509,13 @@ xfs_iflush(
>  			__func__, ip->i_ino, ip->i_d.di_forkoff, ip);
>  		goto flush_out;
>  	}
> +	if (xfs_sb_version_hasbigtime(&mp->m_sb) &&
> +	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {
> +		xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> +			"%s: bad inode %Lu, bigtime unset, ptr "PTR_FMT,
> +			__func__, ip->i_ino, ip);
> +		goto flush_out;
> +	}

Why is this a fatal corruption error? if the bigtime flag is not
set, we can still store and read the timestamp if it is within
the unix epoch range...

>  
>  	/*
>  	 * Inode item log recovery for v2 inodes are dependent on the
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index cec6d4d82bc4..c38af3eea48f 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -298,9 +298,16 @@ xfs_inode_item_format_attr_fork(
>  /* Write a log_dinode timestamp into an ondisk inode timestamp. */
>  static inline void
>  xfs_log_dinode_to_disk_ts(
> +	struct xfs_log_dinode		*from,
>  	union xfs_timestamp		*ts,
>  	const union xfs_ictimestamp	*its)
>  {
> +	if (from->di_version >= 3 &&
> +	    (from->di_flags2 & XFS_DIFLAG2_BIGTIME)) {

helper.

> +		ts->t_bigtime = cpu_to_be64(its->t_bigtime);
> +		return;
> +	}
> +
>  	ts->t_sec = cpu_to_be32(its->t_sec);
>  	ts->t_nsec = cpu_to_be32(its->t_nsec);
>  }
> @@ -322,9 +329,9 @@ xfs_log_dinode_to_disk(
>  	to->di_projid_hi = cpu_to_be16(from->di_projid_hi);
>  	memcpy(to->di_pad, from->di_pad, sizeof(to->di_pad));
>  
> -	xfs_log_dinode_to_disk_ts(&to->di_atime, &from->di_atime);
> -	xfs_log_dinode_to_disk_ts(&to->di_mtime, &from->di_mtime);
> -	xfs_log_dinode_to_disk_ts(&to->di_ctime, &from->di_ctime);
> +	xfs_log_dinode_to_disk_ts(from, &to->di_atime, &from->di_atime);
> +	xfs_log_dinode_to_disk_ts(from, &to->di_mtime, &from->di_mtime);
> +	xfs_log_dinode_to_disk_ts(from, &to->di_ctime, &from->di_ctime);
>  
>  	to->di_size = cpu_to_be64(from->di_size);
>  	to->di_nblocks = cpu_to_be64(from->di_nblocks);
> @@ -340,7 +347,7 @@ xfs_log_dinode_to_disk(
>  
>  	if (from->di_version == 3) {
>  		to->di_changecount = cpu_to_be64(from->di_changecount);
> -		xfs_log_dinode_to_disk_ts(&to->di_crtime, &from->di_crtime);
> +		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);
>  		to->di_flags2 = cpu_to_be64(from->di_flags2);
>  		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
>  		to->di_ino = cpu_to_be64(from->di_ino);
> @@ -356,9 +363,19 @@ xfs_log_dinode_to_disk(
>  /* Write an incore inode timestamp into a log_dinode timestamp. */
>  static inline void
>  xfs_inode_to_log_dinode_ts(
> +	struct xfs_icdinode		*from,
>  	union xfs_ictimestamp		*its,
>  	const struct timespec64		*ts)
>  {
> +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> +		uint64_t		t;
> +
> +		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
> +		t *= NSEC_PER_SEC;
> +		its->t_bigtime = t + ts->tv_nsec;

helper,

> +		return;
> +	}
> +
>  	its->t_sec = ts->tv_sec;
>  	its->t_nsec = ts->tv_nsec;
>  }
> @@ -381,9 +398,9 @@ xfs_inode_to_log_dinode(
>  
>  	memset(to->di_pad, 0, sizeof(to->di_pad));
>  	memset(to->di_pad3, 0, sizeof(to->di_pad3));
> -	xfs_inode_to_log_dinode_ts(&to->di_atime, &inode->i_atime);
> -	xfs_inode_to_log_dinode_ts(&to->di_mtime, &inode->i_mtime);
> -	xfs_inode_to_log_dinode_ts(&to->di_ctime, &inode->i_ctime);
> +	xfs_inode_to_log_dinode_ts(from, &to->di_atime, &inode->i_atime);
> +	xfs_inode_to_log_dinode_ts(from, &to->di_mtime, &inode->i_mtime);
> +	xfs_inode_to_log_dinode_ts(from, &to->di_ctime, &inode->i_ctime);
>  	to->di_nlink = inode->i_nlink;
>  	to->di_gen = inode->i_generation;
>  	to->di_mode = inode->i_mode;
> @@ -405,7 +422,7 @@ xfs_inode_to_log_dinode(
>  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
>  		to->di_version = 3;
>  		to->di_changecount = inode_peek_iversion(inode);
> -		xfs_inode_to_log_dinode_ts(&to->di_crtime, &from->di_crtime);
> +		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);
>  		to->di_flags2 = from->di_flags2;
>  		to->di_cowextsize = from->di_cowextsize;
>  		to->di_ino = ip->i_ino;
> diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> index 6f22a66777cd..13396c3665d1 100644
> --- a/fs/xfs/xfs_ioctl.c
> +++ b/fs/xfs/xfs_ioctl.c
> @@ -1190,7 +1190,8 @@ xfs_flags2diflags2(
>  	unsigned int		xflags)
>  {
>  	uint64_t		di_flags2 =
> -		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
> +		(ip->i_d.di_flags2 & (XFS_DIFLAG2_REFLINK |
> +				      XFS_DIFLAG2_BIGTIME));
>  
>  	if (xflags & FS_XFLAG_DAX)
>  		di_flags2 |= XFS_DIFLAG2_DAX;
> diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> index 7158a8de719f..3e0c677cff15 100644
> --- a/fs/xfs/xfs_ondisk.h
> +++ b/fs/xfs/xfs_ondisk.h
> @@ -25,6 +25,9 @@ xfs_check_limits(void)
>  	/* make sure timestamp limits are correct */
>  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
>  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
> +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);

That still just doesn't look right to me :/

This implies that the epoch is 2^32 seconds after then minimum
supported time (2038), when in fact it is only 2^31 seconds after the
minimum supported timestamp (1970). :/

> +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MAX,			16299260425LL);

Hmmm. I got 16299260424 when I just ran this through a simple calc.
Mind you, no calculator app I found could handle unsigned 64 bit
values natively (signed 64 bit is good enough for everyone!) so
maybe I got an off-by one here...

Cheers,

Dave.

Darrick J. Wong Aug. 24, 2020, 2:43 a.m. UTC | #3

On Sat, Aug 22, 2020 at 08:33:19AM +0100, Christoph Hellwig wrote:
> >   * in the AGI header so that we can skip the finobt walk at mount time when
> > @@ -855,12 +862,18 @@ struct xfs_agfl {
> >   *
> >   * Inode timestamps consist of signed 32-bit counters for seconds and
> >   * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
> > + *
> > + * When bigtime is enabled, timestamps become an unsigned 64-bit nanoseconds
> > + * counter.  Time zero is the start of the classic timestamp range.
> >   */
> >  union xfs_timestamp {
> >  	struct {
> >  		__be32		t_sec;		/* timestamp seconds */
> >  		__be32		t_nsec;		/* timestamp nanoseconds */
> >  	};
> > +
> > +	/* Nanoseconds since the bigtime epoch. */
> > +	__be64			t_bigtime;
> >  };
> 
> So do we really need the union here?  What about:
> 
>  (1) keep the typedef instead of removing it
>  (2) switch the typedef to be just a __be64, and use trivial helpers
>      to extract the two separate legacy sec/nsec field
>  (3) PROFIT!!!

Been there, done that.  Dave suggested some replacement code (which
corrupted the values), then I modified that into a correct version,
which then made smatch angry because it doesn't like code that does bit
shifts on __be64 values.

> > +/* Convert an ondisk timestamp into the 64-bit safe incore format. */
> >  void
> >  xfs_inode_from_disk_timestamp(
> > +	struct xfs_dinode		*dip,
> >  	struct timespec64		*tv,
> >  	const union xfs_timestamp	*ts)
> 
> I think passing ts by value might lead to somewhat better code
> generation on modern ABIs (and older ABIs just fall back to pass
> by reference transparently).

Hm, ok.  I did not know that. :)

> >  {
> > +	if (dip->di_version >= 3 &&
> > +	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {
> 
> Do we want a helper for this condition?

Yes, yes we do.  Will add.

> > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > +		uint64_t		s;
> > +		uint32_t		n;
> > +
> > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > +		tv->tv_nsec = n;
> > +		return;
> > +	}
> > +
> >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> 
> Nit: for these kinds of symmetric conditions and if/else feels a little
> more natural.
> 
> > +		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);
> 
> This adds a > 80 char line.

Do we care now that checkpatch has been changed to allow up to 100
columns?

> > +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> > +		uint64_t		t;
> > +
> > +		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
> > +		t *= NSEC_PER_SEC;
> > +		its->t_bigtime = t + ts->tv_nsec;
> 
> This calculation is dupliated in two places, might be worth
> adding a little helper (which will need to get the sec/nsec values
> passed separately due to the different structures).
> 
> > +		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);
> 
> Another line over 8 characters here.
> 
> > +	if (xfs_sb_version_hasbigtime(&mp->m_sb)) {
> > +		sb->s_time_min = XFS_INO_BIGTIME_MIN;
> > +		sb->s_time_max = XFS_INO_BIGTIME_MAX;
> > +	} else {
> > +		sb->s_time_min = XFS_INO_TIME_MIN;
> > +		sb->s_time_max = XFS_INO_TIME_MAX;
> > +	}
> 
> This is really a comment on the earlier patch, but maybe we should
> name the old constants with "OLD" or "LEGACY" or "SMALL" in the name?

Yes, good suggestion!

> > @@ -1494,6 +1499,10 @@ xfs_fc_fill_super(
> >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> >  		sb->s_flags |= SB_I_VERSION;
> >  
> > +	if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > +		xfs_warn(mp,
> > + "EXPERIMENTAL big timestamp feature in use. Use at your own risk!");
> > +
> 
> Is there any good reason to mark this experimental?

As you and Dave have both pointed out, there are plenty of stupid bugs
still in this.  I think I'd like to have at least one EXPERIMENTAL cycle
to make sure I didn't commit anything pathologically stupid in here.

<cough> ext4 34-bit sign extension bug <cough>.

--D

Darrick J. Wong Aug. 24, 2020, 3:13 a.m. UTC | #4

On Mon, Aug 25, 2020 at 11:25:27AM +1000, Dave Chinner wrote:
> On Thu, Aug 20, 2020 at 07:12:21PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <[e-mail]>
> > 
> > Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
> > nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
> > time epoch).  This enables us to handle dates up to 2486, which solves
> > the y2038 problem.
> > 
> > Signed-off-by: Darrick J. Wong <[e-mail]>
> > Reviewed-by: Amir Goldstein <[e-mail]>
> 
> ....
> 
> > @@ -875,6 +888,25 @@ union xfs_timestamp {
> >   */
> >  #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
> >  
> > +/*
> > + * Number of seconds between the start of the bigtime timestamp range and the
> > + * start of the Unix epoch.
> > + */
> > +#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)
> 
> This is confusing. It's taken me 15 minutes so far to get my head
> around this because the reference frame for all these definitions is
> not clear. I though these had something to do with nanosecond
> timestamp limits because that's what BIGTIME records, but.....
> 
> The start of the epoch is a negative number based on the definition
> of the on-disk format for the minimum number of seconds that the
> "Unix" timestamp format can store?  Why is this not defined in
> nanoseconds given that is what is stored on disk?
> 
> XFS_INO_BIGTIME_EPOCH = (-XFS_INO_TIME_MIN)
> 			= (-((int64_t)S32_MIN))
> 			= (-((int64_t)-2^31))
> 			= 2^31?
> 
> So the bigtime epoch is considered to be 2^31 *seconds* into the
> range of the on-disk nanosecond timestamp? Huh?

They're the incore limits, not the ondisk limits.

Prior to bigtime, the ondisk timestamp epoch was the Unix epoch.  This
isn't the case anymore in bigtime (bigtime's epoch is Dec. 1901, aka the
minimum timestamp under the old scheme), so that misnamed
XFS_INO_BIGTIME_EPOCH value is the conversion factor between epochs.

(I'll come back to this at the bottom.)

> > +
> > +/*
> > + * Smallest possible timestamp with big timestamps, which is
> > + * Dec 13 20:45:52 UTC 1901.
> > + */
> > +#define XFS_INO_BIGTIME_MIN	(XFS_INO_TIME_MIN)
> 
> Same here. The reference here is "seconds before the Unix epoch",
> not the minimum valid on disk nanosecond value.
> 
> Oh, these are defining the post-disk-to-in-memory-format conversion
> limits? They have nothing to do with on-disk limits nor that on-disk
> format is stored in nanosecond? i.e. the reference frame for these
> limits is actually still the in-kernel Unix epoch definition?

Yes.  The on-disk limits are 0 and -1ULL with an epoch of Dec 1901, and
these XFS_INO_BIGTIME_{MIN,MAX} constants are the incore limits, which
of course have to be relative to the Unix epoch.

> > +/*
> > + * Largest possible timestamp with big timestamps, which is
> > + * Jul  2 20:20:25 UTC 2486.
> > + */
> > +#define XFS_INO_BIGTIME_MAX	((int64_t)((-1ULL / NSEC_PER_SEC) - \
> > +					   XFS_INO_BIGTIME_EPOCH))
> 
> Urk. This makes my head -hurt-.
> 
> It's converting the maximum on-disk format nanosecond count to a
> maximum second count then taking away the second count for the epoch
> and then casting it to a signed int because the in-memory format for
> seconds is signed? Oh, and the 64 bit division is via constants, so
> the compiler does it and doesn't need to use something like
> div_u64(), right?

Right.

> Mind you, I'm just guessing here that the "-1ULL" is the
> representation of the maximum on-disk nanosecond timestamp here,
> because that doesn't seem to be defined anywhere....

Yes.  Clearly I shouldn't have taken that shortcut.

> > diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
> > index cc1316a5fe0c..c59ddb56bb90 100644
> > --- a/fs/xfs/libxfs/xfs_inode_buf.c
> > +++ b/fs/xfs/libxfs/xfs_inode_buf.c
> > @@ -157,11 +157,25 @@ xfs_imap_to_bp(
> >  	return 0;
> >  }
> >  
> > +/* Convert an ondisk timestamp into the 64-bit safe incore format. */
> >  void
> >  xfs_inode_from_disk_timestamp(
> > +	struct xfs_dinode		*dip,
> >  	struct timespec64		*tv,
> >  	const union xfs_timestamp	*ts)
> >  {
> > +	if (dip->di_version >= 3 &&
> > +	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {
> 
> Helper. xfs_dinode_has_bigtime() was what I suggested previously.
> 
> > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > +		uint64_t		s;
> > +		uint32_t		n;
> > +
> > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > +		tv->tv_nsec = n;
> > +		return;
> > +	}
> > +
> >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> >  }
> 
> I still don't really like the way this turned out :(

I'll think about this further and hope that hch comes up with something
that's both functional and doesn't piss off smatch/sparse.  Note that I
also don't have any big endian machines anymore, so I don't really have
a good way to test this.  powerpc32 and sparc are verrrrry dead now.

> > @@ -220,9 +234,9 @@ xfs_inode_from_disk(
> >  	 * a time before epoch is converted to a time long after epoch
> >  	 * on 64 bit systems.
> >  	 */
> > -	xfs_inode_from_disk_timestamp(&inode->i_atime, &from->di_atime);
> > -	xfs_inode_from_disk_timestamp(&inode->i_mtime, &from->di_mtime);
> > -	xfs_inode_from_disk_timestamp(&inode->i_ctime, &from->di_ctime);
> > +	xfs_inode_from_disk_timestamp(from, &inode->i_atime, &from->di_atime);
> > +	xfs_inode_from_disk_timestamp(from, &inode->i_mtime, &from->di_mtime);
> > +	xfs_inode_from_disk_timestamp(from, &inode->i_ctime, &from->di_ctime);
> >  
> >  	to->di_size = be64_to_cpu(from->di_size);
> >  	to->di_nblocks = be64_to_cpu(from->di_nblocks);
> > @@ -235,9 +249,17 @@ xfs_inode_from_disk(
> >  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
> >  		inode_set_iversion_queried(inode,
> >  					   be64_to_cpu(from->di_changecount));
> > -		xfs_inode_from_disk_timestamp(&to->di_crtime, &from->di_crtime);
> > +		xfs_inode_from_disk_timestamp(from, &to->di_crtime,
> > +				&from->di_crtime);
> >  		to->di_flags2 = be64_to_cpu(from->di_flags2);
> >  		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
> > +		/*
> > +		 * Set the bigtime flag incore so that we automatically convert
> > +		 * this inode's ondisk timestamps to bigtime format the next
> > +		 * time we write the inode core to disk.
> > +		 */
> > +		if (xfs_sb_version_hasbigtime(&ip->i_mount->m_sb))
> > +			to->di_flags2 |= XFS_DIFLAG2_BIGTIME;
> >  	}
> 
> iAs I said before, you can't set this flag here - it needs to be
> done transactionally when the timestamp is next logged.

ARRGH.  I added code to xfs_trans_log_inode to do the conversion, and
then forgot to remove this.  I /swear/ I removed it, but there it still
is.

> 
> > @@ -259,9 +281,19 @@ xfs_inode_from_disk(
> >  
> >  void
> >  xfs_inode_to_disk_timestamp(
> > +	struct xfs_icdinode		*from,
> >  	union xfs_timestamp		*ts,
> >  	const struct timespec64		*tv)
> >  {
> > +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> 
> Shouldn't this also check the inode version number like the dinode
> decoder? Perhaps a helper like xfs_inode_has_bigtime(ip)?

Yes, will add.

> > +		uint64_t		t;
> > +
> > +		t = (uint64_t)(tv->tv_sec + XFS_INO_BIGTIME_EPOCH);
> 
> tv_sec can still overflow, right?
> 
> 		t = (uint64_t)tv->tv_sec + XFS_INO_BIGTIME_EPOCH;

It /shouldn't/ since we also set the superblock timestamp limits such
that the VFS will never pass us an over-large value, but I guess we
should be defensive here anyway.

> 
> > +		t *= NSEC_PER_SEC;
> > +		ts->t_bigtime = cpu_to_be64(t + tv->tv_nsec);
> > +		return;
> > +	}
> > +
> >  	ts->t_sec = cpu_to_be32(tv->tv_sec);
> >  	ts->t_nsec = cpu_to_be32(tv->tv_nsec);
> >  }
> > @@ -285,9 +317,9 @@ xfs_inode_to_disk(
> >  	to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
> >  
> >  	memset(to->di_pad, 0, sizeof(to->di_pad));
> > -	xfs_inode_to_disk_timestamp(&to->di_atime, &inode->i_atime);
> > -	xfs_inode_to_disk_timestamp(&to->di_mtime, &inode->i_mtime);
> > -	xfs_inode_to_disk_timestamp(&to->di_ctime, &inode->i_ctime);
> > +	xfs_inode_to_disk_timestamp(from, &to->di_atime, &inode->i_atime);
> > +	xfs_inode_to_disk_timestamp(from, &to->di_mtime, &inode->i_mtime);
> > +	xfs_inode_to_disk_timestamp(from, &to->di_ctime, &inode->i_ctime);
> >  	to->di_nlink = cpu_to_be32(inode->i_nlink);
> >  	to->di_gen = cpu_to_be32(inode->i_generation);
> >  	to->di_mode = cpu_to_be16(inode->i_mode);
> > @@ -306,7 +338,8 @@ xfs_inode_to_disk(
> >  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
> >  		to->di_version = 3;
> >  		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
> > -		xfs_inode_to_disk_timestamp(&to->di_crtime, &from->di_crtime);
> > +		xfs_inode_to_disk_timestamp(from, &to->di_crtime,
> > +				&from->di_crtime);
> >  		to->di_flags2 = cpu_to_be64(from->di_flags2);
> >  		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
> >  		to->di_ino = cpu_to_be64(ip->i_ino);
> > @@ -526,6 +559,11 @@ xfs_dinode_verify(
> >  	if (fa)
> >  		return fa;
> >  
> > +	/* bigtime iflag can only happen on bigtime filesystems */
> > +	if ((flags2 & (XFS_DIFLAG2_BIGTIME)) &&
> > +	    !xfs_sb_version_hasbigtime(&mp->m_sb))
> 
> 	if (xfs_dinode_has_bigtime(dip) &&
> 	    !xfs_sb_version_hasbigtime(&mp->m_sb))

<nod>

> 
> > +void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
> > +		union xfs_timestamp *ts, const struct timespec64 *tv);
> >  
> >  #endif	/* __XFS_INODE_BUF_H__ */
> > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > index 17c83d29998c..569721f7f9e5 100644
> > --- a/fs/xfs/libxfs/xfs_log_format.h
> > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > @@ -373,6 +373,9 @@ union xfs_ictimestamp {
> >  		int32_t		t_sec;		/* timestamp seconds */
> >  		int32_t		t_nsec;		/* timestamp nanoseconds */
> >  	};
> > +
> > +	/* Nanoseconds since the bigtime epoch. */
> > +	uint64_t		t_bigtime;
> >  };
> 
> Where are we using this again? Right now the timestamps are
> converted directly into the VFS inode timestamp fields so we can get
> rid of these incore timestamp fields. So shouldn't we be trying to
> get rid of this structure rather than adding more functionality to
> it?

We would have to enlarge xfs_log_dinode to log a full timespec64-like
entity.   I understand that it's annoying to convert a vfs timestamp
back into a u64 nanoseconds counter for the sake of the log, but doing
so will add complexity to the log for absolutely zero gain because
having 96 bits per timestamp in the log doesn't buy us anything.

> > @@ -131,6 +131,17 @@ xfs_trans_log_inode(
> >  			iversion_flags = XFS_ILOG_CORE;
> >  	}
> >  
> > +	/*
> > +	 * If we're updating the inode core or the timestamps and it's possible
> > +	 * to upgrade this inode to bigtime format, do so now.
> > +	 */
> > +	if ((flags & (XFS_ILOG_CORE | XFS_ILOG_TIMESTAMP)) &&
> > +	    xfs_sb_version_hasbigtime(&tp->t_mountp->m_sb) &&
> > +	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {
> 
> The latter two checks could be wrapped up into a helper named
> something obvious like xfs_inode_need_bigtime(ip)?

Ok.

> > +		ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> > +		flags |= XFS_ILOG_CORE;
> > +	}
> > +
> >  	/*
> >  	 * Record the specific change for fdatasync optimisation. This allows
> >  	 * fdatasync to skip log forces for inodes that are only timestamp
> > diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> > index 9f036053fdb7..6f00309de2d4 100644
> > --- a/fs/xfs/scrub/inode.c
> > +++ b/fs/xfs/scrub/inode.c
> > @@ -190,6 +190,11 @@ xchk_inode_flags2(
> >  	if ((flags2 & XFS_DIFLAG2_DAX) && (flags2 & XFS_DIFLAG2_REFLINK))
> >  		goto bad;
> >  
> > +	/* no bigtime iflag without the bigtime feature */
> > +	if (!xfs_sb_version_hasbigtime(&mp->m_sb) &&
> > +	    (flags2 & XFS_DIFLAG2_BIGTIME))
> 
> Can we get all these open coded checks wrapped up into a single
> helper?

Ok.

> > +		xchk_dinode_nsec(sc, ino, dip, &dip->di_crtime);
> >  		xchk_inode_flags2(sc, dip, ino, mode, flags, flags2);
> >  		xchk_inode_cowextsize(sc, dip, ino, mode, flags,
> >  				flags2);
> > diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> > index c06129cffba9..bbc309bc70c4 100644
> > --- a/fs/xfs/xfs_inode.c
> > +++ b/fs/xfs/xfs_inode.c
> > @@ -841,6 +841,8 @@ xfs_ialloc(
> >  	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
> >  		inode_set_iversion(inode, 1);
> >  		ip->i_d.di_flags2 = 0;
> > +		if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > +			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> 
> Rather than calculate the initial inode falgs on every allocation,
> shouldn't we just have the defaults pre-calculated at mount time?

Hm, yes.  Add that to the inode geometry structure?

> >  		ip->i_d.di_cowextsize = 0;
> >  		ip->i_d.di_crtime = tv;
> >  	}
> > @@ -2717,7 +2719,11 @@ xfs_ifree(
> >  
> >  	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
> >  	ip->i_d.di_flags = 0;
> > -	ip->i_d.di_flags2 = 0;
> > +	/*
> > +	 * Preserve the bigtime flag so that di_ctime accurately stores the
> > +	 * deletion time.
> > +	 */
> > +	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;
> 
> Oh, that's a nasty wart.

And here again?

> >  	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
> >  	ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
> > @@ -3503,6 +3509,13 @@ xfs_iflush(
> >  			__func__, ip->i_ino, ip->i_d.di_forkoff, ip);
> >  		goto flush_out;
> >  	}
> > +	if (xfs_sb_version_hasbigtime(&mp->m_sb) &&
> > +	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {
> > +		xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
> > +			"%s: bad inode %Lu, bigtime unset, ptr "PTR_FMT,
> > +			__func__, ip->i_ino, ip);
> > +		goto flush_out;
> > +	}
> 
> Why is this a fatal corruption error? if the bigtime flag is not
> set, we can still store and read the timestamp if it is within
> the unix epoch range...

Ugh, it's not.  It's a leftover artifact from when I would just set the
flag incore unconditionally, all of which should have been removed when
I added the modifications to xfs_trans_log_inode, but clearly I forgot.

> >  
> >  	/*
> >  	 * Inode item log recovery for v2 inodes are dependent on the
> > diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> > index cec6d4d82bc4..c38af3eea48f 100644
> > --- a/fs/xfs/xfs_inode_item.c
> > +++ b/fs/xfs/xfs_inode_item.c
> > @@ -298,9 +298,16 @@ xfs_inode_item_format_attr_fork(
> >  /* Write a log_dinode timestamp into an ondisk inode timestamp. */
> >  static inline void
> >  xfs_log_dinode_to_disk_ts(
> > +	struct xfs_log_dinode		*from,
> >  	union xfs_timestamp		*ts,
> >  	const union xfs_ictimestamp	*its)
> >  {
> > +	if (from->di_version >= 3 &&
> > +	    (from->di_flags2 & XFS_DIFLAG2_BIGTIME)) {
> 
> helper.
> 
> > +		ts->t_bigtime = cpu_to_be64(its->t_bigtime);
> > +		return;
> > +	}
> > +
> >  	ts->t_sec = cpu_to_be32(its->t_sec);
> >  	ts->t_nsec = cpu_to_be32(its->t_nsec);
> >  }
> > @@ -322,9 +329,9 @@ xfs_log_dinode_to_disk(
> >  	to->di_projid_hi = cpu_to_be16(from->di_projid_hi);
> >  	memcpy(to->di_pad, from->di_pad, sizeof(to->di_pad));
> >  
> > -	xfs_log_dinode_to_disk_ts(&to->di_atime, &from->di_atime);
> > -	xfs_log_dinode_to_disk_ts(&to->di_mtime, &from->di_mtime);
> > -	xfs_log_dinode_to_disk_ts(&to->di_ctime, &from->di_ctime);
> > +	xfs_log_dinode_to_disk_ts(from, &to->di_atime, &from->di_atime);
> > +	xfs_log_dinode_to_disk_ts(from, &to->di_mtime, &from->di_mtime);
> > +	xfs_log_dinode_to_disk_ts(from, &to->di_ctime, &from->di_ctime);
> >  
> >  	to->di_size = cpu_to_be64(from->di_size);
> >  	to->di_nblocks = cpu_to_be64(from->di_nblocks);
> > @@ -340,7 +347,7 @@ xfs_log_dinode_to_disk(
> >  
> >  	if (from->di_version == 3) {
> >  		to->di_changecount = cpu_to_be64(from->di_changecount);
> > -		xfs_log_dinode_to_disk_ts(&to->di_crtime, &from->di_crtime);
> > +		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);
> >  		to->di_flags2 = cpu_to_be64(from->di_flags2);
> >  		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
> >  		to->di_ino = cpu_to_be64(from->di_ino);
> > @@ -356,9 +363,19 @@ xfs_log_dinode_to_disk(
> >  /* Write an incore inode timestamp into a log_dinode timestamp. */
> >  static inline void
> >  xfs_inode_to_log_dinode_ts(
> > +	struct xfs_icdinode		*from,
> >  	union xfs_ictimestamp		*its,
> >  	const struct timespec64		*ts)
> >  {
> > +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> > +		uint64_t		t;
> > +
> > +		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
> > +		t *= NSEC_PER_SEC;
> > +		its->t_bigtime = t + ts->tv_nsec;
> 
> helper,
> 
> > +		return;
> > +	}
> > +
> >  	its->t_sec = ts->tv_sec;
> >  	its->t_nsec = ts->tv_nsec;
> >  }
> > @@ -381,9 +398,9 @@ xfs_inode_to_log_dinode(
> >  
> >  	memset(to->di_pad, 0, sizeof(to->di_pad));
> >  	memset(to->di_pad3, 0, sizeof(to->di_pad3));
> > -	xfs_inode_to_log_dinode_ts(&to->di_atime, &inode->i_atime);
> > -	xfs_inode_to_log_dinode_ts(&to->di_mtime, &inode->i_mtime);
> > -	xfs_inode_to_log_dinode_ts(&to->di_ctime, &inode->i_ctime);
> > +	xfs_inode_to_log_dinode_ts(from, &to->di_atime, &inode->i_atime);
> > +	xfs_inode_to_log_dinode_ts(from, &to->di_mtime, &inode->i_mtime);
> > +	xfs_inode_to_log_dinode_ts(from, &to->di_ctime, &inode->i_ctime);
> >  	to->di_nlink = inode->i_nlink;
> >  	to->di_gen = inode->i_generation;
> >  	to->di_mode = inode->i_mode;
> > @@ -405,7 +422,7 @@ xfs_inode_to_log_dinode(
> >  	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
> >  		to->di_version = 3;
> >  		to->di_changecount = inode_peek_iversion(inode);
> > -		xfs_inode_to_log_dinode_ts(&to->di_crtime, &from->di_crtime);
> > +		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);
> >  		to->di_flags2 = from->di_flags2;
> >  		to->di_cowextsize = from->di_cowextsize;
> >  		to->di_ino = ip->i_ino;
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 6f22a66777cd..13396c3665d1 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1190,7 +1190,8 @@ xfs_flags2diflags2(
> >  	unsigned int		xflags)
> >  {
> >  	uint64_t		di_flags2 =
> > -		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
> > +		(ip->i_d.di_flags2 & (XFS_DIFLAG2_REFLINK |
> > +				      XFS_DIFLAG2_BIGTIME));
> >  
> >  	if (xflags & FS_XFLAG_DAX)
> >  		di_flags2 |= XFS_DIFLAG2_DAX;
> > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > index 7158a8de719f..3e0c677cff15 100644
> > --- a/fs/xfs/xfs_ondisk.h
> > +++ b/fs/xfs/xfs_ondisk.h
> > @@ -25,6 +25,9 @@ xfs_check_limits(void)
> >  	/* make sure timestamp limits are correct */
> >  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
> >  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
> > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);
> 
> That still just doesn't look right to me :/
> 
> This implies that the epoch is 2^32 seconds after then minimum
> supported time (2038), when in fact it is only 2^31 seconds after the
> minimum supported timestamp (1970). :/

Ok, so XFS_INO_UNIX_BIGTIME_MIN is -2147483648, to signify that the
smallest bigtime timestamp is (still) December 1901.

That thing currently known as XFS_INO_BIGTIME_EPOCH should probably get
renamed to something less confusing, like...

/*
 * Since the bigtime epoch is Dec. 1901, add this number of seconds to
 * an ondisk bigtime timestamp to convert it to the Unix epoch.
 */
#define XFS_BIGTIME_TO_UNIX		(-XFS_INO_UNIX_BIGTIME_MIN)

/*
 * Subtract this many seconds from a Unix epoch timestamp to get the
 * ondisk bigtime timestamp.
 */
#define XFS_UNIX_TO_BIGTIME		(-XFS_BIGTIME_TO_UNIX)

Is that clearer?

> > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MAX,			16299260425LL);
> 
> Hmmm. I got 16299260424 when I just ran this through a simple calc.
> Mind you, no calculator app I found could handle unsigned 64 bit
> values natively (signed 64 bit is good enough for everyone!) so
> maybe I got an off-by one here...

-1ULL = 18,446,744,073,709,551,615
-1ULL / NSEC_PER_SEC = 18,446,744,073
(-1ULL / NSEC_PER_SEC) - XFS_INO_BIGTIME_EPOCH = 16,299,260,425

Assuming you accept XFS_INO_BIGTIME_EPOCH being 2^31.

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]

Dave Chinner Aug. 24, 2020, 6:15 a.m. UTC | #5

On Sun, Aug 23, 2020 at 08:13:54PM -0700, Darrick J. Wong wrote:
> On Mon, Aug 25, 2020 at 11:25:27AM +1000, Dave Chinner wrote:
> > On Thu, Aug 20, 2020 at 07:12:21PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <[e-mail]>
> > > 
> > > Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
> > > nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
> > > time epoch).  This enables us to handle dates up to 2486, which solves
> > > the y2038 problem.
> > > 
> > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > Reviewed-by: Amir Goldstein <[e-mail]>
> > 
> > ....
> > 
> > > @@ -875,6 +888,25 @@ union xfs_timestamp {
> > >   */
> > >  #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
> > >  
> > > +/*
> > > + * Number of seconds between the start of the bigtime timestamp range and the
> > > + * start of the Unix epoch.
> > > + */
> > > +#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)
> > 
> > This is confusing. It's taken me 15 minutes so far to get my head
> > around this because the reference frame for all these definitions is
> > not clear. I though these had something to do with nanosecond
> > timestamp limits because that's what BIGTIME records, but.....
> > 
> > The start of the epoch is a negative number based on the definition
> > of the on-disk format for the minimum number of seconds that the
> > "Unix" timestamp format can store?  Why is this not defined in
> > nanoseconds given that is what is stored on disk?
> > 
> > XFS_INO_BIGTIME_EPOCH = (-XFS_INO_TIME_MIN)
> > 			= (-((int64_t)S32_MIN))
> > 			= (-((int64_t)-2^31))
> > 			= 2^31?
> > 
> > So the bigtime epoch is considered to be 2^31 *seconds* into the
> > range of the on-disk nanosecond timestamp? Huh?
> 
> They're the incore limits, not the ondisk limits.
> 
> Prior to bigtime, the ondisk timestamp epoch was the Unix epoch.  This
> isn't the case anymore in bigtime (bigtime's epoch is Dec. 1901, aka the
> minimum timestamp under the old scheme), so that misnamed
> XFS_INO_BIGTIME_EPOCH value is the conversion factor between epochs.
> 
> (I'll come back to this at the bottom.)

Ok, I'll come back to that at the bottom :)

> > > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > > +		uint64_t		s;
> > > +		uint32_t		n;
> > > +
> > > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > > +		tv->tv_nsec = n;
> > > +		return;
> > > +	}
> > > +
> > >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> > >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> > >  }
> > 
> > I still don't really like the way this turned out :(
> 
> I'll think about this further and hope that hch comes up with something
> that's both functional and doesn't piss off smatch/sparse.  Note that I
> also don't have any big endian machines anymore, so I don't really have
> a good way to test this.  powerpc32 and sparc are verrrrry dead now.

I'm not sure that anyone has current BE machines to test on....

> > > +void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
> > > +		union xfs_timestamp *ts, const struct timespec64 *tv);
> > >  
> > >  #endif	/* __XFS_INODE_BUF_H__ */
> > > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > > index 17c83d29998c..569721f7f9e5 100644
> > > --- a/fs/xfs/libxfs/xfs_log_format.h
> > > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > > @@ -373,6 +373,9 @@ union xfs_ictimestamp {
> > >  		int32_t		t_sec;		/* timestamp seconds */
> > >  		int32_t		t_nsec;		/* timestamp nanoseconds */
> > >  	};
> > > +
> > > +	/* Nanoseconds since the bigtime epoch. */
> > > +	uint64_t		t_bigtime;
> > >  };
> > 
> > Where are we using this again? Right now the timestamps are
> > converted directly into the VFS inode timestamp fields so we can get
> > rid of these incore timestamp fields. So shouldn't we be trying to
> > get rid of this structure rather than adding more functionality to
> > it?
> 
> We would have to enlarge xfs_log_dinode to log a full timespec64-like
> entity.   I understand that it's annoying to convert a vfs timestamp
> back into a u64 nanoseconds counter for the sake of the log, but doing
> so will add complexity to the log for absolutely zero gain because
> having 96 bits per timestamp in the log doesn't buy us anything.

Sure, I understand that we only need to log a 64bit value, but we
don't actually need a structure for that as the log is in native
endian format. Hence it can just be a 64 bit field that we mask and
shift for !bigtime inodes...

Note that we have to be real careful about dynamic conversion,
especially in recovery, as the inode read from disk might be in
small time format, but logged and recovered in bigtime format. I
didn't actually check the recovery code does that correctly, because
it only just occurred to me that the logged timestamp format may not
match the inode flags read from disk during recovery...

> > > --- a/fs/xfs/xfs_inode.c
> > > +++ b/fs/xfs/xfs_inode.c
> > > @@ -841,6 +841,8 @@ xfs_ialloc(
> > >  	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
> > >  		inode_set_iversion(inode, 1);
> > >  		ip->i_d.di_flags2 = 0;
> > > +		if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > > +			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> > 
> > Rather than calculate the initial inode falgs on every allocation,
> > shouldn't we just have the defaults pre-calculated at mount time?
> 
> Hm, yes.  Add that to the inode geometry structure?

Sounds like a reasonable place to me.

> > >  		ip->i_d.di_cowextsize = 0;
> > >  		ip->i_d.di_crtime = tv;
> > >  	}
> > > @@ -2717,7 +2719,11 @@ xfs_ifree(
> > >  
> > >  	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
> > >  	ip->i_d.di_flags = 0;
> > > -	ip->i_d.di_flags2 = 0;
> > > +	/*
> > > +	 * Preserve the bigtime flag so that di_ctime accurately stores the
> > > +	 * deletion time.
> > > +	 */
> > > +	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;
> > 
> > Oh, that's a nasty wart.
> 
> And here again?

*nod*. Good idea - we will have logged the inode core and converted
it in-core to bigtime by this point...

> > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > > index 7158a8de719f..3e0c677cff15 100644
> > > --- a/fs/xfs/xfs_ondisk.h
> > > +++ b/fs/xfs/xfs_ondisk.h
> > > @@ -25,6 +25,9 @@ xfs_check_limits(void)
> > >  	/* make sure timestamp limits are correct */
> > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
> > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
> > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);
> > 
> > That still just doesn't look right to me :/
> > 
> > This implies that the epoch is 2^32 seconds after then minimum
> > supported time (2038), when in fact it is only 2^31 seconds after the
> > minimum supported timestamp (1970). :/
> 
> Ok, so XFS_INO_UNIX_BIGTIME_MIN is -2147483648, to signify that the
> smallest bigtime timestamp is (still) December 1901.

Let's drop the "ino" from the name - it's unnecessary, I think.

> That thing currently known as XFS_INO_BIGTIME_EPOCH should probably get
> renamed to something less confusing, like...
>
> /*
>  * Since the bigtime epoch is Dec. 1901, add this number of seconds to
>  * an ondisk bigtime timestamp to convert it to the Unix epoch.
>  */
> #define XFS_BIGTIME_TO_UNIX		(-XFS_INO_UNIX_BIGTIME_MIN)
> 
> /*
>  * Subtract this many seconds from a Unix epoch timestamp to get the
>  * ondisk bigtime timestamp.
>  */
> #define XFS_UNIX_TO_BIGTIME		(-XFS_BIGTIME_TO_UNIX)
> 
> Is that clearer?

Hmmm. Definitely better, but how about:

/*
 * Bigtime epoch is set exactly to the minimum time value that a
 * traditional 32 bit timestamp can represent when using the Unix
 * epoch as a reference. Hence the Unix epoch is at a fixed offset
 * into the supported bigtime timestamp range.
 *
 * The bigtime epoch also matches the minimum value an on-disk 32
 * bit XFS timestamp can represent so we will not lose any fidelity
 * in converting to/from unix and bigtime timestamps.
 */
#define XFS_BIGTIME_EPOCH_OFFSET	(XFS_INO_TIME_MIN)

And then two static inline helpers follow immediately -
xfs_bigtime_to_unix() and xfs_bigtime_from_unix() can do the
conversion between the two formats and the XFS_BIGTIME_EPOCH_OFFSET
variable never gets seen anywhere else in the code. To set the max
timestamp value the superblock holds for the filesystem, just
calculate it directly via a call to xfs_bigtime_to_unix(-1ULL, ...)

> > Hmmm. I got 16299260424 when I just ran this through a simple calc.
> > Mind you, no calculator app I found could handle unsigned 64 bit
> > values natively (signed 64 bit is good enough for everyone!) so
> > maybe I got an off-by one here...
> 
> -1ULL = 18,446,744,073,709,551,615
> -1ULL / NSEC_PER_SEC = 18,446,744,073
> (-1ULL / NSEC_PER_SEC) - XFS_INO_BIGTIME_EPOCH = 16,299,260,425

Yup, I got an off by one thanks to integer rounding on the
division. I should have just done it long hand like that...

Cheers,

Dave.

Darrick J. Wong Aug. 24, 2020, 4:24 p.m. UTC | #6

On Mon, Aug 24, 2020 at 04:15:31PM +1000, Dave Chinner wrote:
> On Sun, Aug 23, 2020 at 08:13:54PM -0700, Darrick J. Wong wrote:
> > On Mon, Aug 25, 2020 at 11:25:27AM +1000, Dave Chinner wrote:
> > > On Thu, Aug 20, 2020 at 07:12:21PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <[e-mail]>
> > > > 
> > > > Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
> > > > nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
> > > > time epoch).  This enables us to handle dates up to 2486, which solves
> > > > the y2038 problem.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > Reviewed-by: Amir Goldstein <[e-mail]>
> > > 
> > > ....
> > > 
> > > > @@ -875,6 +888,25 @@ union xfs_timestamp {
> > > >   */
> > > >  #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
> > > >  
> > > > +/*
> > > > + * Number of seconds between the start of the bigtime timestamp range and the
> > > > + * start of the Unix epoch.
> > > > + */
> > > > +#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)
> > > 
> > > This is confusing. It's taken me 15 minutes so far to get my head
> > > around this because the reference frame for all these definitions is
> > > not clear. I though these had something to do with nanosecond
> > > timestamp limits because that's what BIGTIME records, but.....
> > > 
> > > The start of the epoch is a negative number based on the definition
> > > of the on-disk format for the minimum number of seconds that the
> > > "Unix" timestamp format can store?  Why is this not defined in
> > > nanoseconds given that is what is stored on disk?
> > > 
> > > XFS_INO_BIGTIME_EPOCH = (-XFS_INO_TIME_MIN)
> > > 			= (-((int64_t)S32_MIN))
> > > 			= (-((int64_t)-2^31))
> > > 			= 2^31?
> > > 
> > > So the bigtime epoch is considered to be 2^31 *seconds* into the
> > > range of the on-disk nanosecond timestamp? Huh?
> > 
> > They're the incore limits, not the ondisk limits.
> > 
> > Prior to bigtime, the ondisk timestamp epoch was the Unix epoch.  This
> > isn't the case anymore in bigtime (bigtime's epoch is Dec. 1901, aka the
> > minimum timestamp under the old scheme), so that misnamed
> > XFS_INO_BIGTIME_EPOCH value is the conversion factor between epochs.
> > 
> > (I'll come back to this at the bottom.)
> 
> Ok, I'll come back to that at the bottom :)
> 
> > > > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > > > +		uint64_t		s;
> > > > +		uint32_t		n;
> > > > +
> > > > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > > > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > > > +		tv->tv_nsec = n;
> > > > +		return;
> > > > +	}
> > > > +
> > > >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> > > >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> > > >  }
> > > 
> > > I still don't really like the way this turned out :(
> > 
> > I'll think about this further and hope that hch comes up with something
> > that's both functional and doesn't piss off smatch/sparse.  Note that I
> > also don't have any big endian machines anymore, so I don't really have
> > a good way to test this.  powerpc32 and sparc are verrrrry dead now.
> 
> I'm not sure that anyone has current BE machines to test on....

...which makes me all the more nervous about replacing the timestamp
union with open-coded bit shifting.  We know the existing code does the
conversions properly with the separate sec/nsec fields since that code
has been around for a while.  We can use BUILD_BUG_ON macros to ensure
that inside the union, the bigtime nanoseconds counter is overlayed
/exactly/ on top of the old structure.  There's a feature flag within
the ondisk structure, which means that reasoning about this code is no
more difficult than any other tagged union.

Flag == 0?  Use the same old code from before.
Flag == 1?  Use the new code.

I was about to say that I'll experiment with this as a new patch at the
end of the series, but I guess converting xfs_timestamp back to a
typedef is more churn and belongs at the start of the series...

> > > > +void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
> > > > +		union xfs_timestamp *ts, const struct timespec64 *tv);
> > > >  
> > > >  #endif	/* __XFS_INODE_BUF_H__ */
> > > > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > > > index 17c83d29998c..569721f7f9e5 100644
> > > > --- a/fs/xfs/libxfs/xfs_log_format.h
> > > > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > > > @@ -373,6 +373,9 @@ union xfs_ictimestamp {
> > > >  		int32_t		t_sec;		/* timestamp seconds */
> > > >  		int32_t		t_nsec;		/* timestamp nanoseconds */
> > > >  	};
> > > > +
> > > > +	/* Nanoseconds since the bigtime epoch. */
> > > > +	uint64_t		t_bigtime;
> > > >  };
> > > 
> > > Where are we using this again? Right now the timestamps are
> > > converted directly into the VFS inode timestamp fields so we can get
> > > rid of these incore timestamp fields. So shouldn't we be trying to
> > > get rid of this structure rather than adding more functionality to
> > > it?
> > 
> > We would have to enlarge xfs_log_dinode to log a full timespec64-like
> > entity.   I understand that it's annoying to convert a vfs timestamp
> > back into a u64 nanoseconds counter for the sake of the log, but doing
> > so will add complexity to the log for absolutely zero gain because
> > having 96 bits per timestamp in the log doesn't buy us anything.
> 
> Sure, I understand that we only need to log a 64bit value, but we
> don't actually need a structure for that as the log is in native
> endian format. Hence it can just be a 64 bit field that we mask and
> shift for !bigtime inodes...
> 
> Note that we have to be real careful about dynamic conversion,
> especially in recovery, as the inode read from disk might be in
> small time format, but logged and recovered in bigtime format. I
> didn't actually check the recovery code does that correctly, because
> it only just occurred to me that the logged timestamp format may not
> match the inode flags read from disk during recovery...

Oh my, you're right, that xfs_log_dinode_to_disk_timestamp needs to be
more careful to convert whatever we logged into something that is
agnostic to disk format, and then convert it to whatever is the
xfs_dinode format.

I'll throw that on the fixme pile too.

> > > > --- a/fs/xfs/xfs_inode.c
> > > > +++ b/fs/xfs/xfs_inode.c
> > > > @@ -841,6 +841,8 @@ xfs_ialloc(
> > > >  	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
> > > >  		inode_set_iversion(inode, 1);
> > > >  		ip->i_d.di_flags2 = 0;
> > > > +		if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > > > +			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> > > 
> > > Rather than calculate the initial inode falgs on every allocation,
> > > shouldn't we just have the defaults pre-calculated at mount time?
> > 
> > Hm, yes.  Add that to the inode geometry structure?
> 
> Sounds like a reasonable place to me.
> 
> > > >  		ip->i_d.di_cowextsize = 0;
> > > >  		ip->i_d.di_crtime = tv;
> > > >  	}
> > > > @@ -2717,7 +2719,11 @@ xfs_ifree(
> > > >  
> > > >  	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
> > > >  	ip->i_d.di_flags = 0;
> > > > -	ip->i_d.di_flags2 = 0;
> > > > +	/*
> > > > +	 * Preserve the bigtime flag so that di_ctime accurately stores the
> > > > +	 * deletion time.
> > > > +	 */
> > > > +	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;
> > > 
> > > Oh, that's a nasty wart.
> > 
> > And here again?
> 
> *nod*. Good idea - we will have logged the inode core and converted
> it in-core to bigtime by this point...
> 
> > > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > > > index 7158a8de719f..3e0c677cff15 100644
> > > > --- a/fs/xfs/xfs_ondisk.h
> > > > +++ b/fs/xfs/xfs_ondisk.h
> > > > @@ -25,6 +25,9 @@ xfs_check_limits(void)
> > > >  	/* make sure timestamp limits are correct */
> > > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
> > > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> > > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
> > > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);
> > > 
> > > That still just doesn't look right to me :/
> > > 
> > > This implies that the epoch is 2^32 seconds after then minimum
> > > supported time (2038), when in fact it is only 2^31 seconds after the
> > > minimum supported timestamp (1970). :/
> > 
> > Ok, so XFS_INO_UNIX_BIGTIME_MIN is -2147483648, to signify that the
> > smallest bigtime timestamp is (still) December 1901.
> 
> Let's drop the "ino" from the name - it's unnecessary, I think.

Ok.

> > That thing currently known as XFS_INO_BIGTIME_EPOCH should probably get
> > renamed to something less confusing, like...
> >
> > /*
> >  * Since the bigtime epoch is Dec. 1901, add this number of seconds to
> >  * an ondisk bigtime timestamp to convert it to the Unix epoch.
> >  */
> > #define XFS_BIGTIME_TO_UNIX		(-XFS_INO_UNIX_BIGTIME_MIN)
> > 
> > /*
> >  * Subtract this many seconds from a Unix epoch timestamp to get the
> >  * ondisk bigtime timestamp.
> >  */
> > #define XFS_UNIX_TO_BIGTIME		(-XFS_BIGTIME_TO_UNIX)
> > 
> > Is that clearer?
> 
> Hmmm. Definitely better, but how about:
> 
> /*
>  * Bigtime epoch is set exactly to the minimum time value that a
>  * traditional 32 bit timestamp can represent when using the Unix
>  * epoch as a reference. Hence the Unix epoch is at a fixed offset
>  * into the supported bigtime timestamp range.
>  *
>  * The bigtime epoch also matches the minimum value an on-disk 32
>  * bit XFS timestamp can represent so we will not lose any fidelity
>  * in converting to/from unix and bigtime timestamps.
>  */
> #define XFS_BIGTIME_EPOCH_OFFSET	(XFS_INO_TIME_MIN)
> 
> And then two static inline helpers follow immediately -
> xfs_bigtime_to_unix() and xfs_bigtime_from_unix() can do the
> conversion between the two formats and the XFS_BIGTIME_EPOCH_OFFSET
> variable never gets seen anywhere else in the code. To set the max
> timestamp value the superblock holds for the filesystem, just
> calculate it directly via a call to xfs_bigtime_to_unix(-1ULL, ...)

<nod>

--D

> > > Hmmm. I got 16299260424 when I just ran this through a simple calc.
> > > Mind you, no calculator app I found could handle unsigned 64 bit
> > > values natively (signed 64 bit is good enough for everyone!) so
> > > maybe I got an off-by one here...
> > 
> > -1ULL = 18,446,744,073,709,551,615
> > -1ULL / NSEC_PER_SEC = 18,446,744,073
> > (-1ULL / NSEC_PER_SEC) - XFS_INO_BIGTIME_EPOCH = 16,299,260,425
> 
> Yup, I got an off by one thanks to integer rounding on the
> division. I should have just done it long hand like that...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]

Darrick J. Wong Aug. 24, 2020, 9:13 p.m. UTC | #7

On Mon, Aug 24, 2020 at 09:24:20AM -0700, Darrick J. Wong wrote:
> On Mon, Aug 24, 2020 at 04:15:31PM +1000, Dave Chinner wrote:
> > On Sun, Aug 23, 2020 at 08:13:54PM -0700, Darrick J. Wong wrote:
> > > On Mon, Aug 25, 2020 at 11:25:27AM +1000, Dave Chinner wrote:
> > > > On Thu, Aug 20, 2020 at 07:12:21PM -0700, Darrick J. Wong wrote:
> > > > > From: Darrick J. Wong <[e-mail]>
> > > > > 
> > > > > Redesign the ondisk timestamps to be a simple unsigned 64-bit counter of
> > > > > nanoseconds since 14 Dec 1901 (i.e. the minimum time in the 32-bit unix
> > > > > time epoch).  This enables us to handle dates up to 2486, which solves
> > > > > the y2038 problem.
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <[e-mail]>
> > > > > Reviewed-by: Amir Goldstein <[e-mail]>
> > > > 
> > > > ....
> > > > 
> > > > > @@ -875,6 +888,25 @@ union xfs_timestamp {
> > > > >   */
> > > > >  #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
> > > > >  
> > > > > +/*
> > > > > + * Number of seconds between the start of the bigtime timestamp range and the
> > > > > + * start of the Unix epoch.
> > > > > + */
> > > > > +#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)
> > > > 
> > > > This is confusing. It's taken me 15 minutes so far to get my head
> > > > around this because the reference frame for all these definitions is
> > > > not clear. I though these had something to do with nanosecond
> > > > timestamp limits because that's what BIGTIME records, but.....
> > > > 
> > > > The start of the epoch is a negative number based on the definition
> > > > of the on-disk format for the minimum number of seconds that the
> > > > "Unix" timestamp format can store?  Why is this not defined in
> > > > nanoseconds given that is what is stored on disk?
> > > > 
> > > > XFS_INO_BIGTIME_EPOCH = (-XFS_INO_TIME_MIN)
> > > > 			= (-((int64_t)S32_MIN))
> > > > 			= (-((int64_t)-2^31))
> > > > 			= 2^31?
> > > > 
> > > > So the bigtime epoch is considered to be 2^31 *seconds* into the
> > > > range of the on-disk nanosecond timestamp? Huh?
> > > 
> > > They're the incore limits, not the ondisk limits.
> > > 
> > > Prior to bigtime, the ondisk timestamp epoch was the Unix epoch.  This
> > > isn't the case anymore in bigtime (bigtime's epoch is Dec. 1901, aka the
> > > minimum timestamp under the old scheme), so that misnamed
> > > XFS_INO_BIGTIME_EPOCH value is the conversion factor between epochs.
> > > 
> > > (I'll come back to this at the bottom.)
> > 
> > Ok, I'll come back to that at the bottom :)
> > 
> > > > > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > > > > +		uint64_t		s;
> > > > > +		uint32_t		n;
> > > > > +
> > > > > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > > > > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > > > > +		tv->tv_nsec = n;
> > > > > +		return;
> > > > > +	}
> > > > > +
> > > > >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> > > > >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> > > > >  }
> > > > 
> > > > I still don't really like the way this turned out :(
> > > 
> > > I'll think about this further and hope that hch comes up with something
> > > that's both functional and doesn't piss off smatch/sparse.  Note that I
> > > also don't have any big endian machines anymore, so I don't really have
> > > a good way to test this.  powerpc32 and sparc are verrrrry dead now.
> > 
> > I'm not sure that anyone has current BE machines to test on....
> 
> ...which makes me all the more nervous about replacing the timestamp
> union with open-coded bit shifting.  We know the existing code does the
> conversions properly with the separate sec/nsec fields since that code
> has been around for a while.  We can use BUILD_BUG_ON macros to ensure
> that inside the union, the bigtime nanoseconds counter is overlayed
> /exactly/ on top of the old structure.  There's a feature flag within
> the ondisk structure, which means that reasoning about this code is no
> more difficult than any other tagged union.
> 
> Flag == 0?  Use the same old code from before.
> Flag == 1?  Use the new code.
> 
> I was about to say that I'll experiment with this as a new patch at the
> end of the series, but I guess converting xfs_timestamp back to a
> typedef is more churn and belongs at the start of the series...
> 
> > > > > +void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
> > > > > +		union xfs_timestamp *ts, const struct timespec64 *tv);
> > > > >  
> > > > >  #endif	/* __XFS_INODE_BUF_H__ */
> > > > > diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
> > > > > index 17c83d29998c..569721f7f9e5 100644
> > > > > --- a/fs/xfs/libxfs/xfs_log_format.h
> > > > > +++ b/fs/xfs/libxfs/xfs_log_format.h
> > > > > @@ -373,6 +373,9 @@ union xfs_ictimestamp {
> > > > >  		int32_t		t_sec;		/* timestamp seconds */
> > > > >  		int32_t		t_nsec;		/* timestamp nanoseconds */
> > > > >  	};
> > > > > +
> > > > > +	/* Nanoseconds since the bigtime epoch. */
> > > > > +	uint64_t		t_bigtime;
> > > > >  };
> > > > 
> > > > Where are we using this again? Right now the timestamps are
> > > > converted directly into the VFS inode timestamp fields so we can get
> > > > rid of these incore timestamp fields. So shouldn't we be trying to
> > > > get rid of this structure rather than adding more functionality to
> > > > it?
> > > 
> > > We would have to enlarge xfs_log_dinode to log a full timespec64-like
> > > entity.   I understand that it's annoying to convert a vfs timestamp
> > > back into a u64 nanoseconds counter for the sake of the log, but doing
> > > so will add complexity to the log for absolutely zero gain because
> > > having 96 bits per timestamp in the log doesn't buy us anything.
> > 
> > Sure, I understand that we only need to log a 64bit value, but we
> > don't actually need a structure for that as the log is in native
> > endian format. Hence it can just be a 64 bit field that we mask and
> > shift for !bigtime inodes...
> > 
> > Note that we have to be real careful about dynamic conversion,
> > especially in recovery, as the inode read from disk might be in
> > small time format, but logged and recovered in bigtime format. I
> > didn't actually check the recovery code does that correctly, because
> > it only just occurred to me that the logged timestamp format may not
> > match the inode flags read from disk during recovery...
> 
> Oh my, you're right, that xfs_log_dinode_to_disk_timestamp needs to be
> more careful to convert whatever we logged into something that is
> agnostic to disk format, and then convert it to whatever is the
> xfs_dinode format.

On second thought, I think the inode logging code handles the timestamp
format correctly as it is now written because we always log the inode
core even if we're doing an XFS_ILOG_TIMESTAMP update, right?  Which
means that we always log the timestamps along with flags2 in
xfs_log_dinode, and therefore there's no chance for inconsistency.

--D

> I'll throw that on the fixme pile too.
> 
> > > > > --- a/fs/xfs/xfs_inode.c
> > > > > +++ b/fs/xfs/xfs_inode.c
> > > > > @@ -841,6 +841,8 @@ xfs_ialloc(
> > > > >  	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
> > > > >  		inode_set_iversion(inode, 1);
> > > > >  		ip->i_d.di_flags2 = 0;
> > > > > +		if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > > > > +			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
> > > > 
> > > > Rather than calculate the initial inode falgs on every allocation,
> > > > shouldn't we just have the defaults pre-calculated at mount time?
> > > 
> > > Hm, yes.  Add that to the inode geometry structure?
> > 
> > Sounds like a reasonable place to me.
> > 
> > > > >  		ip->i_d.di_cowextsize = 0;
> > > > >  		ip->i_d.di_crtime = tv;
> > > > >  	}
> > > > > @@ -2717,7 +2719,11 @@ xfs_ifree(
> > > > >  
> > > > >  	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
> > > > >  	ip->i_d.di_flags = 0;
> > > > > -	ip->i_d.di_flags2 = 0;
> > > > > +	/*
> > > > > +	 * Preserve the bigtime flag so that di_ctime accurately stores the
> > > > > +	 * deletion time.
> > > > > +	 */
> > > > > +	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;
> > > > 
> > > > Oh, that's a nasty wart.
> > > 
> > > And here again?
> > 
> > *nod*. Good idea - we will have logged the inode core and converted
> > it in-core to bigtime by this point...
> > 
> > > > > diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
> > > > > index 7158a8de719f..3e0c677cff15 100644
> > > > > --- a/fs/xfs/xfs_ondisk.h
> > > > > +++ b/fs/xfs/xfs_ondisk.h
> > > > > @@ -25,6 +25,9 @@ xfs_check_limits(void)
> > > > >  	/* make sure timestamp limits are correct */
> > > > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
> > > > >  	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
> > > > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
> > > > > +	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);
> > > > 
> > > > That still just doesn't look right to me :/
> > > > 
> > > > This implies that the epoch is 2^32 seconds after then minimum
> > > > supported time (2038), when in fact it is only 2^31 seconds after the
> > > > minimum supported timestamp (1970). :/
> > > 
> > > Ok, so XFS_INO_UNIX_BIGTIME_MIN is -2147483648, to signify that the
> > > smallest bigtime timestamp is (still) December 1901.
> > 
> > Let's drop the "ino" from the name - it's unnecessary, I think.
> 
> Ok.
> 
> > > That thing currently known as XFS_INO_BIGTIME_EPOCH should probably get
> > > renamed to something less confusing, like...
> > >
> > > /*
> > >  * Since the bigtime epoch is Dec. 1901, add this number of seconds to
> > >  * an ondisk bigtime timestamp to convert it to the Unix epoch.
> > >  */
> > > #define XFS_BIGTIME_TO_UNIX		(-XFS_INO_UNIX_BIGTIME_MIN)
> > > 
> > > /*
> > >  * Subtract this many seconds from a Unix epoch timestamp to get the
> > >  * ondisk bigtime timestamp.
> > >  */
> > > #define XFS_UNIX_TO_BIGTIME		(-XFS_BIGTIME_TO_UNIX)
> > > 
> > > Is that clearer?
> > 
> > Hmmm. Definitely better, but how about:
> > 
> > /*
> >  * Bigtime epoch is set exactly to the minimum time value that a
> >  * traditional 32 bit timestamp can represent when using the Unix
> >  * epoch as a reference. Hence the Unix epoch is at a fixed offset
> >  * into the supported bigtime timestamp range.
> >  *
> >  * The bigtime epoch also matches the minimum value an on-disk 32
> >  * bit XFS timestamp can represent so we will not lose any fidelity
> >  * in converting to/from unix and bigtime timestamps.
> >  */
> > #define XFS_BIGTIME_EPOCH_OFFSET	(XFS_INO_TIME_MIN)
> > 
> > And then two static inline helpers follow immediately -
> > xfs_bigtime_to_unix() and xfs_bigtime_from_unix() can do the
> > conversion between the two formats and the XFS_BIGTIME_EPOCH_OFFSET
> > variable never gets seen anywhere else in the code. To set the max
> > timestamp value the superblock holds for the filesystem, just
> > calculate it directly via a call to xfs_bigtime_to_unix(-1ULL, ...)
> 
> <nod>
> 
> --D
> 
> > > > Hmmm. I got 16299260424 when I just ran this through a simple calc.
> > > > Mind you, no calculator app I found could handle unsigned 64 bit
> > > > values natively (signed 64 bit is good enough for everyone!) so
> > > > maybe I got an off-by one here...
> > > 
> > > -1ULL = 18,446,744,073,709,551,615
> > > -1ULL / NSEC_PER_SEC = 18,446,744,073
> > > (-1ULL / NSEC_PER_SEC) - XFS_INO_BIGTIME_EPOCH = 16,299,260,425
> > 
> > Yup, I got an off by one thanks to integer rounding on the
> > division. I should have just done it long hand like that...
> > 
> > Cheers,
> > 
> > Dave.
> > -- 
> > Dave Chinner
> > [e-mail]

Darrick J. Wong Aug. 25, 2020, 12:39 a.m. UTC | #8

On Sun, Aug 23, 2020 at 07:43:41PM -0700, Darrick J. Wong wrote:
> On Sat, Aug 22, 2020 at 08:33:19AM +0100, Christoph Hellwig wrote:
> > >   * in the AGI header so that we can skip the finobt walk at mount time when
> > > @@ -855,12 +862,18 @@ struct xfs_agfl {
> > >   *
> > >   * Inode timestamps consist of signed 32-bit counters for seconds and
> > >   * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
> > > + *
> > > + * When bigtime is enabled, timestamps become an unsigned 64-bit nanoseconds
> > > + * counter.  Time zero is the start of the classic timestamp range.
> > >   */
> > >  union xfs_timestamp {
> > >  	struct {
> > >  		__be32		t_sec;		/* timestamp seconds */
> > >  		__be32		t_nsec;		/* timestamp nanoseconds */
> > >  	};
> > > +
> > > +	/* Nanoseconds since the bigtime epoch. */
> > > +	__be64			t_bigtime;
> > >  };
> > 
> > So do we really need the union here?  What about:
> > 
> >  (1) keep the typedef instead of removing it
> >  (2) switch the typedef to be just a __be64, and use trivial helpers
> >      to extract the two separate legacy sec/nsec field
> >  (3) PROFIT!!!
> 
> Been there, done that.  Dave suggested some replacement code (which
> corrupted the values), then I modified that into a correct version,
> which then made smatch angry because it doesn't like code that does bit
> shifts on __be64 values.

Backing up here, I've realized that my own analysis of Dave's pseudocode
was incorrect.

On a little endian machine, we'll start with the following.  A is the
LSB of seconds; D is the MSB of seconds; E is the LSB of nsec, and H is
the MSB of nsec.

  sec  nsec (incore)
  l  m l  m
  ABCD EFGH

Now we encode that with an old kernel, which calls cpu_to_be32 to turn
that into:

  sec  nsec (ondisk)
  m  l m  l
  DCBA HGFE

Move over to a new kernel, and that becomes:

  tstamp (ondisk)
  m      l
  DCBAHGFE

Next we decode with be64_to_cpu:

  tstamp (incore)
  l      m
  EFGHABCD

Now we extract nsec from (tstamp & -1U) and sec from (tstamp >> 32):

  sec  nsec
  l  m l  m
  ABCD EFGH

So yes, masking and shifting /after/ the endian conversion works just
fine and doesn't throw any sparse/smatch errors.

Now on a big endian machine:

  sec  nsec (incore)
  m  l m  l
  DCBA HGFE

Now we encode that with an old kernel, which calls cpu_to_be32 (a nop)
to turn that into:

  sec  nsec (ondisk)
  m  l m  l
  DCBA HGFE

Move over to a new kernel, and that becomes:

  tstamp (ondisk)
  m      l
  DCBAHGFE

Next we decode with be64_to_cpu (a nop):

  tstamp (incore)
  m      l
  DCBAHGFE

Now we extract nsec from (tstamp & -1U) and sec from (tstamp >> 32):

  sec  nsec
  m  l m  l
  DCBA HGFE

Works fine here too.

Now the /truly/ nasty case here is xfs_ictimestamp, since we log the
inode core in host endian format.  If we start with this the vfs
timestamp on a new kernel:

  sec  nsec (incore)
  l  m l  m
  ABCD EFGH

We need to encode that as:

  tstamp (ondisk)
  l      m
  ABCDEFGH

The only way to do this is: (nsec << 32) | (sec & -1U).  That makes the
log timestamp encoding is the opposite of what we do for the ondisk
inodes, because log formats don't use cpu_to_be64.

At least for a big endian machine, log timestamp coding is easy:

  sec  nsec (incore)
  m  l m  l
  DCBA HGFE

We need to encode that as:

  tstamp (ondisk)
  m      l
  DCBAHGFE

And the only way to get there is (sec << 32) | (nsec & -1U), which is
what the ondisk inode timestamp coding does.

I still think this is grody, but at least now now I have a new fstest to
make sure that log recovery doesn't trip over this.  So, you were
technically right and I was wrong.  We'll see how you like the new
stuff. ;)

--D

> > > +/* Convert an ondisk timestamp into the 64-bit safe incore format. */
> > >  void
> > >  xfs_inode_from_disk_timestamp(
> > > +	struct xfs_dinode		*dip,
> > >  	struct timespec64		*tv,
> > >  	const union xfs_timestamp	*ts)
> > 
> > I think passing ts by value might lead to somewhat better code
> > generation on modern ABIs (and older ABIs just fall back to pass
> > by reference transparently).
> 
> Hm, ok.  I did not know that. :)
> 
> > >  {
> > > +	if (dip->di_version >= 3 &&
> > > +	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {
> > 
> > Do we want a helper for this condition?
> 
> Yes, yes we do.  Will add.
> 
> > > +		uint64_t		t = be64_to_cpu(ts->t_bigtime);
> > > +		uint64_t		s;
> > > +		uint32_t		n;
> > > +
> > > +		s = div_u64_rem(t, NSEC_PER_SEC, &n);
> > > +		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
> > > +		tv->tv_nsec = n;
> > > +		return;
> > > +	}
> > > +
> > >  	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
> > >  	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
> > 
> > Nit: for these kinds of symmetric conditions and if/else feels a little
> > more natural.
> > 
> > > +		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);
> > 
> > This adds a > 80 char line.
> 
> Do we care now that checkpatch has been changed to allow up to 100
> columns?
> 
> > > +	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
> > > +		uint64_t		t;
> > > +
> > > +		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
> > > +		t *= NSEC_PER_SEC;
> > > +		its->t_bigtime = t + ts->tv_nsec;
> > 
> > This calculation is dupliated in two places, might be worth
> > adding a little helper (which will need to get the sec/nsec values
> > passed separately due to the different structures).
> > 
> > > +		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);
> > 
> > Another line over 8 characters here.
> > 
> > > +	if (xfs_sb_version_hasbigtime(&mp->m_sb)) {
> > > +		sb->s_time_min = XFS_INO_BIGTIME_MIN;
> > > +		sb->s_time_max = XFS_INO_BIGTIME_MAX;
> > > +	} else {
> > > +		sb->s_time_min = XFS_INO_TIME_MIN;
> > > +		sb->s_time_max = XFS_INO_TIME_MAX;
> > > +	}
> > 
> > This is really a comment on the earlier patch, but maybe we should
> > name the old constants with "OLD" or "LEGACY" or "SMALL" in the name?
> 
> Yes, good suggestion!
> 
> > > @@ -1494,6 +1499,10 @@ xfs_fc_fill_super(
> > >  	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
> > >  		sb->s_flags |= SB_I_VERSION;
> > >  
> > > +	if (xfs_sb_version_hasbigtime(&mp->m_sb))
> > > +		xfs_warn(mp,
> > > + "EXPERIMENTAL big timestamp feature in use. Use at your own risk!");
> > > +
> > 
> > Is there any good reason to mark this experimental?
> 
> As you and Dave have both pointed out, there are plenty of stupid bugs
> still in this.  I think I'd like to have at least one EXPERIMENTAL cycle
> to make sure I didn't commit anything pathologically stupid in here.
> 
> <cough> ext4 34-bit sign extension bug <cough>.
> 
> --D

Patch

diff --git a/fs/xfs/libxfs/xfs_format.h b/fs/xfs/libxfs/xfs_format.h
index 772113db41aa..57343973e5e5 100644
--- a/fs/xfs/libxfs/xfs_format.h
+++ b/fs/xfs/libxfs/xfs_format.h
@@ -467,6 +467,7 @@  xfs_sb_has_ro_compat_feature(
 #define XFS_SB_FEAT_INCOMPAT_FTYPE	(1 << 0)	/* filetype in dirent */
 #define XFS_SB_FEAT_INCOMPAT_SPINODES	(1 << 1)	/* sparse inode chunks */
 #define XFS_SB_FEAT_INCOMPAT_META_UUID	(1 << 2)	/* metadata UUID */
+#define XFS_SB_FEAT_INCOMPAT_BIGTIME	(1 << 3)	/* large timestamps */
 #define XFS_SB_FEAT_INCOMPAT_ALL \
 		(XFS_SB_FEAT_INCOMPAT_FTYPE|	\
 		 XFS_SB_FEAT_INCOMPAT_SPINODES|	\
@@ -565,6 +566,12 @@  static inline bool xfs_sb_version_hasreflink(struct xfs_sb *sbp)
 		(sbp->sb_features_ro_compat & XFS_SB_FEAT_RO_COMPAT_REFLINK);
 }
 
+static inline bool xfs_sb_version_hasbigtime(struct xfs_sb *sbp)
+{
+	return XFS_SB_VERSION_NUM(sbp) == XFS_SB_VERSION_5 &&
+		(sbp->sb_features_incompat & XFS_SB_FEAT_INCOMPAT_BIGTIME);
+}
+
 /*
  * Inode btree block counter.  We record the number of inobt and finobt blocks
  * in the AGI header so that we can skip the finobt walk at mount time when
@@ -855,12 +862,18 @@  struct xfs_agfl {
  *
  * Inode timestamps consist of signed 32-bit counters for seconds and
  * nanoseconds; time zero is the Unix epoch, Jan  1 00:00:00 UTC 1970.
+ *
+ * When bigtime is enabled, timestamps become an unsigned 64-bit nanoseconds
+ * counter.  Time zero is the start of the classic timestamp range.
  */
 union xfs_timestamp {
 	struct {
 		__be32		t_sec;		/* timestamp seconds */
 		__be32		t_nsec;		/* timestamp nanoseconds */
 	};
+
+	/* Nanoseconds since the bigtime epoch. */
+	__be64			t_bigtime;
 };
 
 /*
@@ -875,6 +888,25 @@  union xfs_timestamp {
  */
 #define XFS_INO_TIME_MAX	((int64_t)S32_MAX)
 
+/*
+ * Number of seconds between the start of the bigtime timestamp range and the
+ * start of the Unix epoch.
+ */
+#define XFS_INO_BIGTIME_EPOCH	(-XFS_INO_TIME_MIN)
+
+/*
+ * Smallest possible timestamp with big timestamps, which is
+ * Dec 13 20:45:52 UTC 1901.
+ */
+#define XFS_INO_BIGTIME_MIN	(XFS_INO_TIME_MIN)
+
+/*
+ * Largest possible timestamp with big timestamps, which is
+ * Jul  2 20:20:25 UTC 2486.
+ */
+#define XFS_INO_BIGTIME_MAX	((int64_t)((-1ULL / NSEC_PER_SEC) - \
+					   XFS_INO_BIGTIME_EPOCH))
+
 /*
  * On-disk inode structure.
  *
@@ -1100,12 +1132,16 @@  static inline void xfs_dinode_put_rdev(struct xfs_dinode *dip, xfs_dev_t rdev)
 #define XFS_DIFLAG2_DAX_BIT	0	/* use DAX for this inode */
 #define XFS_DIFLAG2_REFLINK_BIT	1	/* file's blocks may be shared */
 #define XFS_DIFLAG2_COWEXTSIZE_BIT   2  /* copy on write extent size hint */
+#define XFS_DIFLAG2_BIGTIME_BIT	3	/* big timestamps */
+
 #define XFS_DIFLAG2_DAX		(1 << XFS_DIFLAG2_DAX_BIT)
 #define XFS_DIFLAG2_REFLINK     (1 << XFS_DIFLAG2_REFLINK_BIT)
 #define XFS_DIFLAG2_COWEXTSIZE  (1 << XFS_DIFLAG2_COWEXTSIZE_BIT)
+#define XFS_DIFLAG2_BIGTIME	(1 << XFS_DIFLAG2_BIGTIME_BIT)
 
 #define XFS_DIFLAG2_ANY \
-	(XFS_DIFLAG2_DAX | XFS_DIFLAG2_REFLINK | XFS_DIFLAG2_COWEXTSIZE)
+	(XFS_DIFLAG2_DAX | XFS_DIFLAG2_REFLINK | XFS_DIFLAG2_COWEXTSIZE | \
+	 XFS_DIFLAG2_BIGTIME)
 
 /*
  * Inode number format:
diff --git a/fs/xfs/libxfs/xfs_fs.h b/fs/xfs/libxfs/xfs_fs.h
index 84bcffa87753..2a2e3cfd94f0 100644
--- a/fs/xfs/libxfs/xfs_fs.h
+++ b/fs/xfs/libxfs/xfs_fs.h
@@ -249,6 +249,7 @@  typedef struct xfs_fsop_resblks {
 #define XFS_FSOP_GEOM_FLAGS_SPINODES	(1 << 18) /* sparse inode chunks   */
 #define XFS_FSOP_GEOM_FLAGS_RMAPBT	(1 << 19) /* reverse mapping btree */
 #define XFS_FSOP_GEOM_FLAGS_REFLINK	(1 << 20) /* files can share blocks */
+#define XFS_FSOP_GEOM_FLAGS_BIGTIME	(1 << 21) /* 64-bit nsec timestamps */
 
 /*
  * Minimum and maximum sizes need for growth checks.
diff --git a/fs/xfs/libxfs/xfs_inode_buf.c b/fs/xfs/libxfs/xfs_inode_buf.c
index cc1316a5fe0c..c59ddb56bb90 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.c
+++ b/fs/xfs/libxfs/xfs_inode_buf.c
@@ -157,11 +157,25 @@  xfs_imap_to_bp(
 	return 0;
 }
 
+/* Convert an ondisk timestamp into the 64-bit safe incore format. */
 void
 xfs_inode_from_disk_timestamp(
+	struct xfs_dinode		*dip,
 	struct timespec64		*tv,
 	const union xfs_timestamp	*ts)
 {
+	if (dip->di_version >= 3 &&
+	    (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) {
+		uint64_t		t = be64_to_cpu(ts->t_bigtime);
+		uint64_t		s;
+		uint32_t		n;
+
+		s = div_u64_rem(t, NSEC_PER_SEC, &n);
+		tv->tv_sec = s - XFS_INO_BIGTIME_EPOCH;
+		tv->tv_nsec = n;
+		return;
+	}
+
 	tv->tv_sec = (int)be32_to_cpu(ts->t_sec);
 	tv->tv_nsec = (int)be32_to_cpu(ts->t_nsec);
 }
@@ -220,9 +234,9 @@  xfs_inode_from_disk(
 	 * a time before epoch is converted to a time long after epoch
 	 * on 64 bit systems.
 	 */
-	xfs_inode_from_disk_timestamp(&inode->i_atime, &from->di_atime);
-	xfs_inode_from_disk_timestamp(&inode->i_mtime, &from->di_mtime);
-	xfs_inode_from_disk_timestamp(&inode->i_ctime, &from->di_ctime);
+	xfs_inode_from_disk_timestamp(from, &inode->i_atime, &from->di_atime);
+	xfs_inode_from_disk_timestamp(from, &inode->i_mtime, &from->di_mtime);
+	xfs_inode_from_disk_timestamp(from, &inode->i_ctime, &from->di_ctime);
 
 	to->di_size = be64_to_cpu(from->di_size);
 	to->di_nblocks = be64_to_cpu(from->di_nblocks);
@@ -235,9 +249,17 @@  xfs_inode_from_disk(
 	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
 		inode_set_iversion_queried(inode,
 					   be64_to_cpu(from->di_changecount));
-		xfs_inode_from_disk_timestamp(&to->di_crtime, &from->di_crtime);
+		xfs_inode_from_disk_timestamp(from, &to->di_crtime,
+				&from->di_crtime);
 		to->di_flags2 = be64_to_cpu(from->di_flags2);
 		to->di_cowextsize = be32_to_cpu(from->di_cowextsize);
+		/*
+		 * Set the bigtime flag incore so that we automatically convert
+		 * this inode's ondisk timestamps to bigtime format the next
+		 * time we write the inode core to disk.
+		 */
+		if (xfs_sb_version_hasbigtime(&ip->i_mount->m_sb))
+			to->di_flags2 |= XFS_DIFLAG2_BIGTIME;
 	}
 
 	error = xfs_iformat_data_fork(ip, from);
@@ -259,9 +281,19 @@  xfs_inode_from_disk(
 
 void
 xfs_inode_to_disk_timestamp(
+	struct xfs_icdinode		*from,
 	union xfs_timestamp		*ts,
 	const struct timespec64		*tv)
 {
+	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
+		uint64_t		t;
+
+		t = (uint64_t)(tv->tv_sec + XFS_INO_BIGTIME_EPOCH);
+		t *= NSEC_PER_SEC;
+		ts->t_bigtime = cpu_to_be64(t + tv->tv_nsec);
+		return;
+	}
+
 	ts->t_sec = cpu_to_be32(tv->tv_sec);
 	ts->t_nsec = cpu_to_be32(tv->tv_nsec);
 }
@@ -285,9 +317,9 @@  xfs_inode_to_disk(
 	to->di_projid_hi = cpu_to_be16(from->di_projid >> 16);
 
 	memset(to->di_pad, 0, sizeof(to->di_pad));
-	xfs_inode_to_disk_timestamp(&to->di_atime, &inode->i_atime);
-	xfs_inode_to_disk_timestamp(&to->di_mtime, &inode->i_mtime);
-	xfs_inode_to_disk_timestamp(&to->di_ctime, &inode->i_ctime);
+	xfs_inode_to_disk_timestamp(from, &to->di_atime, &inode->i_atime);
+	xfs_inode_to_disk_timestamp(from, &to->di_mtime, &inode->i_mtime);
+	xfs_inode_to_disk_timestamp(from, &to->di_ctime, &inode->i_ctime);
 	to->di_nlink = cpu_to_be32(inode->i_nlink);
 	to->di_gen = cpu_to_be32(inode->i_generation);
 	to->di_mode = cpu_to_be16(inode->i_mode);
@@ -306,7 +338,8 @@  xfs_inode_to_disk(
 	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
 		to->di_version = 3;
 		to->di_changecount = cpu_to_be64(inode_peek_iversion(inode));
-		xfs_inode_to_disk_timestamp(&to->di_crtime, &from->di_crtime);
+		xfs_inode_to_disk_timestamp(from, &to->di_crtime,
+				&from->di_crtime);
 		to->di_flags2 = cpu_to_be64(from->di_flags2);
 		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
 		to->di_ino = cpu_to_be64(ip->i_ino);
@@ -526,6 +559,11 @@  xfs_dinode_verify(
 	if (fa)
 		return fa;
 
+	/* bigtime iflag can only happen on bigtime filesystems */
+	if ((flags2 & (XFS_DIFLAG2_BIGTIME)) &&
+	    !xfs_sb_version_hasbigtime(&mp->m_sb))
+		return __this_address;
+
 	return NULL;
 }
 
diff --git a/fs/xfs/libxfs/xfs_inode_buf.h b/fs/xfs/libxfs/xfs_inode_buf.h
index f6160033fcbd..3b25e43eafa1 100644
--- a/fs/xfs/libxfs/xfs_inode_buf.h
+++ b/fs/xfs/libxfs/xfs_inode_buf.h
@@ -58,9 +58,9 @@  xfs_failaddr_t xfs_inode_validate_cowextsize(struct xfs_mount *mp,
 		uint32_t cowextsize, uint16_t mode, uint16_t flags,
 		uint64_t flags2);
 
-void xfs_inode_from_disk_timestamp(struct timespec64 *tv,
-		const union xfs_timestamp *ts);
-void xfs_inode_to_disk_timestamp(union xfs_timestamp *ts,
-		const struct timespec64 *tv);
+void xfs_inode_from_disk_timestamp(struct xfs_dinode *dip,
+		struct timespec64 *tv, const union xfs_timestamp *ts);
+void xfs_inode_to_disk_timestamp(struct xfs_icdinode *from,
+		union xfs_timestamp *ts, const struct timespec64 *tv);
 
 #endif	/* __XFS_INODE_BUF_H__ */
diff --git a/fs/xfs/libxfs/xfs_log_format.h b/fs/xfs/libxfs/xfs_log_format.h
index 17c83d29998c..569721f7f9e5 100644
--- a/fs/xfs/libxfs/xfs_log_format.h
+++ b/fs/xfs/libxfs/xfs_log_format.h
@@ -373,6 +373,9 @@  union xfs_ictimestamp {
 		int32_t		t_sec;		/* timestamp seconds */
 		int32_t		t_nsec;		/* timestamp nanoseconds */
 	};
+
+	/* Nanoseconds since the bigtime epoch. */
+	uint64_t		t_bigtime;
 };
 
 /*
diff --git a/fs/xfs/libxfs/xfs_sb.c b/fs/xfs/libxfs/xfs_sb.c
index ae9aaf1f34bf..d5d60cd1c2ea 100644
--- a/fs/xfs/libxfs/xfs_sb.c
+++ b/fs/xfs/libxfs/xfs_sb.c
@@ -1166,6 +1166,8 @@  xfs_fs_geometry(
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_RMAPBT;
 	if (xfs_sb_version_hasreflink(sbp))
 		geo->flags |= XFS_FSOP_GEOM_FLAGS_REFLINK;
+	if (xfs_sb_version_hasbigtime(sbp))
+		geo->flags |= XFS_FSOP_GEOM_FLAGS_BIGTIME;
 	if (xfs_sb_version_hassector(sbp))
 		geo->logsectsize = sbp->sb_logsectsize;
 	else
diff --git a/fs/xfs/libxfs/xfs_trans_inode.c b/fs/xfs/libxfs/xfs_trans_inode.c
index e15129647e00..71aca4587715 100644
--- a/fs/xfs/libxfs/xfs_trans_inode.c
+++ b/fs/xfs/libxfs/xfs_trans_inode.c
@@ -131,6 +131,17 @@  xfs_trans_log_inode(
 			iversion_flags = XFS_ILOG_CORE;
 	}
 
+	/*
+	 * If we're updating the inode core or the timestamps and it's possible
+	 * to upgrade this inode to bigtime format, do so now.
+	 */
+	if ((flags & (XFS_ILOG_CORE | XFS_ILOG_TIMESTAMP)) &&
+	    xfs_sb_version_hasbigtime(&tp->t_mountp->m_sb) &&
+	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {
+		ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
+		flags |= XFS_ILOG_CORE;
+	}
+
 	/*
 	 * Record the specific change for fdatasync optimisation. This allows
 	 * fdatasync to skip log forces for inodes that are only timestamp
diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
index 9f036053fdb7..6f00309de2d4 100644
--- a/fs/xfs/scrub/inode.c
+++ b/fs/xfs/scrub/inode.c
@@ -190,6 +190,11 @@  xchk_inode_flags2(
 	if ((flags2 & XFS_DIFLAG2_DAX) && (flags2 & XFS_DIFLAG2_REFLINK))
 		goto bad;
 
+	/* no bigtime iflag without the bigtime feature */
+	if (!xfs_sb_version_hasbigtime(&mp->m_sb) &&
+	    (flags2 & XFS_DIFLAG2_BIGTIME))
+		goto bad;
+
 	return;
 bad:
 	xchk_ino_set_corrupt(sc, ino);
@@ -199,11 +204,12 @@  static inline void
 xchk_dinode_nsec(
 	struct xfs_scrub		*sc,
 	xfs_ino_t			ino,
+	struct xfs_dinode		*dip,
 	const union xfs_timestamp	*ts)
 {
 	struct timespec64		tv;
 
-	xfs_inode_from_disk_timestamp(&tv, ts);
+	xfs_inode_from_disk_timestamp(dip, &tv, ts);
 	if (tv.tv_nsec < 0 || tv.tv_nsec >= NSEC_PER_SEC)
 		xchk_ino_set_corrupt(sc, ino);
 }
@@ -306,9 +312,9 @@  xchk_dinode(
 	}
 
 	/* di_[amc]time.nsec */
-	xchk_dinode_nsec(sc, ino, &dip->di_atime);
-	xchk_dinode_nsec(sc, ino, &dip->di_mtime);
-	xchk_dinode_nsec(sc, ino, &dip->di_ctime);
+	xchk_dinode_nsec(sc, ino, dip, &dip->di_atime);
+	xchk_dinode_nsec(sc, ino, dip, &dip->di_mtime);
+	xchk_dinode_nsec(sc, ino, dip, &dip->di_ctime);
 
 	/*
 	 * di_size.  xfs_dinode_verify checks for things that screw up
@@ -413,7 +419,7 @@  xchk_dinode(
 	}
 
 	if (dip->di_version >= 3) {
-		xchk_dinode_nsec(sc, ino, &dip->di_crtime);
+		xchk_dinode_nsec(sc, ino, dip, &dip->di_crtime);
 		xchk_inode_flags2(sc, dip, ino, mode, flags, flags2);
 		xchk_inode_cowextsize(sc, dip, ino, mode, flags,
 				flags2);
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index c06129cffba9..bbc309bc70c4 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -841,6 +841,8 @@  xfs_ialloc(
 	if (xfs_sb_version_has_v3inode(&mp->m_sb)) {
 		inode_set_iversion(inode, 1);
 		ip->i_d.di_flags2 = 0;
+		if (xfs_sb_version_hasbigtime(&mp->m_sb))
+			ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;
 		ip->i_d.di_cowextsize = 0;
 		ip->i_d.di_crtime = tv;
 	}
@@ -2717,7 +2719,11 @@  xfs_ifree(
 
 	VFS_I(ip)->i_mode = 0;		/* mark incore inode as free */
 	ip->i_d.di_flags = 0;
-	ip->i_d.di_flags2 = 0;
+	/*
+	 * Preserve the bigtime flag so that di_ctime accurately stores the
+	 * deletion time.
+	 */
+	ip->i_d.di_flags2 &= XFS_DIFLAG2_BIGTIME;
 	ip->i_d.di_dmevmask = 0;
 	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
 	ip->i_df.if_format = XFS_DINODE_FMT_EXTENTS;
@@ -3503,6 +3509,13 @@  xfs_iflush(
 			__func__, ip->i_ino, ip->i_d.di_forkoff, ip);
 		goto flush_out;
 	}
+	if (xfs_sb_version_hasbigtime(&mp->m_sb) &&
+	    !(ip->i_d.di_flags2 & XFS_DIFLAG2_BIGTIME)) {
+		xfs_alert_tag(mp, XFS_PTAG_IFLUSH,
+			"%s: bad inode %Lu, bigtime unset, ptr "PTR_FMT,
+			__func__, ip->i_ino, ip);
+		goto flush_out;
+	}
 
 	/*
 	 * Inode item log recovery for v2 inodes are dependent on the
diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
index cec6d4d82bc4..c38af3eea48f 100644
--- a/fs/xfs/xfs_inode_item.c
+++ b/fs/xfs/xfs_inode_item.c
@@ -298,9 +298,16 @@  xfs_inode_item_format_attr_fork(
 /* Write a log_dinode timestamp into an ondisk inode timestamp. */
 static inline void
 xfs_log_dinode_to_disk_ts(
+	struct xfs_log_dinode		*from,
 	union xfs_timestamp		*ts,
 	const union xfs_ictimestamp	*its)
 {
+	if (from->di_version >= 3 &&
+	    (from->di_flags2 & XFS_DIFLAG2_BIGTIME)) {
+		ts->t_bigtime = cpu_to_be64(its->t_bigtime);
+		return;
+	}
+
 	ts->t_sec = cpu_to_be32(its->t_sec);
 	ts->t_nsec = cpu_to_be32(its->t_nsec);
 }
@@ -322,9 +329,9 @@  xfs_log_dinode_to_disk(
 	to->di_projid_hi = cpu_to_be16(from->di_projid_hi);
 	memcpy(to->di_pad, from->di_pad, sizeof(to->di_pad));
 
-	xfs_log_dinode_to_disk_ts(&to->di_atime, &from->di_atime);
-	xfs_log_dinode_to_disk_ts(&to->di_mtime, &from->di_mtime);
-	xfs_log_dinode_to_disk_ts(&to->di_ctime, &from->di_ctime);
+	xfs_log_dinode_to_disk_ts(from, &to->di_atime, &from->di_atime);
+	xfs_log_dinode_to_disk_ts(from, &to->di_mtime, &from->di_mtime);
+	xfs_log_dinode_to_disk_ts(from, &to->di_ctime, &from->di_ctime);
 
 	to->di_size = cpu_to_be64(from->di_size);
 	to->di_nblocks = cpu_to_be64(from->di_nblocks);
@@ -340,7 +347,7 @@  xfs_log_dinode_to_disk(
 
 	if (from->di_version == 3) {
 		to->di_changecount = cpu_to_be64(from->di_changecount);
-		xfs_log_dinode_to_disk_ts(&to->di_crtime, &from->di_crtime);
+		xfs_log_dinode_to_disk_ts(from, &to->di_crtime, &from->di_crtime);
 		to->di_flags2 = cpu_to_be64(from->di_flags2);
 		to->di_cowextsize = cpu_to_be32(from->di_cowextsize);
 		to->di_ino = cpu_to_be64(from->di_ino);
@@ -356,9 +363,19 @@  xfs_log_dinode_to_disk(
 /* Write an incore inode timestamp into a log_dinode timestamp. */
 static inline void
 xfs_inode_to_log_dinode_ts(
+	struct xfs_icdinode		*from,
 	union xfs_ictimestamp		*its,
 	const struct timespec64		*ts)
 {
+	if (from->di_flags2 & XFS_DIFLAG2_BIGTIME) {
+		uint64_t		t;
+
+		t = (uint64_t)(ts->tv_sec + XFS_INO_BIGTIME_EPOCH);
+		t *= NSEC_PER_SEC;
+		its->t_bigtime = t + ts->tv_nsec;
+		return;
+	}
+
 	its->t_sec = ts->tv_sec;
 	its->t_nsec = ts->tv_nsec;
 }
@@ -381,9 +398,9 @@  xfs_inode_to_log_dinode(
 
 	memset(to->di_pad, 0, sizeof(to->di_pad));
 	memset(to->di_pad3, 0, sizeof(to->di_pad3));
-	xfs_inode_to_log_dinode_ts(&to->di_atime, &inode->i_atime);
-	xfs_inode_to_log_dinode_ts(&to->di_mtime, &inode->i_mtime);
-	xfs_inode_to_log_dinode_ts(&to->di_ctime, &inode->i_ctime);
+	xfs_inode_to_log_dinode_ts(from, &to->di_atime, &inode->i_atime);
+	xfs_inode_to_log_dinode_ts(from, &to->di_mtime, &inode->i_mtime);
+	xfs_inode_to_log_dinode_ts(from, &to->di_ctime, &inode->i_ctime);
 	to->di_nlink = inode->i_nlink;
 	to->di_gen = inode->i_generation;
 	to->di_mode = inode->i_mode;
@@ -405,7 +422,7 @@  xfs_inode_to_log_dinode(
 	if (xfs_sb_version_has_v3inode(&ip->i_mount->m_sb)) {
 		to->di_version = 3;
 		to->di_changecount = inode_peek_iversion(inode);
-		xfs_inode_to_log_dinode_ts(&to->di_crtime, &from->di_crtime);
+		xfs_inode_to_log_dinode_ts(from, &to->di_crtime, &from->di_crtime);
 		to->di_flags2 = from->di_flags2;
 		to->di_cowextsize = from->di_cowextsize;
 		to->di_ino = ip->i_ino;
diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index 6f22a66777cd..13396c3665d1 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1190,7 +1190,8 @@  xfs_flags2diflags2(
 	unsigned int		xflags)
 {
 	uint64_t		di_flags2 =
-		(ip->i_d.di_flags2 & XFS_DIFLAG2_REFLINK);
+		(ip->i_d.di_flags2 & (XFS_DIFLAG2_REFLINK |
+				      XFS_DIFLAG2_BIGTIME));
 
 	if (xflags & FS_XFLAG_DAX)
 		di_flags2 |= XFS_DIFLAG2_DAX;
diff --git a/fs/xfs/xfs_ondisk.h b/fs/xfs/xfs_ondisk.h
index 7158a8de719f..3e0c677cff15 100644
--- a/fs/xfs/xfs_ondisk.h
+++ b/fs/xfs/xfs_ondisk.h
@@ -25,6 +25,9 @@  xfs_check_limits(void)
 	/* make sure timestamp limits are correct */
 	XFS_CHECK_VALUE(XFS_INO_TIME_MIN,			-2147483648LL);
 	XFS_CHECK_VALUE(XFS_INO_TIME_MAX,			2147483647LL);
+	XFS_CHECK_VALUE(XFS_INO_BIGTIME_EPOCH,			2147483648LL);
+	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MIN,			-2147483648LL);
+	XFS_CHECK_VALUE(XFS_INO_BIGTIME_MAX,			16299260425LL);
 	XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MIN,			1LL);
 	XFS_CHECK_VALUE(XFS_DQ_TIMEOUT_MAX,			4294967295LL);
 	XFS_CHECK_VALUE(XFS_DQ_GRACE_MIN,			0LL);
@@ -58,6 +61,9 @@  xfs_check_ondisk_structs(void)
 	XFS_CHECK_STRUCT_SIZE(struct xfs_rmap_key,		20);
 	XFS_CHECK_STRUCT_SIZE(struct xfs_rmap_rec,		24);
 	XFS_CHECK_STRUCT_SIZE(union xfs_timestamp,		8);
+	XFS_CHECK_OFFSET(union xfs_timestamp, t_bigtime,	0);
+	XFS_CHECK_OFFSET(union xfs_timestamp, t_sec,		0);
+	XFS_CHECK_OFFSET(union xfs_timestamp, t_nsec,		4);
 	XFS_CHECK_STRUCT_SIZE(xfs_alloc_key_t,			8);
 	XFS_CHECK_STRUCT_SIZE(xfs_alloc_ptr_t,			4);
 	XFS_CHECK_STRUCT_SIZE(xfs_alloc_rec_t,			8);
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 375f05a47ba4..b19ae052f7a3 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1484,8 +1484,13 @@  xfs_fc_fill_super(
 	sb->s_maxbytes = MAX_LFS_FILESIZE;
 	sb->s_max_links = XFS_MAXLINK;
 	sb->s_time_gran = 1;
-	sb->s_time_min = XFS_INO_TIME_MIN;
-	sb->s_time_max = XFS_INO_TIME_MAX;
+	if (xfs_sb_version_hasbigtime(&mp->m_sb)) {
+		sb->s_time_min = XFS_INO_BIGTIME_MIN;
+		sb->s_time_max = XFS_INO_BIGTIME_MAX;
+	} else {
+		sb->s_time_min = XFS_INO_TIME_MIN;
+		sb->s_time_max = XFS_INO_TIME_MAX;
+	}
 	sb->s_iflags |= SB_I_CGROUPWB;
 
 	set_posix_acl_flag(sb);
@@ -1494,6 +1499,10 @@  xfs_fc_fill_super(
 	if (XFS_SB_VERSION_NUM(&mp->m_sb) == XFS_SB_VERSION_5)
 		sb->s_flags |= SB_I_VERSION;
 
+	if (xfs_sb_version_hasbigtime(&mp->m_sb))
+		xfs_warn(mp,
+ "EXPERIMENTAL big timestamp feature in use. Use at your own risk!");
+
 	if (mp->m_flags & XFS_MOUNT_DAX_ALWAYS) {
 		bool rtdev_is_dax = false, datadev_is_dax;