Re: [PATCH 17/18] xfs_repair: support bigtime

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

On Tue, Aug 18, 2020 at 05:58:11PM +0300, Amir Goldstein wrote:
> On Tue, Aug 18, 2020 at 2:23 AM Darrick J. Wong <darrick.wong@oracle.com> wrote:
> >
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> >
> > Check the bigtime iflag in relation to the fs feature set.
> >
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Reviewed-by: Amir Goldstein <amir73il@gmail.com>
> 
> and some questions below...
> 
> > ---
> >  repair/dinode.c |   13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> >
> > diff --git a/repair/dinode.c b/repair/dinode.c
> > index ad2f672d8703..3507cd06075d 100644
> > --- a/repair/dinode.c
> > +++ b/repair/dinode.c
> > @@ -2173,7 +2173,8 @@ check_nsec(
> >         union xfs_timestamp     *t,
> >         int                     *dirty)
> >  {
> > -       if ((dip->di_flags2 & be64_to_cpu(XFS_DIFLAG2_BIGTIME)) ||
> > +       if ((dip->di_version >= 3 &&
> 
> It seems a bit strange that di_version check was added by this commit.

Ooh, yeah.  That was added a few patches back, though it shouldn't have
been.

> > +            (dip->di_flags2 & cpu_to_be64(XFS_DIFLAG2_BIGTIME))) ||
> >             be32_to_cpu(t->t_nsec) < NSEC_PER_SEC)
> >                 return;
> >
> > @@ -2601,6 +2602,16 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> >                         flags2 &= ~XFS_DIFLAG2_COWEXTSIZE;
> >                 }
> >
> > +               if ((flags2 & XFS_DIFLAG2_BIGTIME) &&
> > +                   !xfs_sb_version_hasbigtime(&mp->m_sb)) {
> > +                       if (!uncertain) {
> > +                               do_warn(
> > +       _("inode %" PRIu64 " is marked bigtime but file system does not support large timestamps\n"),
> > +                                       lino);
> > +                       }
> > +                       flags2 &= ~XFS_DIFLAG2_BIGTIME;
> 
> Should we maybe also reset the timestamps to epoc in this case?

Yeah, since it's possible that the timestamp would then have an invalid
nsec field once the bigtime field is cleared.  This also needs to log
something about logging "...would zero timestamps and clear flag".

Thanks for catching this. :)

--D

> 
> Thanks,
> Amir.