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 <darrick.wong@xxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxx>
> > > > > Reviewed-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > > >
> > > > ....
> > > >
> > > > > @@ -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
> > david@xxxxxxxxxxxxx
