xfs_repair - clear inodes in incorrect btree format
RFC PATCH 1/1 xfs_repair - clear inodes in incorrect btree format
See also RH bug #510823:
https://bugzilla.redhat.com/show_bug.cgi?id=510823
This check in xfs_iformat_btree() tripped:
/*
* blow out if -- fork has less extents than can fit in
* fork (fork shouldn't be a btree format), root btree
* block has more records than can fit into the fork,
* or the number of extents is greater than the number of
* blocks.
*/
leading to:
Jul 10 23:22:45 hermes kernel: Filesystem "dm-11": corrupt inode 2339503222 (btree). Unmount and run xfs_repair. Jul 10 23:22:45 hermes kernel: Filesystem "dm-11": XFS internal error
xfs_iformat_btree at line 625 of file fs/xfs/xfs_inode.c.
but repair finds nothing at all. xfs_check, however, does flag the inodes as problematic:
extent count for ino 2339503222 data fork too low (6) for file format
So I copied the xfs_check test into xfs_repair, and voila, it clears these inodes.
But questions remain...
1) How'd it get into this state? ... but maybe more importantly...
2) Should these really get cleared? It's possibly a sane extent list, it's just that it -could- be in extents rather than btree format...
3) By the same token, should the kernel really be choking on it?
---
repair/dinode.c +8 -0
@@ -1280,6 +1280,14 @@ process_btinode( last_key = cursor.level[level-1 ].first_key; } } +if (*nex <=XFS_DFORK_SIZE (dip, mp, whichfork) /sizeof (xfs_bmbt_rec_t)) { +do_warn (_ ("extent count for ino %lld %s fork too low " +"(%d) for file format\n" ), + lino, + whichfork ==XFS_DATA_FORK ?_ ("data" ) :_ ("attr" ), + *nex); +return (1 ); + }/* * Check that the last child block's forward sibling pointer * is NULL.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Eric Sandeen writes:
[...]
Quoted 6 lines
But questions remain...
1) How'd it get into this state? ... but maybe more importantly...
2) Should these really get cleared? It's possibly a sane extent list, it's just that it -could- be in extents rather than btree format...
3) By the same token, should the kernel really be choking on it?
It is not clear to me yet how you could get into this state, but this is clearly an invariant the kernel actively maintains.
If the kernel "just" missed the underflow and kept the extents in btree format, then I don't see an apriori reason why the extent list as such would be invalid (as opposed to inefficiently stored). If that's the primary model for getting into this state, then the file contents can be rescued and kernel-side the event should be survivable.
But if the kernel tried to convert, failed, and didn't properly detect failure... Without having a good answer for (1) I find it hard to convince myself that to be more forgiving wrt to (2) and (3) is safe.
Olaf
Quoted 1 line
Signed-off-by Eric Sandeen <[email protected]>
Quoted 21 lines
---
repair/dinode.c +8
-0@@ -1280,6 +1280,14 @@ process_btinode( last_key = cursor.level[level-1 ].first_key; } } +if (*nex <=XFS_DFORK_SIZE (dip, mp, whichfork) /sizeof (xfs_bmbt_rec_t)) { +do_warn (_ ("extent count for ino %lld %s fork too low " +"(%d) for file format\n" ), + lino, + whichfork ==XFS_DATA_FORK ?_ ("data" ) :_ ("attr" ), + *nex); +return (1 ); + }/* * Check that the last child block's forward sibling pointer * is NULL.
Signature · 9 lines
Olaf Weber SGI Phone: +31(0)30-6696752
Veldzigt 2b Fax: +31(0)30-6696799
Technical Lead 3454 PW de Meern Vnet: 955-7151
Storage Software The Netherlands Email: [email protected]
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfsQuoted 21 lines · Olaf Weber
Eric Sandeen writes:
[...]
Quoted 6 lines
But questions remain...
1) How'd it get into this state? ... but maybe more importantly...
2) Should these really get cleared? It's possibly a sane extent list, it's just that it -could- be in extents rather than btree format...
3) By the same token, should the kernel really be choking on it?It is not clear to me yet how you could get into this state, but this is clearly an invariant the kernel actively maintains.
If the kernel "just" missed the underflow and kept the extents in btree format, then I don't see an apriori reason why the extent list as such would be invalid (as opposed to inefficiently stored). If that's the primary model for getting into this state, then the file contents can be rescued and kernel-side the event should be survivable.
Quoted 3 lines
But if the kernel tried to convert, failed, and didn't properly detect failure... Without having a good answer for (1) I find it hard to convince myself that to be more forgiving wrt to (2) and (3) is safe.
True.
Though FWIW, in this case, the user edited out the kernel check, mounted, and successfully copied off the files before running xfs_repair and nuking it with this patch.
So I guess bonus points would be teaching repair to fix up the problem if the list seems valid ... and maybe even the same kernelside....
Thanks,
-Eric
Quoted 20 lines
Olaf
Quoted 1 line
Signed-off-by Eric Sandeen <[email protected]>Acked-by Olaf Weber <[email protected]>Quoted 13 lines
---
repair/dinode.c +3
-0@@ -1280,6 +1280,14 @@ process_btinode( last_key = cursor.level[level-1 ].first_key; } } +if (*nex <=XFS_DFORK_SIZE (dip, mp, whichfork) /sizeof (xfs_bmbt_rec_t)) { + do_warn(_("extent count for ino %lld %s fork too low " + "(%d) for file format\n"),
Quoted 10 lines
repair/dinode.c process_btinode( · line 1293
Quoted 8 lines
repair/dinode.c +5
-0+ lino, + whichfork ==XFS_DATA_FORK ?_ ("data" ) :_ ("attr" ), + *nex); +return (1 ); + }/* * Check that the last child block's forward sibling pointer * is NULL.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Adding this check is certainly better than having nothing, but I would be much happier if we could do something.
Quoted 1 line · Eric Sandeen
1) How'd it get into this state? ... but maybe more importantly...
End of last year lachlan had case that looked a bit like this where we had problems resetting the fork state.
Quoted 2 lines
2) Should these really get cleared? It's possibly a sane extent list, it's just that it -could- be in extents rather than btree format...
That is indeed the the most likely case. Do you still have a metadump with this problem around? We should probably sanity-check for a valid looking extent format inode and then process it as such.
Quoted 1 line
3) By the same token, should the kernel really be choking on it?
Well, not choking could cause all kinds of harm by treating it as a btree inode while it's not. We could try to apply a very careful variant of 2) above, but I'd really rather leave that kind of thing to repair.
repair/dinode.c process_btinode( · line 1290
Quoted 8 lines
repair/dinode.c +8
-0+if (*nex <=XFS_DFORK_SIZE (dip, mp, whichfork) /sizeof (xfs_bmbt_rec_t)) { +do_warn (_ ("extent count for ino %lld %s fork too low " +"(%d) for file format\n" ), + lino, + whichfork ==XFS_DATA_FORK ?_ ("data" ) :_ ("attr" ), + *nex); +return (1 ); + }
Well, you'll get my ok in the sense of this looks good and better than nothing, but I'd really prefer a real fixup for this issues. Also the code above looks a bit unreadable, why not:
if (*nex <= XFS_DFORK_SIZE(dip, mp, whichfork) /
sizeof(xfs_bmbt_rec_t)) {
do_warn(
_("extent count for ino %lld %s fork too low (%d) for file format\n"),
lino, forkname, *nex);
return 1;
}_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Christoph Hellwig
Adding this check is certainly better than having nothing, but I would be much happier if we could do something.
Quoted 1 line · Eric Sandeen
1) How'd it get into this state? ... but maybe more importantly...
End of last year lachlan had case that looked a bit like this where we had problems resetting the fork state.
Quoted 6 lines
Quoted 2 lines
2) Should these really get cleared? It's possibly a sane extent list, it's just that it -could- be in extents rather than btree format...
That is indeed the the most likely case. Do you still have a metadump with this problem around? We should probably sanity-check for a valid looking extent format inode and then process it as such.
yep I do... and the user was able to perfectly copy off the files by disabling the kernel check, FWIW. So in this case it really was OK.
Quoted 6 lines
Quoted 1 line
3) By the same token, should the kernel really be choking on it?
Well, not choking could cause all kinds of harm by treating it as a btree inode while it's not. We could try to apply a very careful variant of 2) above, but I'd really rather leave that kind of thing to repair.
Yep, probably best.
Quoted 4 lines
Quoted 4 lines
repair/dinode.c +4
-0+if (*nex <=XFS_DFORK_SIZE (dip, mp, whichfork) /sizeof (xfs_bmbt_rec_t)) { + do_warn(_("extent count for ino %lld %s fork too low " + "(%d) for file format\n"), + lino,
Quoted 8 lines
repair/dinode.c process_btinode( · line 1290
Quoted 4 lines
repair/dinode.c +4
-0+ whichfork ==XFS_DATA_FORK ?_ ("data" ) :_ ("attr" ), + *nex); +return (1 ); + }Well, you'll get my ok in the sense of this looks good and better than nothing, but I'd really prefer a real fixup for this issues. Also the code above looks a bit unreadable, why not:
I guess I tend to prefer a real fixup too, if possible; I suppose there's existing infrastructure to check it as a btree inode, and hopefully to move it into extents as well.
FWIW I just copied the check above from xfs_check ;)
Sure, below formatting is better.
thanks,
-Eric
Quoted 8 lines
if (*nex <= XFS_DFORK_SIZE(dip, mp, whichfork) /
sizeof(xfs_bmbt_rec_t)) {
do_warn(
_("extent count for ino %lld %s fork too low (%d) for file format\n"),
lino, forkname, *nex);
return 1;
}_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs