Re: [PATCH 2/6] xfs_repair: clear DIFLAG2_NREXT64 when filesystem doesn't support nrext64

[Dave Chinner <david@fromorbit.com>]

On Wed, Jun 29, 2022 at 04:10:47PM -0700, Darrick J. Wong wrote:
> On Wed, Jun 29, 2022 at 08:58:57AM +1000, Dave Chinner wrote:
> > On Tue, Jun 28, 2022 at 01:49:25PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <djwong@kernel.org>
> > > 
> > > Clear the nrext64 inode flag if the filesystem doesn't have the nrext64
> > > feature enabled in the superblock.
> > > 
> > > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > > ---
> > >  repair/dinode.c |   19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > > 
> > > 
> > > diff --git a/repair/dinode.c b/repair/dinode.c
> > > index 00de31fb..547c5833 100644
> > > --- a/repair/dinode.c
> > > +++ b/repair/dinode.c
> > > @@ -2690,6 +2690,25 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
> > >  			}
> > >  		}
> > >  
> > > +		if (xfs_dinode_has_large_extent_counts(dino) &&
> > > +		    !xfs_has_large_extent_counts(mp)) {
> > > +			if (!uncertain) {
> > > +				do_warn(
> > > +	_("inode %" PRIu64 " is marked large extent counts but file system does not support large extent counts\n"),
> > > +					lino);
> > > +			}
> > > +			flags2 &= ~XFS_DIFLAG2_NREXT64;
> > > +
> > > +			if (no_modify) {
> > > +				do_warn(_("would zero extent counts.\n"));
> > > +			} else {
> > > +				do_warn(_("zeroing extent counts.\n"));
> > > +				dino->di_nextents = 0;
> > > +				dino->di_anextents = 0;
> > > +				*dirty = 1;
> > 
> > Is that necessary? If the existing extent counts are within the
> > bounds of the old extent fields, then shouldn't we just rewrite the
> > current values into the old format rather than trashing all the
> > data/xattrs on the inode?
> 
> It's hard to know what to do here -- we haven't actually checked the
> forks yet, so we don't know if the dinode flag was set but the !nrext64
> extent count fields are ok so all we have to do is clear the dinode
> flag; or if the dinode flag was set, the nrext64 extent count fields are
> correct and must be moved to the !nrext64 fields; or what?
> 
> I guess I could just leave the extent count fields as-is and let the
> process_*_fork functions deal with it.

Can we check it -after- the inode has otherwise been validated
and do the conversion then?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com