Re: [PATCH 13/21] xfs: repair inode records

From: "Darrick J. Wong" <darrick.wong@oracle.com>
To: Dave Chinner <david@fromorbit.com>
Cc: linux-xfs@vger.kernel.org
Subject: Re: [PATCH 13/21] xfs: repair inode records
Date: Tue, 3 Jul 2018 18:30:05 -0700	[thread overview]
Message-ID: <20180704013005.GZ32415@magnolia> (raw)
In-Reply-To: <20180704010344.GK2234@dastard>

On Wed, Jul 04, 2018 at 11:03:44AM +1000, Dave Chinner wrote:
> On Tue, Jul 03, 2018 at 05:16:12PM -0700, Darrick J. Wong wrote:
> > On Tue, Jul 03, 2018 at 04:17:18PM +1000, Dave Chinner wrote:
> > > On Sun, Jun 24, 2018 at 12:24:51PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > Try to reinitialize corrupt inodes, or clear the reflink flag
> > > > if it's not needed.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > A comment somewhere that this is only attmepting to repair inodes
> > > that have failed verifier checks on read would be good.
> > 
> > There are a few comments in the callers, e.g.
> > 
> > "Repair all the things that the inode verifiers care about"
> > 
> > "Fix everything xfs_dinode_verify cares about."
> > 
> > "Make sure we can pass the inode buffer verifier."
> > 
> > Hmm, I think maybe you meant that I need to make it more obvious which
> > functions exist to make the verifiers happy (and so there won't be any
> > in-core inodes while they run) vs. which ones fix irregularities that
> > aren't caught as a condition for setting up in-core inodes?
> 
> Well, that too. My main point is that various one-liners don't
> explain the overall picture of what, why and how the repair is being
> done....

Ok.

> > xrep_inode_unchecked_* are the ones that run on in-core inodes; the rest
> > run on inodes so damaged they can't be _iget'd.
> 
> It's completely ambiguous, though: "unchecked" by what, exactly? :P
> 
> > > Also, is there a need to check the inode CRC here?
> > 
> > We already know the inode core is bad, so why not just reset it?
> 
> But you don't recalculate it if di_ok and unlinked_ok are true. It
> only gets recalc'd if when changes need to be made. hence an inode
> that failed the verifier because of a CRC error still won't pass the
> verifier after going through this function.

D'oh.  Thank you for catching that. :)

> > > > +		dip->di_magic = cpu_to_be16(XFS_DINODE_MAGIC);
> > > > +		dip->di_version = 3;
> > > > +		if (!unlinked_ok)
> > > > +			dip->di_next_unlinked = cpu_to_be32(NULLAGINO);
> > > > +		xfs_dinode_calc_crc(mp, dip);
> > > > +		xfs_trans_buf_set_type(tp, bp, XFS_BLFT_DINO_BUF);
> > > > +		xfs_trans_log_buf(tp, bp, ioff, ioff + sizeof(*dip) - 1);
> > > 
> > > Hmmmm. how does this interact with other transactions in repair that
> > > might have logged changes to the same in-core inode? If it was just
> > > changing the unlinked pointer, then that would be ok, but
> > > magic/version are overwritten by the inode item recovery...
> > 
> > There shouldn't be an in-core inode; this function should only get
> > called if we failed to _iget the inode, which implies that nobody else
> > has an in-core inode.
> 
> OK - so we've held the buffer locked across a check for in-core
> inodes we are trying to repair?

That's the intent, anyway. :)

> > > > +	switch (mode & S_IFMT) {
> > > > +	case S_IFIFO:
> > > > +	case S_IFCHR:
> > > > +	case S_IFBLK:
> > > > +	case S_IFSOCK:
> > > > +		/* di_size can't be nonzero for special files */
> > > > +		dip->di_size = 0;
> > > > +		break;
> > > > +	case S_IFREG:
> > > > +		/* Regular files can't be larger than 2^63-1 bytes. */
> > > > +		dip->di_size = cpu_to_be64(size & ~(1ULL << 63));
> > > > +		break;
> > > > +	case S_IFLNK:
> > > > +		/* Catch over- or under-sized symlinks. */
> > > > +		if (size > XFS_SYMLINK_MAXLEN)
> > > > +			dip->di_size = cpu_to_be64(XFS_SYMLINK_MAXLEN);
> > > > +		else if (size == 0)
> > > > +			dip->di_size = cpu_to_be64(1);
> > > 
> > > Not sure this is valid - if the inode is in extent format then a
> > > size of 1 is invalid and means the symlink will point to the
> > > first byte in the data fork, and that could be anything....
> > 
> > I picked these wonky looking formats so that we'd always trigger the
> > higher level repair functions to have a look at the link/dir without
> > blowing up elsewhere in the code if we tried to use them.  Not that we
> > can do much for broken symlinks, but directories could be rebuilt.
> 
> Change the symlink to an inline symlink that points to "zero length
> symlink repaired by online repair" and set the c/mtimes to the
> current time?

Ok.

> > But maybe directories should simply be reset to an empty inline
> > directory, and eventually grow an iflag that will always trigger
> > directory reconstruction (when parent pointers become a thing).
> 
> Yeah, I think if we are going to do anything here we should be
> setting the inodes to valid "empty" state. Or at least comment that
> it's being set to a state that will detected and rebuilt by the
> upcoming fork repair pass.

Ok.

> > > what if the inode failed the fork verifiers rather than the dinode
> > > verifier?
> > 
> > That's coming up in the next patch.  Want me to put in an XXX comment to
> > that effect?
> 
> Not if all you do is remove it in the next patch - better to
> document where we are making changes that the fork rebuild will
> detect and fix up. :P

<nod>

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html