[PATCH 1/3] btrfs-progs: check: report more specific info about invalid location

[PATCH 1/3] btrfs-progs: check: report more specific info about invalid location

[Su Yue]

Previously, it was so useless to print message like
"invalid location %d".

Let it print objectid and offset of dir_item.
Debug is easier now.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index a93ac2c88a38..5c15dfb60b9a 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -1635,8 +1635,8 @@ static int process_dir_item(struct extent_buffer *eb,
 					  namebuf, len, filetype,
 					  key->type, error);
 		} else {
-			fprintf(stderr, "invalid location in dir item %u\n",
-				location.type);
+			fprintf(stderr, "invalid location in dir item[%llu %llu]\n",
+				key->objectid, key->offset);
 			add_inode_backref(inode_cache, BTRFS_MULTIPLE_OBJECTIDS,
 					  key->objectid, key->offset, namebuf,
 					  len, filetype, key->type, error);
-- 
2.15.0

[PATCH 2/3] btrfs-progs: check: do not reset ret after try_repair_inode

[Su Yue]

After failure of try_repair_inode(), the value @ret will be reseted
anyway. Then counter @error won't be incremented.

So do not reset @ret after try_repair_inode().

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 5c15dfb60b9a..68348ae94c8b 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -3968,7 +3968,6 @@ static int check_inode_recs(struct btrfs_root *root,
 				free_inode_rec(rec);
 				continue;
 			}
-			ret = 0;
 		}
 
 		if (!(repair && ret == 0))
-- 
2.15.0

[PATCH 3/3] btrfs-progs: check: record last checked root_item in original mode

[Su Yue]

check_fs_roots() will check all fs trees again if fs_info->tree_root
have been cowed.
It is inefficient if there are many subvolumes in filesystem.

And it also causes dead loop while repairing
fuzz-tests/images/bko-161811.raw:
=============================
ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
invalid location in dir item 0
root 5 root dir 256 error
root 5 inode 256 errors 10, odd dir item
Failed to reset nlink for inode 18446744073709551361: No such file or directory
        unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
invalid location in dir item 0
root 5 root dir 256 error
root 5 inode 256 errors 10, odd dir item
Failed to reset nlink for inode 18446744073709551361: No such file or directory
        unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283
...
==============================

Process of the dead loop:
1) check_fs_root() failed to repair the inode.
2) btrfs_commit_transaction() did cow of the fs_info->tree_root
3) check_fs_roots() restarted to check fs tree.
4) goto 1).

Introduce a variable @prev_key to record last checked root_item.
If check_fs_root() failed, go to check the next fs_tree instead of
trying it again and again.

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
 cmds-check.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index 68348ae94c8b..d149608acde2 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -4587,6 +4587,7 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
 {
 	struct btrfs_path path;
 	struct btrfs_key key;
+	struct btrfs_key prev_key;
 	struct walk_control wc;
 	struct extent_buffer *leaf, *tree_node;
 	struct btrfs_root *tmp_root;
@@ -4609,19 +4610,21 @@ static int check_fs_roots(struct btrfs_fs_info *fs_info,
 	cache_tree_init(&wc.shared);
 	btrfs_init_path(&path);
 
+	prev_key.offset = 0;
+	prev_key.objectid = 0;
+	prev_key.type = BTRFS_ROOT_ITEM_KEY;
 again:
-	key.offset = 0;
-	key.objectid = 0;
-	key.type = BTRFS_ROOT_ITEM_KEY;
+	key = prev_key;
 	ret = btrfs_search_slot(NULL, tree_root, &key, &path, 0, 0);
 	if (ret < 0) {
 		err = 1;
 		goto out;
 	}
+	if (!ret)
+		path.slots[0]++;
 	tree_node = tree_root->node;
 	while (1) {
 		if (tree_node != tree_root->node) {
-			free_root_recs_tree(root_cache);
 			btrfs_release_path(&path);
 			goto again;
 		}
@@ -4636,6 +4639,9 @@ again:
 			leaf = path.nodes[0];
 		}
 		btrfs_item_key_to_cpu(leaf, &key, path.slots[0]);
+		/* save key */
+		prev_key = key;
+
 		if (key.type == BTRFS_ROOT_ITEM_KEY &&
 		    fs_root_objectid(key.objectid)) {
 			if (key.objectid == BTRFS_TREE_RELOC_OBJECTID) {
@@ -4654,6 +4660,8 @@ again:
 			if (ret == -EAGAIN) {
 				free_root_recs_tree(root_cache);
 				btrfs_release_path(&path);
+				prev_key.objectid = 0;
+				prev_key.offset = 0;
 				goto again;
 			}
 			if (ret)
-- 
2.15.0

Re: [PATCH 1/3] btrfs-progs: check: report more specific info about invalid location

[Nikolay Borisov]

On 21.11.2017 12:15, Su Yue wrote:
> Previously, it was so useless to print message like
> "invalid location %d".
> 
> Let it print objectid and offset of dir_item.
> Debug is easier now.
> 
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> ---
>  cmds-check.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index a93ac2c88a38..5c15dfb60b9a 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -1635,8 +1635,8 @@ static int process_dir_item(struct extent_buffer *eb,
>  					  namebuf, len, filetype,
>  					  key->type, error);
>  		} else {
> -			fprintf(stderr, "invalid location in dir item %u\n",
> -				location.type);
> +			fprintf(stderr, "invalid location in dir item[%llu %llu]\n",
> +				key->objectid, key->offset);

I think it will be good if in addition to the dir item's key you also
print the invalid type value. I.e. this check is triggered if
location.type is unrecognizable. So the error message could be something
like :

fprintf(stderr, "Unrecognised location.type (%u) in DIR_ITEM[%llu
%llu]", key->objectid, key->offset);

Let's not force the user to go and read the code to understand why this
error happened.


>  			add_inode_backref(inode_cache, BTRFS_MULTIPLE_OBJECTIDS,
>  					  key->objectid, key->offset, namebuf,
>  					  len, filetype, key->type, error);
> 

Re: [PATCH 1/3] btrfs-progs: check: report more specific info about invalid location

[Su Yue]

On 11/21/2017 07:46 PM, Nikolay Borisov wrote:
> 
> 
> On 21.11.2017 12:15, Su Yue wrote:
>> Previously, it was so useless to print message like
>> "invalid location %d".
>>
>> Let it print objectid and offset of dir_item.
>> Debug is easier now.
>>
>> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
>> ---
>>   cmds-check.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/cmds-check.c b/cmds-check.c
>> index a93ac2c88a38..5c15dfb60b9a 100644
>> --- a/cmds-check.c
>> +++ b/cmds-check.c
>> @@ -1635,8 +1635,8 @@ static int process_dir_item(struct extent_buffer *eb,
>>   					  namebuf, len, filetype,
>>   					  key->type, error);
>>   		} else {
>> -			fprintf(stderr, "invalid location in dir item %u\n",
>> -				location.type);
>> +			fprintf(stderr, "invalid location in dir item[%llu %llu]\n",
>> +				key->objectid, key->offset);
> 
> I think it will be good if in addition to the dir item's key you also
> print the invalid type value. I.e. this check is triggered if
> location.type is unrecognizable. So the error message could be something
> like :
> 
> fprintf(stderr, "Unrecognised location.type (%u) in DIR_ITEM[%llu
> %llu]", key->objectid, key->offset);
> 
Thanks. Printing location type is more specific.
I will update the patch.

Thanks,
Su
> Let's not force the user to go and read the code to understand why this
> error happened.
> 
> 
>>   			add_inode_backref(inode_cache, BTRFS_MULTIPLE_OBJECTIDS,
>>   					  key->objectid, key->offset, namebuf,
>>   					  len, filetype, key->type, error);
>>
> 
> 

[PATCH v2 1/3] btrfs-progs: check: report more specific info about invalid location

[Su Yue]

Previously, it was so useless to print message like
"invalid location %d".

Let it print objectid and offset of dir_item too.
Debug is easier now.

Suggested-by: Nikolay Borisov <nborisov@suse.com>
Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
changelog:
v2:
	Print unknown location type.
---
 cmds-check.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/cmds-check.c b/cmds-check.c
index a93ac2c88a38..416a7601bce1 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -1635,8 +1635,9 @@ static int process_dir_item(struct extent_buffer *eb,
 					  namebuf, len, filetype,
 					  key->type, error);
 		} else {
-			fprintf(stderr, "invalid location in dir item %u\n",
-				location.type);
+			fprintf(stderr,
+				"Unknown location type %d in DIR_ITEM[%llu %llu]\n",
+				location.type, key->objectid, key->offset);
 			add_inode_backref(inode_cache, BTRFS_MULTIPLE_OBJECTIDS,
 					  key->objectid, key->offset, namebuf,
 					  len, filetype, key->type, error);
-- 
2.15.0

Re: [PATCH v2 1/3] btrfs-progs: check: report more specific info about invalid location

[Nikolay Borisov]

On 22.11.2017 04:17, Su Yue wrote:
> Previously, it was so useless to print message like
> "invalid location %d".
> 
> Let it print objectid and offset of dir_item too.
> Debug is easier now.
> 
> Suggested-by: Nikolay Borisov <nborisov@suse.com>
> Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>


Reviewed-by: Nikolay Borisov <nborisov@suse.com>

> ---
> changelog:
> v2:
> 	Print unknown location type.
> ---
>  cmds-check.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/cmds-check.c b/cmds-check.c
> index a93ac2c88a38..416a7601bce1 100644
> --- a/cmds-check.c
> +++ b/cmds-check.c
> @@ -1635,8 +1635,9 @@ static int process_dir_item(struct extent_buffer *eb,
>  					  namebuf, len, filetype,
>  					  key->type, error);
>  		} else {
> -			fprintf(stderr, "invalid location in dir item %u\n",
> -				location.type);
> +			fprintf(stderr,
> +				"Unknown location type %d in DIR_ITEM[%llu %llu]\n",
> +				location.type, key->objectid, key->offset);
>  			add_inode_backref(inode_cache, BTRFS_MULTIPLE_OBJECTIDS,
>  					  key->objectid, key->offset, namebuf,
>  					  len, filetype, key->type, error);
> 

Re: [PATCH 2/3] btrfs-progs: check: do not reset ret after try_repair_inode

[David Sterba]

On Tue, Nov 21, 2017 at 06:15:23PM +0800, Su Yue wrote:
> After failure of try_repair_inode(), the value @ret will be reseted
> anyway. Then counter @error won't be incremented.

Can you pleae expand the changelog? I don't see how this patch helps,
the extra line does not hurt readability so I don't see a strong reason
to remove it.

Re: [PATCH 3/3] btrfs-progs: check: record last checked root_item in original mode

[David Sterba]

On Tue, Nov 21, 2017 at 06:15:24PM +0800, Su Yue wrote:
> check_fs_roots() will check all fs trees again if fs_info->tree_root
> have been cowed.
> It is inefficient if there are many subvolumes in filesystem.
> 
> And it also causes dead loop while repairing
> fuzz-tests/images/bko-161811.raw:
> =============================
> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
> invalid location in dir item 0
> root 5 root dir 256 error
> root 5 inode 256 errors 10, odd dir item
> Failed to reset nlink for inode 18446744073709551361: No such file or directory
>         unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
> invalid location in dir item 0
> root 5 root dir 256 error
> root 5 inode 256 errors 10, odd dir item
> Failed to reset nlink for inode 18446744073709551361: No such file or directory
>         unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283
> ...
> ==============================
> 
> Process of the dead loop:
> 1) check_fs_root() failed to repair the inode.
> 2) btrfs_commit_transaction() did cow of the fs_info->tree_root
> 3) check_fs_roots() restarted to check fs tree.
> 4) goto 1).
> 
> Introduce a variable @prev_key to record last checked root_item.
> If check_fs_root() failed, go to check the next fs_tree instead of
> trying it again and again.

Patch looks good to me, do you have a test image?

Re: [PATCH v2 1/3] btrfs-progs: check: report more specific info about invalid location

[David Sterba]

On Wed, Nov 22, 2017 at 09:06:55AM +0200, Nikolay Borisov wrote:
> 
> 
> On 22.11.2017 04:17, Su Yue wrote:
> > Previously, it was so useless to print message like
> > "invalid location %d".
> > 
> > Let it print objectid and offset of dir_item too.
> > Debug is easier now.
> > 
> > Suggested-by: Nikolay Borisov <nborisov@suse.com>
> > Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>

Applied, thanks.

Re: [PATCH 3/3] btrfs-progs: check: record last checked root_item in original mode

[Su Yue]

On 01/09/2018 03:03 AM, David Sterba wrote:
> On Tue, Nov 21, 2017 at 06:15:24PM +0800, Su Yue wrote:
>> check_fs_roots() will check all fs trees again if fs_info->tree_root
>> have been cowed.
>> It is inefficient if there are many subvolumes in filesystem.
>>
>> And it also causes dead loop while repairing
>> fuzz-tests/images/bko-161811.raw:
>> =============================
>> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
>> invalid location in dir item 0
>> root 5 root dir 256 error
>> root 5 inode 256 errors 10, odd dir item
>> Failed to reset nlink for inode 18446744073709551361: No such file or directory
>>          unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
>> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283096 have 709597396
>> invalid location in dir item 0
>> root 5 root dir 256 error
>> root 5 inode 256 errors 10, odd dir item
>> Failed to reset nlink for inode 18446744073709551361: No such file or directory
>>          unresolved ref dir 256 index 0 namelen 32 name  filetype 1 errors 106, no dir index, no inode ref, name too long
>> ERROR: DIR_ITEM[256 1167283096] name  namelen 32 filetype 1 mismatch with its hash, wanted 1167283
>> ...
>> ==============================
>>
>> Process of the dead loop:
>> 1) check_fs_root() failed to repair the inode.
>> 2) btrfs_commit_transaction() did cow of the fs_info->tree_root
>> 3) check_fs_roots() restarted to check fs tree.
>> 4) goto 1).
>>
>> Introduce a variable @prev_key to record last checked root_item.
>> If check_fs_root() failed, go to check the next fs_tree instead of
>> trying it again and again.
> 
> Patch looks good to me, do you have a test image?
> 
fuzz-tests/images/bko-161811.raw there is.
Or do I need to add an another case to fsck-tests?

Thanks,
Su

> 

[PATCH 2/3 v2] btrfs-progs: check: do not reset ret after try_repair_inode

[Su Yue]

If repair is enabled and try_repair_inode() failed in
check_inode_recs(), then value @ret will be reset to 0 anyway.
Then counter @error won't be incremented.

In other words, if previous check of the root directory is fine,
check_inode_recs() always returns 0 even it failed to repair inodes.

So do not reset @ret after try_repair_inode().

Signed-off-by: Su Yue <suy.fnst@cn.fujitsu.com>
---
Changlog:
v1->v2:
	Add more detailed commit message.
---
 cmds-check.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/cmds-check.c b/cmds-check.c
index 635c1c44ff6f..00cbc0114b22 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -3968,7 +3968,6 @@ static int check_inode_recs(struct btrfs_root *root,
 				free_inode_rec(rec);
 				continue;
 			}
-			ret = 0;
 		}
 
 		if (!(repair && ret == 0))
-- 
2.15.1