Re: [PATCH v2 3/6] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

[...]
>>> I have intentionally deleted INODE_REF too see what's happening. Is this intended?
>>
>> Yes, completely intended.
>>
>> For this case, you need to iterate through the whole tree to locate the
>> DIR_INDEX to fix, which is not really possible with current code base,
>> which only search based on the INODE, not the DIR_INDEX/DIR_ITEM from
>> its parent dir.
>>
>> Furthermore, didn't you mention that if we don't have confident about
>> the imode, then we should fail out instead of using REG as default?
>
> I did, what I supposed could happen here is if we can't find an
> INODE_REF then do a search for DIR_INDEX/DIR_ITEM since those items also
> contain the type of the inode they are pointing to. Fixing really boils
> down to exploiting redundancy in the on-disk metadata to repair existing
> items. Here comes the question, of course, what to do if we don't have
> an INODE_REF and DIR_INDEX/DIR_ITEM are broken. I guess it's a judgement
> call, currently you decided that inode_ref will be the source of
> information. I'm fine with that I was merely pointing there is more we
> can do. Of course we need to weigh the pros/cons between code complexity
> and the returns we get.

BTW, the kinda of "inconsistency" between different judgement calls is
somewhat causing the problem you see in some more complex repair situation.

To me and the old Fujitsu guys, what we (at least used to) believe is,
if we have any chance to recover, then try our best. (just like what I
did when we can't find inode ref)

But if we have redundant info, like the INODE_REF/DIR_INDEX/DIR_ITEM, we
go democracy, two valid items then recovery, one valid item, then
discard it.

If we have a very consistent policy, like anything wrong the discard the
bad part even it will cause data loss.
The recovery will be much easier and consistent.
In this imode case, we don't "recover" the inode, but remove it
completely, and if the INODE_REF/DIR_INDEX/DIR_ITEM repair follows the
same standard, a lot of things will be much easier to do.
(Now I'm regretting the call I did for the INODE_REF/DIR_INDEX/DIR_ITEM
repair code)

But I guess that's the ultimate judgement call made way beyond I joined
the btrfs-progs development (see the extent repair code and bad key
order repair), thus not that easy to change without a large rework...

>
> Just that I will advise to make it explicit in the changelog that you
> made a judgement call to utilize INODE_REF as the starting point of
> doing imode repair but not necessarily the only one.

No problem, I'd add more comment and the disadvantage of the current
behavior.

Thanks,
Qu

>
>
> <snip>
>