Re: [PATCH] xfs_repair: validate & fix inode CRCs

[Brian Foster <bfoster@redhat.com>]

On Tue, Sep 16, 2014 at 04:30:41PM -0500, Eric Sandeen wrote:
> xfs_repair doesn't ever check an inode's CRC, so it never repairs
> them.  If the root inode or realtime inodes have bad crcs, the
> fs won't even mount and can't be fixed (without using xfs_db).
> 
> It's fairly straightforward to just test the inode CRC before
> we do any other checking or modification of the inode, once we
> get past the "verify only" phase of process_dinode_int();
> just mark it dirty if it's wrong and needs to be re-written.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> forgive the gratuitous big honkin' comment line, but
> process_dinode_int is so long, I thought the visual delimiter was
> useful.  ;)
> 
> diff --git a/repair/dinode.c b/repair/dinode.c
> index 8891e84..27c0da6 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -2524,6 +2524,30 @@ _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"),
>  	if (verify_mode)
>  		return retval;
>  
> +	/* =========== END OF VERIFY MODE =================== */
> +
> +	/*
> +	 * We'd really like to know if the CRC is bad before we
> +	 * go fixing anything; that way we have some hint about
> +	 * bit-rot vs bugs.  Also, any changes will invalidate the
> +	 * existing CRC, so this is the only valid point to test it.
> +	 *
> +	 * Of course if we make any modifications after this, the
> +	 * inode gets rewritten, and CRC is updated automagically.
> +	 */
> +	if (xfs_sb_version_hascrc(&mp->m_sb)) {
> +		ASSERT(!verify_mode);
> +		if(!xfs_verify_cksum((char *)dino, mp->m_sb.sb_inodesize,
> +                		XFS_DINODE_CRC_OFF)) {
> +			do_warn(_("bad CRC for inode %" PRIu64), lino);
> +			if (!no_modify) {
> +				do_warn(_(", will rewrite\n"));
> +				*dirty = 1;
> +			} else
> +				do_warn(_(", would rewrite\n"));
> +		}
> +	}
> +

So we verify each inode first in process_inode_chunk() and then follow
on with process_dinode(). There's a comment further up in
process_dinode_int() that indicates we explicitly do not check the crc
at that point, presumably considering verify_mode. I only see one call
to each of verify_inode() and process_dinode() (in that order). The
other process_dinode_int() caller is verify_uncertain_dinode(), which
looks like it occurs ultimately from process_uncertain_aginodes() in
phase 3.

I suppose that logic makes sense, but it's not totally clear tbh. We do
fix up the crc in the caller if the inode is marked dirty. It also seems
like it's possible to modify the inode before this point where we check
the crc. Given that, it seems like we could just add an "if
(!verify_mode)" hunk to the preexisting hascrc() hunk further up in the
function..?

I don't really have a clear beat on this at the moment. I think what I'm
getting after is that right now if we see the new crc message, it isn't
necessarily clear if that is because repair changed something or it is
legitimately wrong on-disk, depending on how the inode might be
corrupted or if the inode is free (do we care about crc's in that case?
it looks like we miss that as well). It would be nice if we had some
kind of consistent rule for this scenario, such as "always print the
error if the crc is wrong" (e.g., before we make any modifications) or
"only print the error if the crc is the only thing wrong" (e.g., we've
already verified the inode and the crc itself is wrong). Either way it
seems inappropriate to print a crc error that might be caused by repair
itself.

Brian

>  	/*
>  	 * clear the next unlinked field if necessary on a good
>  	 * inode only during phase 4 -- when checking for inodes
> 
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs