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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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 <darrick.wong@oracle.com>
> > > > 
> > > > 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 <darrick.wong@oracle.com>
> > > > Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> > > 
> > > ....
> > > 
> > > > @@ -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
> david@fromorbit.com