[...]
>> - 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 <w...@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?
>
>
>> + if (ret < 0)
>> + goto out;
>> + leaf = path->nodes[0];
>> + slot = path->slots[0];
>> + btrfs_item_key_to_cpu(leaf, &key, slot);
>> + if (key.objectid != ino)
>> + goto out;
>> +
>> + /*
>> + * We ignore some types to make life easier:
>> + * - XATTR
>> + * Both REG and DIR can have xattr, so not useful
>> + */
>> + switch (key.type) {
>> + case BTRFS_INODE_REF_KEY:
>> + /* The most accurate way to determine filetype */
>> + cur = btrfs_item_ptr_offset(leaf, slot);
>> + end = cur + btrfs_item_size_nr(leaf, slot);
>> + while (cur < end) {
>> + iref = (struct btrfs_inode_ref *)cur;
>> + namelen = min_t(u32, end - cur - sizeof(&iref),
>> + btrfs_inode_ref_name_len(leaf, iref));
>> + index = btrfs_inode_ref_index(leaf, iref);
>> + read_extent_buffer(leaf, namebuf,
>> + (unsigned long)(iref + 1), namelen);
>> + ret = find_file_type(root, ino, key.offset,
>> + index, namebuf, namelen,
>> + &imode);
>> + if (ret == 0) {
>> + found = true;
>> + goto out;
>> + }
>> + cur += sizeof(*iref) + namelen;
>> + }
>> + break;
>> + case BTRFS_DIR_ITEM_KEY:
>> + case BTRFS_DIR_INDEX_KEY:
>> + imode = S_IFDIR;
>
> You should set found here.
Yep, also found that and fixed in next version.
Thanks,
Qu
> Otherwise this function returns ENOENT in those 2 branches.
> BTW in relation to out private conversation I found this while trying to
> create test
> cases which will trigger all branches. The fact it found bugs proves we
> should strive
> for as much testing coverage as possible.
>
>> + 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;
> ditto
>
>> + goto out;
>> + }
>> + }
>> +
>> +out:
>> + /*
>> + * Both CHR and BLK uses rdev, no way to distinguish them, so fall back
>> + * to BLK. But either way it doesn't really matter, as CHR/BLK on btrfs
>> + * should be pretty rare, and no real data will be lost.
>> + */
>> + if (!found && btrfs_stack_inode_rdev(&iitem) != 0) {
>> + imode = S_IFBLK;
>> + found = true;
>> + }
>> +
>> + if (found)
>> + *imode_ret = (imode | priv);
>> + else
>> + ret = -ENOENT;
>> + return ret;
>> +}
>> +
>> /*
>> * Reset the inode mode of the inode specified by @path.
>
> <snip>
>
