xfs
[Top] [All Lists]

Re: [PATCH V2] xfs_repair: test for bad level in dir2 node

To: Mark Tinguely <tinguely@xxxxxxx>
Subject: Re: [PATCH V2] xfs_repair: test for bad level in dir2 node
From: Eric Sandeen <sandeen@xxxxxxxxxxx>
Date: Tue, 10 Sep 2013 21:27:44 -0500
Cc: "'linux-xfs@xxxxxxxxxxx'" <linux-xfs@xxxxxxxxxxx>
Delivered-to: linux-xfs@xxxxxxxxxxx
In-reply-to: <522F5ED7.80005@xxxxxxx>
References: <52274F96.2010702@xxxxxxxxxxx> <522F4001.8010104@xxxxxxxxxxx> <522F4C26.2080106@xxxxxxx> <522F55B9.3030509@xxxxxxxxxxx> <522F5ED7.80005@xxxxxxx>
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:17.0) Gecko/20130801 Thunderbird/17.0.8
On 9/10/13 1:03 PM, Mark Tinguely wrote:
>> 1) The block magic is LEAFN.  If so, we stop.  We warn if it's not root 
>> level (but don't fix?  Maybe that's a bug for another patch?)
> 
> Yes. We do not loop if "i == 1", so another LEAF should not be found.



>> 2) The block magic is NODE.  If not, we error out.
> 
> Yes.
> 
>> and as I showed above:
>> 3) The level matches each level we're at in the loop.
>>
>> So:
>>
>> Any block which isn't LEAFN or NODE is caught prior to the (i == -1) block.
> 
> Yes must be a NODE.
> 
>> Any block which has a level that doesn't match is caught on the else of the 
>> (i == -1) block.
> 
> Yes, and "i" has to be larger than 1 because of the loop. Which I did not 
> catch before.
>>
>> And those are the only 2 valid types here.
>>
>> What case is missing?
>>
>> -eric
>>
> 
> With loop condition of "i > 1" then it cannot miss what I first thought was 
> being missed, but the level of 1 being a leaf is not checked.

But I don't think that's right, is it?  level[0] is leaf; level[1] is a node, 
right?

Argh.  Now I'm more confused; xfs_check has:

        case XFS_DA_NODE_MAGIC:
                node = iocur_top->data;
                xfs_da3_node_hdr_from_disk(&nodehdr, node);
                if (nodehdr.level < 1 || nodehdr.level > XFS_DA_NODE_MAXDEPTH) {
                        if (!sflag || v)
                                dbprintf(_("bad node block level %d for dir ino 
"
                                         "%lld block %d\n"),
                                        nodehdr.level, id->ino,
                                        dabno);
                        error++;

so nodehdr.level == XFS_DA_NODE_MAXDEPTH is valid there (and level == 1 is a 
valid
node), but repair says:

                        if (i >= XFS_DA_NODE_MAXDEPTH) {
                                do_warn(
_("bad header depth for directory inode %" PRIu64 "\n"),
                                        da_cursor->ino);

so nodehdr.level == XFS_DA_NODE_MAXDEPTH is *not* valid here.

indices and counters and depths, oh my.  I need to back up and remember what's 
what.  :(

... Still not sure any of this invalidates my targeted fix - although I should 
just 
make it a one-liner and do:

                if (i == -1) {
                        i = da_cursor->active = nodehdr.level;
-                       if (i >= XFS_DA_NODE_MAXDEPTH) {
+                       if (i < 1 || i >= XFS_DA_NODE_MAXDEPTH) {
                                do_warn(
 _("bad header depth for directory inode %" PRIu64 "\n"),

-Eric

> --Mark.

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