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>]

On 2019/9/9 下午10:17, Nikolay Borisov wrote:
>
>
> On 5.09.19 г. 10:57 ч., Qu Wenruo wrote:
>> Before this patch, repair_imode_common() can only handle two types of
>> inodes:
>> - Free space cache inodes
>> - ROOT DIR inodes
>>
>> For inodes in subvolume trees, the core complexity is how to determine the
>> correct imode, thus it was not implemented.
>>
>> However there are more reports of incorrect imode in subvolume trees, we
>> need to support such fix.
>>
>> So this patch adds a new function, detect_imode(), to detect imode for
>> inodes in subvolume trees.
>>
>> That function will determine imode by:
>> - Search for INODE_REF
>>   If we have INODE_REF, we will then try to find DIR_ITEM/DIR_INDEX.
>>   As long as one valid DIR_ITEM or DIR_INDEX can be found, we convert
>>   the BTRFS_FT_* to imode, then call it a day.
>>   This should be the most accurate way.
>>
>> - Search for DIR_INDEX/DIR_ITEM
>>   If above search fails, we falls back to locate the DIR_INDEX/DIR_ITEM
>>   just after the INODE_ITEM.
>>   If any can be found, it's definitely a directory.
>>
>> - Search for EXTENT_DATA
>>   If EXTENT_DATA can be found, it's either REG or LNK.
>>   For this case, we default to REG, as user can inspect the file to
>>   determine if it's a file or just a path.
>>
>> - Use rdev to detect BLK/CHR
>>   If all above fails, but INODE_ITEM has non-zero rdev, then it's either
>>   a BLK or CHR file. Then we default to BLK.
>>
>> - Fail out if none of above methods succeeded
>>   No educated guess to make things worse.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/mode-common.c | 130 +++++++++++++++++++++++++++++++++++++++-----
>>  1 file changed, 117 insertions(+), 13 deletions(-)
>>
>> diff --git a/check/mode-common.c b/check/mode-common.c
>> index c0ddc50a1dd0..abea2ceda4c4 100644
>> --- a/check/mode-common.c
>> +++ b/check/mode-common.c
>> @@ -935,6 +935,113 @@ out:
>>  	return ret;
>>  }
>>
>> +static int detect_imode(struct btrfs_root *root, struct btrfs_path *path,
>> +			u32 *imode_ret)
>
> I think the imode is less than u32 so it should be possible to return it
> directly from the function as a positive number and error as negative?

It still doesn't look good enough to me.
Combining data value from error number is not a good idea to me, even
even the range doesn't overlap.

>
>> +{
>> +	struct btrfs_key key;
>> +	struct btrfs_inode_item iitem;
>> +	const u32 priv = 0700;
>
> Having this in a variable doesn't bring more clarity, just use 0700
> directly at the end of the function.

I'm OK with that.

Thanks,
Qu