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

[Eric Sandeen <sandeen@sandeen.net>]

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