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

[Nikolay Borisov <nborisov@suse.com>]

On 11.09.19 г. 3:39 ч., Qu Wenruo wrote:
> [...]
>>> - 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.
>>
>> This needs an explicit satement that it will only work for non-empty files and directories
> 
> Indeed.
> 
>>>
>>> - 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)
>>> +{
>>> +	struct btrfs_key key;
>>> +	struct btrfs_inode_item iitem;
>>> +	const u32 priv = 0700;
>>> +	bool found = false;
>>> +	u64 ino;
>>> +	u32 imode;
>>> +	int ret = 0;
>>> +
>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +	ino = key.objectid;
>>> +	read_extent_buffer(path->nodes[0], &iitem,
>>> +			btrfs_item_ptr_offset(path->nodes[0], path->slots[0]),
>>> +			sizeof(iitem));
>>> +	/* root inode */
>>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>>> +		imode = S_IFDIR;
>>> +		found = true;
>>> +		goto out;
>>> +	}
>>> +
>>> +	while (1) {
>>> +		struct btrfs_inode_ref *iref;
>>> +		struct extent_buffer *leaf;
>>> +		unsigned long cur;
>>> +		unsigned long end;
>>> +		char namebuf[BTRFS_NAME_LEN] = {0};
>>> +		u64 index;
>>> +		u32 namelen;
>>> +		int slot;
>>> +
>>> +		ret = btrfs_next_item(root, path);
>>> +		if (ret > 0) {
>>> +			/* falls back to rdev check */
>>> +			ret = 0;
>>> +			goto out;
>>> +		}
>>
>> In my testing if an inode is the last one in the leaf and it doesn't have
>> an INODE_REF item then it won't be repaired. But e.g. it can have perfectly
>> valid DIR_ITEM/DIR_INDEX entries which describe this inode as being a file. E.g.
>>
>> 	item 2 key (256 DIR_ITEM 388586943) itemoff 16076 itemsize 35
>> 		location key (260 INODE_ITEM 0) type FILE
>> 		transid 7 data_len 0 name_len 5
>> 		name: file3
>>
>> 	.....
>> 	item 15 key (260 INODE_ITEM 0) itemoff 15184 itemsize 160
>> 		generation 7 transid 7 size 0 nbytes 0
>> 		block group 0 mode 26772225102 links 1 uid 0 gid 0 rdev 0
>> 		sequence 1 flags 0x0(none)
>> 		atime 1568127261.284993602 (2019-09-10 14:54:21)
>> 		ctime 1568127261.284993602 (2019-09-10 14:54:21)
>> 		mtime 1568127261.284993602 (2019-09-10 14:54:21)
>> 		otime 1568127261.284993602 (2019-09-10 14:54:21)
>>
>> 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.

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.


<snip>