diff mbox series

[v4] btrfs: improve error reporting in lookup_inline_extent_backref

Message ID 20220428131449.792353-1-nborisov@suse.com (mailing list archive)
State New, archived
Headers show
Series [v4] btrfs: improve error reporting in lookup_inline_extent_backref | expand

Commit Message

Nikolay Borisov April 28, 2022, 1:14 p.m. UTC 
When iterating the backrefs in an extent item if the ptr to the
'current' backref record goes beyond the extent item a warning is
generated and -ENOENT is returned. However what's more appropriate to
debug such cases would be to return EUCLEAN and also print identifying
information about the performed search as well as the current content of
the leaf containing the possibly corrupted extent item.

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

V4:
 * Also print the value of 'parent' as it's pertinent when metadata inline backrefs
 are being searched (Filipe)
 * Print the leaf before printing the error message so that the latter is
 not lost (Filipe)

V3:
 * Fixed format for the btree slot
 * Removed redundant argument passed to format string

 fs/btrfs/extent-tree.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

--
2.25.1

Comments

Filipe Manana April 28, 2022, 3:16 p.m. UTC | #1 
On Thu, Apr 28, 2022 at 04:14:49PM +0300, Nikolay Borisov wrote:
> When iterating the backrefs in an extent item if the ptr to the
> 'current' backref record goes beyond the extent item a warning is
> generated and -ENOENT is returned. However what's more appropriate to
> debug such cases would be to return EUCLEAN and also print identifying
> information about the performed search as well as the current content of
> the leaf containing the possibly corrupted extent item.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> V4:
>  * Also print the value of 'parent' as it's pertinent when metadata inline backrefs
>  are being searched (Filipe)
>  * Print the leaf before printing the error message so that the latter is
>  not lost (Filipe)
> 
> V3:
>  * Fixed format for the btree slot
>  * Removed redundant argument passed to format string
> 
>  fs/btrfs/extent-tree.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
> index 963160a0c393..cae2ef560f3f 100644
> --- a/fs/btrfs/extent-tree.c
> +++ b/fs/btrfs/extent-tree.c
> @@ -895,7 +895,14 @@ int lookup_inline_extent_backref(struct btrfs_trans_handle *trans,
>  	err = -ENOENT;
>  	while (1) {
>  		if (ptr >= end) {
> -			WARN_ON(ptr > end);
> +			if (ptr > end) {
> +				err = -EUCLEAN;
> +				btrfs_print_leaf(path->nodes[0]);
> +				btrfs_crit(fs_info,
> +"overrun extent record at slot %d [%llu BTRFS_EXTENT_ITEM_KEY %llu] while looking for inline extent for root %llu owner %llu offset %llu parent %llu",
> +				path->slots[0], bytenr, num_bytes,

The key being printed is misleading, as it will often be incorrect.

First the type is not always BTRFS_EXTENT_ITEM_KEY, it can also be
BTRFS_METADATA_ITEM_KEY.

Secondly, the offset's value is not always 'num_bytes', it can also be 'owner'.

So it's better to just print the key as "%llu %u %llu" and using key.objectid,
key.type and key.offset. Or just leave the key from the message since we have
printed the slot, and therefore it's redundant. Either option is fine for me.

Sorry, I missed this before and I hate to have to make you send another version.

With that fixed,

Reviewed-by: Filipe Manana <fdmanana@suse.com>

Thanks.

> +				root_objectid, owner, offset, parent);
> +			}
>  			break;
>  		}
>  		iref = (struct btrfs_extent_inline_ref *)ptr;
> --
> 2.25.1
>
Nikolay Borisov April 28, 2022, 4:39 p.m. UTC | #2 
On 28.04.22 г. 18:16 ч., Filipe Manana wrote:
> On Thu, Apr 28, 2022 at 04:14:49PM +0300, Nikolay Borisov wrote:
>> When iterating the backrefs in an extent item if the ptr to the
>> 'current' backref record goes beyond the extent item a warning is
>> generated and -ENOENT is returned. However what's more appropriate to
>> debug such cases would be to return EUCLEAN and also print identifying
>> information about the performed search as well as the current content of
>> the leaf containing the possibly corrupted extent item.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> V4:
>>   * Also print the value of 'parent' as it's pertinent when metadata inline backrefs
>>   are being searched (Filipe)
>>   * Print the leaf before printing the error message so that the latter is
>>   not lost (Filipe)
>>
>> V3:
>>   * Fixed format for the btree slot
>>   * Removed redundant argument passed to format string
>>
>>   fs/btrfs/extent-tree.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
>> index 963160a0c393..cae2ef560f3f 100644
>> --- a/fs/btrfs/extent-tree.c
>> +++ b/fs/btrfs/extent-tree.c
>> @@ -895,7 +895,14 @@ int lookup_inline_extent_backref(struct btrfs_trans_handle *trans,
>>   	err = -ENOENT;
>>   	while (1) {
>>   		if (ptr >= end) {
>> -			WARN_ON(ptr > end);
>> +			if (ptr > end) {
>> +				err = -EUCLEAN;
>> +				btrfs_print_leaf(path->nodes[0]);
>> +				btrfs_crit(fs_info,
>> +"overrun extent record at slot %d [%llu BTRFS_EXTENT_ITEM_KEY %llu] while looking for inline extent for root %llu owner %llu offset %llu parent %llu",
>> +				path->slots[0], bytenr, num_bytes,
> 
> The key being printed is misleading, as it will often be incorrect.
> 
> First the type is not always BTRFS_EXTENT_ITEM_KEY, it can also be
> BTRFS_METADATA_ITEM_KEY.
> 
> Secondly, the offset's value is not always 'num_bytes', it can also be 'owner'.
> 
> So it's better to just print the key as "%llu %u %llu" and using key.objectid,
> key.type and key.offset. Or just leave the key from the message since we have
> printed the slot, and therefore it's redundant. Either option is fine for me.

Fair enough, I'd go with the slot since it will make the string line 
shorter in any case.

> 
> Sorry, I missed this before and I hate to have to make you send another version.
> 
> With that fixed,
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
> Thanks.
> 
>> +				root_objectid, owner, offset, parent);
>> +			}
>>   			break;
>>   		}
>>   		iref = (struct btrfs_extent_inline_ref *)ptr;
>> --
>> 2.25.1
>>
>
diff mbox series

Patch

diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index 963160a0c393..cae2ef560f3f 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -895,7 +895,14 @@  int lookup_inline_extent_backref(struct btrfs_trans_handle *trans,
 	err = -ENOENT;
 	while (1) {
 		if (ptr >= end) {
-			WARN_ON(ptr > end);
+			if (ptr > end) {
+				err = -EUCLEAN;
+				btrfs_print_leaf(path->nodes[0]);
+				btrfs_crit(fs_info,
+"overrun extent record at slot %d [%llu BTRFS_EXTENT_ITEM_KEY %llu] while looking for inline extent for root %llu owner %llu offset %llu parent %llu",
+				path->slots[0], bytenr, num_bytes,
+				root_objectid, owner, offset, parent);
+			}
 			break;
 		}
 		iref = (struct btrfs_extent_inline_ref *)ptr;