xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH, RFC] xfs_repair - clear inodes in incorrect btree format
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Wed, 15 Jul 2009 08:56:24 -0500
Cc: xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <20090715130705.GA27973@xxxxxxxxxxxxx>
References: <4A582070.9040907@xxxxxxxxxxx> <20090715130705.GA27973@xxxxxxxxxxxxx>
User-agent: Thunderbird 2.0.0.22 (Macintosh/20090605)
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;
>       }
> 

<Prev in Thread] Current Thread [Next in Thread>