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

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

On Tue, Aug 18, 2020 at 08:53:45AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 18, 2020 at 03:53:57PM +0300, Amir Goldstein wrote:
> > On Tue, Aug 18, 2020 at 3:00 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > On Tue, Aug 18, 2020 at 1:57 AM Darrick J. Wong <darrick.wong@oracle.com> 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>
> > > > ---
> > > ...
> > >
> > > > diff --git a/fs/xfs/scrub/inode.c b/fs/xfs/scrub/inode.c
> > > > index 9f036053fdb7..b354825f4e51 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;
> > > >
> > > > +       /* the incore bigtime iflag always follows the feature flag */
> > > > +       if (!!xfs_sb_version_hasbigtime(&mp->m_sb) ^
> > > > +           !!(flags2 & XFS_DIFLAG2_BIGTIME))
> > > > +               goto bad;
> > > > +
> > >
> > > Seems like flags2 are not the incore iflags and that the dinode iflags
> > > can very well
> > > have no bigtime on fs with bigtime support:
> > >
> > > xchk_dinode(...
> > > ...
> > >                 flags2 = be64_to_cpu(dip->di_flags2);
> > >
> > > What am I missing?
> > >
> > 
> > Another question. Do we need to worry about xfs_reinit_inode()?
> > It seems not because incore inode should already have the correct bigtime
> > flag. Right?
> 
> I sure hope so.  Any inode being fed to xfs_reinit_inode was fully read
> into memory, then we tore down the VFS inode, but before we tore down
> the XFS inode, someone wanted it back, so all we have to do is reset
> the VFS part of the incore state.
> 
> Therefore, we don't have to do anything about the XFS part of the incore
> state. :)
> 
> > But by same logic, xfs_ifree() should already have the correct
> > bigtime flag as well, so we don't need to set the flag, maybe assert the flag?
> 
> Right.  I think that piece can go since we set the incore flag
> opportunistically now.

Bleh.  I missed the subtlety of:

	ip->i_d.di_flags2 = 0;
	if (xfs_sb_version_hasbigtime(&ip->i_mount->m_sb))
		ip->i_d.di_flags2 |= XFS_DIFLAG2_BIGTIME;

First we zero flags2 entirely, then we re-enable bigtime so that the
final ctime update is written in the correct format.  I guess that could
be simplified to:

	/*
	 * Preserve the bigtime flag so that ctime accurately stores the
	 * deletion time.
	 */
	ip->i_d.di_flags2 &= ~XFS_DIFLAG2_BIGTIME;

> --D
> 
> > 
> > Thanks,
> > Amir.