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

Eric Sandeen
2013-09-11T02:27:58Z

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.