[1/4] btrfs-progs: check/common: Make repair_imode_common() to handle inodes in subvolume trees
diff mbox series

Message ID 20190903082407.13927-2-wqu@suse.com
State New
Headers show
Series
  • btrfs-progs: check: Repair invalid inode mode in subvolume trees
Related show

Commit Message

Qu Wenruo Sept. 3, 2019, 8:24 a.m. UTC
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 problem 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 (or call it
educated guess) imode for inodes in subvolume trees.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
 1 file changed, 83 insertions(+), 13 deletions(-)

Comments

Nikolay Borisov Sept. 4, 2019, 7:37 a.m. UTC | #1
On 3.09.19 г. 11:24 ч., 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 problem 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 (or call it
> educated guess) imode for inodes in subvolume trees.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>  1 file changed, 83 insertions(+), 13 deletions(-)
> 
> diff --git a/check/mode-common.c b/check/mode-common.c
> index 195b6efaa7aa..807d7daf98a6 100644
> --- a/check/mode-common.c
> +++ b/check/mode-common.c
> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>  	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;
> +	u64 ino;
> +	u32 imode = S_IFREG;
> +	int ret = 0;
> +
> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +	ino = key.objectid;
> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
> +			       struct btrfs_inode_item);
> +
> +	/*
> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
> +	 * to BLK.
> +	 */
> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
> +		imode = S_IFBLK;
> +		goto out;
> +	}
> +
> +	/* root inode */
> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
> +		imode = S_IFDIR;
> +		goto out;
> +	}
> +
> +	while (1) {
> +		ret = btrfs_next_item(root, path);
> +		if (ret > 0) {
> +			/* No determining result found, falls back to REG */
> +			ret = 0;
> +			goto out;
> +		}
> +		if (ret < 0)
> +			goto out;
> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
> +		if (key.objectid != ino)
> +			goto out;
> +
> +		/*
> +		 * We ignore some types to make life easier:
> +		 * - INODE_REF
> +		 *   We need to do a full tree search, which can fail for
> +		 *   corrupted fs, but not worthy compared to other easier
> +		 *   to determine types.
> +		 * - XATTR
> +		 *   Both REG and DIR can have xattr, so not useful
> +		 */
> +		switch (key.type) {
> +		case BTRFS_DIR_ITEM_KEY:
> +		case BTRFS_DIR_INDEX_KEY:
> +			imode = S_IFDIR;
> +			goto out;
> +		case BTRFS_EXTENT_DATA_KEY:
> +			/*
> +			 * Both REG and LINK could have EXTENT_DATA.
> +			 * We just fall back to REG as user can inspect the
> +			 * content.
> +			 */
> +			imode = S_IFREG;
> +			goto out;
> +		}
> +	}
> +
> +out:
> +	/* Set default value even when something wrong happened */
> +	*imode_ret = (imode | priv);
> +	return ret;
> +}
> +
>  /*
>   * Reset the inode mode of the inode specified by @path.
>   *
> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>  	u32 imode;
>  	int ret;
>  
> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
> -		error(
> -		"repair inode mode outside of root tree is not supported yet");
> -		return -ENOTTY;
> -	}
>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
> -	    !is_fstree(key.objectid)) {
> -		error("unsupported ino %llu", key.objectid);
> -		return -ENOTTY;
> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
> +		/* In root tree we only have two possible imode */
> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
> +			imode = S_IFDIR | 0755;
> +		else
> +			imode = S_IFREG | 0600;
> +	} else {
> +		detect_imode(root, path, &imode);
> +		/* Ignore error returned, just use the default value returned */

Is this safe enough though? What if due to an error a directory is
corrected to be file? Let's not forget the context we are oprating here
- a broken fs so it's possible (if not likely) that any of the search
functions inside detect_imode could return negative error value. OTOH
I'd expect the transaction commit to fail if that's the case e.g. faulty
device.

>  	}
> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
> -		imode = 040755;
> -	else
> -		imode = 0100600;
>  
>  	trans = btrfs_start_transaction(root, 1);
>  	if (IS_ERR(trans)) {
>
Qu Wenruo Sept. 4, 2019, 8:30 a.m. UTC | #2
On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>
>
> On 3.09.19 г. 11:24 ч., 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 problem 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 (or call it
>> educated guess) imode for inodes in subvolume trees.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>
>> diff --git a/check/mode-common.c b/check/mode-common.c
>> index 195b6efaa7aa..807d7daf98a6 100644
>> --- a/check/mode-common.c
>> +++ b/check/mode-common.c
>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>  	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;
>> +	u64 ino;
>> +	u32 imode = S_IFREG;
>> +	int ret = 0;
>> +
>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +	ino = key.objectid;
>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>> +			       struct btrfs_inode_item);
>> +
>> +	/*
>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>> +	 * to BLK.
>> +	 */
>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>> +		imode = S_IFBLK;
>> +		goto out;
>> +	}
>> +
>> +	/* root inode */
>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>> +		imode = S_IFDIR;
>> +		goto out;
>> +	}
>> +
>> +	while (1) {
>> +		ret = btrfs_next_item(root, path);
>> +		if (ret > 0) {
>> +			/* No determining result found, falls back to REG */
>> +			ret = 0;
>> +			goto out;
>> +		}
>> +		if (ret < 0)
>> +			goto out;
>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>> +		if (key.objectid != ino)
>> +			goto out;
>> +
>> +		/*
>> +		 * We ignore some types to make life easier:
>> +		 * - INODE_REF
>> +		 *   We need to do a full tree search, which can fail for
>> +		 *   corrupted fs, but not worthy compared to other easier
>> +		 *   to determine types.
>> +		 * - XATTR
>> +		 *   Both REG and DIR can have xattr, so not useful
>> +		 */
>> +		switch (key.type) {
>> +		case BTRFS_DIR_ITEM_KEY:
>> +		case BTRFS_DIR_INDEX_KEY:
>> +			imode = S_IFDIR;
>> +			goto out;
>> +		case BTRFS_EXTENT_DATA_KEY:
>> +			/*
>> +			 * Both REG and LINK could have EXTENT_DATA.
>> +			 * We just fall back to REG as user can inspect the
>> +			 * content.
>> +			 */
>> +			imode = S_IFREG;
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	/* Set default value even when something wrong happened */
>> +	*imode_ret = (imode | priv);
>> +	return ret;
>> +}
>> +
>>  /*
>>   * Reset the inode mode of the inode specified by @path.
>>   *
>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>  	u32 imode;
>>  	int ret;
>>
>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>> -		error(
>> -		"repair inode mode outside of root tree is not supported yet");
>> -		return -ENOTTY;
>> -	}
>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>> -	    !is_fstree(key.objectid)) {
>> -		error("unsupported ino %llu", key.objectid);
>> -		return -ENOTTY;
>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>> +		/* In root tree we only have two possible imode */
>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>> +			imode = S_IFDIR | 0755;
>> +		else
>> +			imode = S_IFREG | 0600;
>> +	} else {
>> +		detect_imode(root, path, &imode);
>> +		/* Ignore error returned, just use the default value returned */
>
> Is this safe enough though?

It depends. But I'd say if we failed in detect_imode(), then it doesn't
matter whatever the type we're putting here.

> What if due to an error a directory is
> corrected to be file?

If a failure happens, it means btrfs-progs fails to read the next leaf.
Then it really doesn't make much sense whatever the type is.

But I get your point, indeed we should error out without trying out to
fix the inode.

I'll change this behavior in next version.

Thanks,
Qu

> Let's not forget the context we are oprating here
> - a broken fs so it's possible (if not likely) that any of the search
> functions inside detect_imode could return negative error value. OTOH
> I'd expect the transaction commit to fail if that's the case e.g. faulty
> device.
>
>>  	}
>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>> -		imode = 040755;
>> -	else
>> -		imode = 0100600;
>>
>>  	trans = btrfs_start_transaction(root, 1);
>>  	if (IS_ERR(trans)) {
>>
Nikolay Borisov Sept. 4, 2019, 10:53 a.m. UTC | #3
On 4.09.19 г. 11:30 ч., Qu Wenruo wrote:
> 
> 
> On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>>
>>
>> On 3.09.19 г. 11:24 ч., 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 problem 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 (or call it
>>> educated guess) imode for inodes in subvolume trees.
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/check/mode-common.c b/check/mode-common.c
>>> index 195b6efaa7aa..807d7daf98a6 100644
>>> --- a/check/mode-common.c
>>> +++ b/check/mode-common.c
>>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>  	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;
>>> +	u64 ino;
>>> +	u32 imode = S_IFREG;
>>> +	int ret = 0;
>>> +
>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +	ino = key.objectid;
>>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>> +			       struct btrfs_inode_item);
>>> +
>>> +	/*
>>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>>> +	 * to BLK.
>>> +	 */
>>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>>> +		imode = S_IFBLK;
>>> +		goto out;
>>> +	}
>>> +
>>> +	/* root inode */
>>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>>> +		imode = S_IFDIR;
>>> +		goto out;
>>> +	}
>>> +
>>> +	while (1) {
>>> +		ret = btrfs_next_item(root, path);
>>> +		if (ret > 0) {
>>> +			/* No determining result found, falls back to REG */
>>> +			ret = 0;
>>> +			goto out;
>>> +		}
>>> +		if (ret < 0)
>>> +			goto out;
>>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>> +		if (key.objectid != ino)
>>> +			goto out;
>>> +
>>> +		/*
>>> +		 * We ignore some types to make life easier:
>>> +		 * - INODE_REF
>>> +		 *   We need to do a full tree search, which can fail for
>>> +		 *   corrupted fs, but not worthy compared to other easier
>>> +		 *   to determine types.
>>> +		 * - XATTR
>>> +		 *   Both REG and DIR can have xattr, so not useful
>>> +		 */
>>> +		switch (key.type) {
>>> +		case BTRFS_DIR_ITEM_KEY:
>>> +		case BTRFS_DIR_INDEX_KEY:
>>> +			imode = S_IFDIR;
>>> +			goto out;
>>> +		case BTRFS_EXTENT_DATA_KEY:
>>> +			/*
>>> +			 * Both REG and LINK could have EXTENT_DATA.
>>> +			 * We just fall back to REG as user can inspect the
>>> +			 * content.
>>> +			 */
>>> +			imode = S_IFREG;
>>> +			goto out;
>>> +		}
>>> +	}
>>> +
>>> +out:
>>> +	/* Set default value even when something wrong happened */
>>> +	*imode_ret = (imode | priv);
>>> +	return ret;
>>> +}
>>> +
>>>  /*
>>>   * Reset the inode mode of the inode specified by @path.
>>>   *
>>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>>  	u32 imode;
>>>  	int ret;
>>>
>>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>>> -		error(
>>> -		"repair inode mode outside of root tree is not supported yet");
>>> -		return -ENOTTY;
>>> -	}
>>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>>> -	    !is_fstree(key.objectid)) {
>>> -		error("unsupported ino %llu", key.objectid);
>>> -		return -ENOTTY;
>>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>>> +		/* In root tree we only have two possible imode */
>>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>>> +			imode = S_IFDIR | 0755;
>>> +		else
>>> +			imode = S_IFREG | 0600;
>>> +	} else {
>>> +		detect_imode(root, path, &imode);
>>> +		/* Ignore error returned, just use the default value returned */
>>
>> Is this safe enough though?
> 
> It depends. But I'd say if we failed in detect_imode(), then it doesn't
> matter whatever the type we're putting here.
> 
>> What if due to an error a directory is
>> corrected to be file?
> 
> If a failure happens, it means btrfs-progs fails to read the next leaf.
> Then it really doesn't make much sense whatever the type is.
> 
> But I get your point, indeed we should error out without trying out to
> fix the inode.
> 
> I'll change this behavior in next version.

IMO the goal of btrfs-progs should be to make a broken filesystem better
and not replace one type of breakage with potentially another. In such
cases we ought to break out gracefully.

> 
> Thanks,
> Qu
> 
>> Let's not forget the context we are oprating here
>> - a broken fs so it's possible (if not likely) that any of the search
>> functions inside detect_imode could return negative error value. OTOH
>> I'd expect the transaction commit to fail if that's the case e.g. faulty
>> device.
>>
>>>  	}
>>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>>> -		imode = 040755;
>>> -	else
>>> -		imode = 0100600;
>>>
>>>  	trans = btrfs_start_transaction(root, 1);
>>>  	if (IS_ERR(trans)) {
>>>
>
Qu Wenruo Sept. 4, 2019, 12:12 p.m. UTC | #4
On 2019/9/4 下午6:53, Nikolay Borisov wrote:
>
>
> On 4.09.19 г. 11:30 ч., Qu Wenruo wrote:
>>
>>
>> On 2019/9/4 下午3:37, Nikolay Borisov wrote:
>>>
>>>
>>> On 3.09.19 г. 11:24 ч., 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 problem 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 (or call it
>>>> educated guess) imode for inodes in subvolume trees.
>>>>
>>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>>> ---
>>>>  check/mode-common.c | 96 +++++++++++++++++++++++++++++++++++++++------
>>>>  1 file changed, 83 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/check/mode-common.c b/check/mode-common.c
>>>> index 195b6efaa7aa..807d7daf98a6 100644
>>>> --- a/check/mode-common.c
>>>> +++ b/check/mode-common.c
>>>> @@ -836,6 +836,80 @@ int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
>>>>  	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;
>>>> +	u64 ino;
>>>> +	u32 imode = S_IFREG;
>>>> +	int ret = 0;
>>>> +
>>>> +	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>> +	ino = key.objectid;
>>>> +	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
>>>> +			       struct btrfs_inode_item);
>>>> +
>>>> +	/*
>>>> +	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>>>> +	 * to BLK.
>>>> +	 */
>>>> +	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
>>>> +		imode = S_IFBLK;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	/* root inode */
>>>> +	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
>>>> +		imode = S_IFDIR;
>>>> +		goto out;
>>>> +	}
>>>> +
>>>> +	while (1) {
>>>> +		ret = btrfs_next_item(root, path);
>>>> +		if (ret > 0) {
>>>> +			/* No determining result found, falls back to REG */
>>>> +			ret = 0;
>>>> +			goto out;
>>>> +		}
>>>> +		if (ret < 0)
>>>> +			goto out;
>>>> +		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>> +		if (key.objectid != ino)
>>>> +			goto out;
>>>> +
>>>> +		/*
>>>> +		 * We ignore some types to make life easier:
>>>> +		 * - INODE_REF
>>>> +		 *   We need to do a full tree search, which can fail for
>>>> +		 *   corrupted fs, but not worthy compared to other easier
>>>> +		 *   to determine types.
>>>> +		 * - XATTR
>>>> +		 *   Both REG and DIR can have xattr, so not useful
>>>> +		 */
>>>> +		switch (key.type) {
>>>> +		case BTRFS_DIR_ITEM_KEY:
>>>> +		case BTRFS_DIR_INDEX_KEY:
>>>> +			imode = S_IFDIR;
>>>> +			goto out;
>>>> +		case BTRFS_EXTENT_DATA_KEY:
>>>> +			/*
>>>> +			 * Both REG and LINK could have EXTENT_DATA.
>>>> +			 * We just fall back to REG as user can inspect the
>>>> +			 * content.
>>>> +			 */
>>>> +			imode = S_IFREG;
>>>> +			goto out;
>>>> +		}
>>>> +	}
>>>> +
>>>> +out:
>>>> +	/* Set default value even when something wrong happened */
>>>> +	*imode_ret = (imode | priv);
>>>> +	return ret;
>>>> +}
>>>> +
>>>>  /*
>>>>   * Reset the inode mode of the inode specified by @path.
>>>>   *
>>>> @@ -852,22 +926,18 @@ int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
>>>>  	u32 imode;
>>>>  	int ret;
>>>>
>>>> -	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
>>>> -		error(
>>>> -		"repair inode mode outside of root tree is not supported yet");
>>>> -		return -ENOTTY;
>>>> -	}
>>>>  	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
>>>>  	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
>>>> -	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
>>>> -	    !is_fstree(key.objectid)) {
>>>> -		error("unsupported ino %llu", key.objectid);
>>>> -		return -ENOTTY;
>>>> +	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
>>>> +		/* In root tree we only have two possible imode */
>>>> +		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
>>>> +			imode = S_IFDIR | 0755;
>>>> +		else
>>>> +			imode = S_IFREG | 0600;
>>>> +	} else {
>>>> +		detect_imode(root, path, &imode);
>>>> +		/* Ignore error returned, just use the default value returned */
>>>
>>> Is this safe enough though?
>>
>> It depends. But I'd say if we failed in detect_imode(), then it doesn't
>> matter whatever the type we're putting here.
>>
>>> What if due to an error a directory is
>>> corrected to be file?
>>
>> If a failure happens, it means btrfs-progs fails to read the next leaf.
>> Then it really doesn't make much sense whatever the type is.
>>
>> But I get your point, indeed we should error out without trying out to
>> fix the inode.
>>
>> I'll change this behavior in next version.
>
> IMO the goal of btrfs-progs should be to make a broken filesystem better
> and not replace one type of breakage with potentially another. In such
> cases we ought to break out gracefully.

Totally agreed.

BTW, I'll also implement the INODE_REF lookup method to provide the most
accurate way to recover.

The planned recovery would be:
- INODE_REF lookup
  If everything goes fine, that's it and call it a day.
  If it failed, try the rest.
- INODE rdev lookup
  For this case, either BLK or CHR, but it doesn't really matter nor
  would cause problem.
- DIR_INDEX/ITEM lookup
- EXTENT_DATA lookup
  Both REG and LNK would be possible but falls back to REG won't cause
  further problem.

Thanks,
Qu
>
>>
>> Thanks,
>> Qu
>>
>>> Let's not forget the context we are oprating here
>>> - a broken fs so it's possible (if not likely) that any of the search
>>> functions inside detect_imode could return negative error value. OTOH
>>> I'd expect the transaction commit to fail if that's the case e.g. faulty
>>> device.
>>>
>>>>  	}
>>>> -	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
>>>> -		imode = 040755;
>>>> -	else
>>>> -		imode = 0100600;
>>>>
>>>>  	trans = btrfs_start_transaction(root, 1);
>>>>  	if (IS_ERR(trans)) {
>>>>
>>

Patch
diff mbox series

diff --git a/check/mode-common.c b/check/mode-common.c
index 195b6efaa7aa..807d7daf98a6 100644
--- a/check/mode-common.c
+++ b/check/mode-common.c
@@ -836,6 +836,80 @@  int reset_imode(struct btrfs_trans_handle *trans, struct btrfs_root *root,
 	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;
+	u64 ino;
+	u32 imode = S_IFREG;
+	int ret = 0;
+
+	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+	ino = key.objectid;
+	iitem = btrfs_item_ptr(path->nodes[0], path->slots[0],
+			       struct btrfs_inode_item);
+
+	/*
+	 * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
+	 * to BLK.
+	 */
+	if (btrfs_inode_rdev(path->nodes[0], iitem) != 0) {
+		imode = S_IFBLK;
+		goto out;
+	}
+
+	/* root inode */
+	if (ino == BTRFS_FIRST_FREE_OBJECTID) {
+		imode = S_IFDIR;
+		goto out;
+	}
+
+	while (1) {
+		ret = btrfs_next_item(root, path);
+		if (ret > 0) {
+			/* No determining result found, falls back to REG */
+			ret = 0;
+			goto out;
+		}
+		if (ret < 0)
+			goto out;
+		btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
+		if (key.objectid != ino)
+			goto out;
+
+		/*
+		 * We ignore some types to make life easier:
+		 * - INODE_REF
+		 *   We need to do a full tree search, which can fail for
+		 *   corrupted fs, but not worthy compared to other easier
+		 *   to determine types.
+		 * - XATTR
+		 *   Both REG and DIR can have xattr, so not useful
+		 */
+		switch (key.type) {
+		case BTRFS_DIR_ITEM_KEY:
+		case BTRFS_DIR_INDEX_KEY:
+			imode = S_IFDIR;
+			goto out;
+		case BTRFS_EXTENT_DATA_KEY:
+			/*
+			 * Both REG and LINK could have EXTENT_DATA.
+			 * We just fall back to REG as user can inspect the
+			 * content.
+			 */
+			imode = S_IFREG;
+			goto out;
+		}
+	}
+
+out:
+	/* Set default value even when something wrong happened */
+	*imode_ret = (imode | priv);
+	return ret;
+}
+
 /*
  * Reset the inode mode of the inode specified by @path.
  *
@@ -852,22 +926,18 @@  int repair_imode_common(struct btrfs_root *root, struct btrfs_path *path)
 	u32 imode;
 	int ret;
 
-	if (root->root_key.objectid != BTRFS_ROOT_TREE_OBJECTID) {
-		error(
-		"repair inode mode outside of root tree is not supported yet");
-		return -ENOTTY;
-	}
 	btrfs_item_key_to_cpu(path->nodes[0], &key, path->slots[0]);
 	ASSERT(key.type == BTRFS_INODE_ITEM_KEY);
-	if (key.objectid != BTRFS_ROOT_TREE_DIR_OBJECTID &&
-	    !is_fstree(key.objectid)) {
-		error("unsupported ino %llu", key.objectid);
-		return -ENOTTY;
+	if (root->objectid == BTRFS_ROOT_TREE_OBJECTID) {
+		/* In root tree we only have two possible imode */
+		if (key.objectid == BTRFS_ROOT_TREE_OBJECTID)
+			imode = S_IFDIR | 0755;
+		else
+			imode = S_IFREG | 0600;
+	} else {
+		detect_imode(root, path, &imode);
+		/* Ignore error returned, just use the default value returned */
 	}
-	if (key.objectid == BTRFS_ROOT_TREE_DIR_OBJECTID)
-		imode = 040755;
-	else
-		imode = 0100600;
 
 	trans = btrfs_start_transaction(root, 1);
 	if (IS_ERR(trans)) {