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

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Christoph Hellwig <hch@infradead.org>,
	Dave Chinner <david@fromorbit.com>
Cc: Amir Goldstein <amir73il@gmail.com>,
	linux-xfs@vger.kernel.org, sandeen@sandeen.net
Subject: Re: [PATCH 08/11] xfs: widen ondisk timestamps to deal with y2038 problem
Date: Mon, 24 Aug 2020 17:39:45 -0700	[thread overview]
Message-ID: <20200825003945.GA6096@magnolia> (raw)
In-Reply-To: <20200824024341.GT6096@magnolia>

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