[PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Eric Sandeen]

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?

Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
---

diff --git a/repair/dinode.c b/repair/dinode.c
index 84e1d05..3fc6cac 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Olaf Weber]

Eric Sandeen writes:

[...]

> 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


> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>

Acked-By: Olaf Weber <olaf@sgi.com>

> ---

> diff --git a/repair/dinode.c b/repair/dinode.c
> index 84e1d05..3fc6cac 100644
> --- a/repair/dinode.c
> +++ b/repair/dinode.c
> @@ -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.


-- 
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:  olaf@sgi.com

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

Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Eric Sandeen]

Olaf Weber wrote:
> Eric Sandeen writes:
> 
> [...]
> 
>> 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.

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

> Olaf
> 
> 
>> Signed-off-by: Eric Sandeen <sandeen@sandeen.net>
> 
> Acked-By: Olaf Weber <olaf@sgi.com>
> 
>> ---
> 
>> diff --git a/repair/dinode.c b/repair/dinode.c
>> index 84e1d05..3fc6cac 100644
>> --- a/repair/dinode.c
>> +++ b/repair/dinode.c
>> @@ -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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Christoph Hellwig]

Adding this check is certainly better than having nothing, but I would
be much happier if we could do something.

On Sat, Jul 11, 2009 at 12:17:36AM -0500, Eric Sandeen wrote:
> 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.

> 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.

> 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.

> +	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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Eric Sandeen]

Christoph Hellwig wrote:
> Adding this check is certainly better than having nothing, but I would
> be much happier if we could do something.
> 
> On Sat, Jul 11, 2009 at 12:17:36AM -0500, Eric Sandeen wrote:
>> 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.
> 
>> 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.

>> 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.

>> +	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:

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

> 	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
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Eric Sandeen]

Zoran Cvetkovic wrote:
> Hello,
> 
> my today posting with subject: "corrupt dinode but xfs_check and 
> xfs_repair are bit detecting any problem and are fixing nothing"
> seems to be a the same issue.
> 
> I will provide the methadump to whom it my be help full.
> 
> Regards,
> Zoran Cvetkovic

If you like, it's in git at
http://git.kernel.org/?p=fs/xfs/xfsprogs-dev.git;a=summary

and you could make test that on a metadump youreslf.

Otherwise, I could take a look.

-Eric

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

re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format

[Zoran Cvetkovic]

Hello,

my today posting with subject: "corrupt dinode but xfs_check and 
xfs_repair are bit detecting any problem and are fixing nothing"
seems to be a the same issue.

I will provide the methadump to whom it my be help full.

Regards,
Zoran Cvetkovic

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