xfs_repair: validate & fix inode CRCs
PATCH 1/1 xfs_repair: validate & fix inode CRCs
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.
---
forgive the gratuitous big honkin' comment line, but process_dinode_int is so long, I thought the visual delimiter was useful. ;)
repair/dinode.c +24 -0
@@ -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" )); + } + } +/* * clear the next unlinked field if necessary on a good * inode only during phase 4 -- when checking for inodes
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 38 lines · Eric Sandeen
xfs_repairdoesn'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 usingxfs_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 <[email protected]>---
forgive the gratuitous big honkin' comment line, but
process_dinode_intis so long, I thought the visual delimiter was useful. ;)repair/dinode.c +14
-0@@ -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,
repair/dinode.c _("bad (negative) size %" PRId64 " on inode %" PRIu64 "\n"), · line 2550
Quoted 10 lines
repair/dinode.c +10
-0+ 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
Quoted 9 lines
repair/dinode.c +0
-0/* * clear the next unlinked field if necessary on a good * inode only during phase 4 -- when checking for inodes _______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Brian Foster
So we verify each inode first in
process_inode_chunk()and then follow on withprocess_dinode(). There's a comment further up inprocess_dinode_int()that indicates we explicitly do not check the crc at that point, presumably consideringverify_mode. I only see one call to each ofverify_inode()andprocess_dinode()(in that order). The otherprocess_dinode_int()caller isverify_uncertain_dinode(), which looks like it occurs ultimately fromprocess_uncertain_aginodes()in phase 3.
Quoted 6 lines
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..?
Ugh, I am not sure how I missed that we could have already modified it. Ok, that throws the whole thing out, I'll take another look.
Thanks,
-Eric
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs