Re: [PATCH v4 3/5] btrfs: tree-checker: Enhance output for btrfs_check_leaf

[Nikolay Borisov <nborisov@suse.com>]

On 12.10.2017 03:28, Qu Wenruo wrote:
> 
> 
> On 2017年10月11日 23:41, Nikolay Borisov wrote:
>>
>>
>> On  9.10.2017 04:51, Qu Wenruo wrote:
>>> Enhance the output to print:
>>> 1) Reason
>>> 2) Bad value
>>>     If reason can't explain enough
>>> 3) Good value (range)
>>>
>>> Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
>>> ---
>>>   fs/btrfs/tree-checker.c | 27 +++++++++++++++++++++------
>>>   1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/btrfs/tree-checker.c b/fs/btrfs/tree-checker.c
>>> index b4ced8d3ce2a..7bba195ecc8b 100644
>>> --- a/fs/btrfs/tree-checker.c
>>> +++ b/fs/btrfs/tree-checker.c
>>> @@ -233,8 +233,9 @@ int btrfs_check_leaf(struct btrfs_root *root,
>>> struct extent_buffer *leaf)
>>>               eb = btrfs_root_node(check_root);
>>>               /* if leaf is the root, then it's fine */
>>>               if (leaf != eb) {
>>> -                CORRUPT("non-root leaf's nritems is 0",
>>> -                    leaf, check_root, 0);
>>> +                generic_err(check_root, leaf, 0,
>>> +                    "invalid nritems, have %u shouldn't be 0 for
>>> non-root leaf",
>>> +                    nritems);
>>
>> I'm a bit confused by what this error messages wants to convey. Even
>> reading the previous version with CORRUPT() it still didn't make sense.
>> So what we want to say here is we shouldn't have empty leaf nodes. So
>> Something along the line of "Unexpected empty leaf".
> 
> Yes, the error message is too fixed to follow the output format.
>>
>> Why would the (leaf != eb) check not trigger, given that we call
>> btrfs_check_leaf when we now that the item is a leaf (level is 0 )?
> 
> What's the problem here? I didn't really get your point.
> 
> Did you mean leaf can't be tree root? Or empty tree root is not possible?
> 
> 
> It's completely possible for a leaf to be a tree root.
> 
> All tree roots of a newly created (without --rootdir) is leaf.
> Because the content of each tree is so few that one leaf can contain
> them all.
> 
> 
> And it's also very possible to have empty tree, whose root (leaf) is
> also empty.
> Still for a newly created btrfs, its csum tree is empty.
> Its uuid tree is also empty.
> 
> 
> But the only valid case for empty leaf is when it's a tree root.
> So the code just checks it, and I didn't find anything wrong here.

I was just confused when the invariant wouldn't hold. Now that you've
explained it I agree with the change.


> 
> Thanks,
> Qu
> 
>>
>>
>>>                   free_extent_buffer(eb);
>>>                   return -EUCLEAN;
>>>               }
>>> @@ -265,7 +266,11 @@ int btrfs_check_leaf(struct btrfs_root *root,
>>> struct extent_buffer *leaf)
>>>             /* Make sure the keys are in the right order */
>>>           if (btrfs_comp_cpu_keys(&prev_key, &key) >= 0) {
>>> -            CORRUPT("bad key order", leaf, root, slot);
>>> +            generic_err(root, leaf, slot,
>>> +                "bad key order, prev key (%llu %u %llu) current key
>>> (%llu %u %llu)",
>>> +                prev_key.objectid, prev_key.type,
>>> +                prev_key.offset, key.objectid, key.type,
>>> +                key.offset);
>>>               return -EUCLEAN;
>>>           }
>>>   @@ -280,7 +285,10 @@ int btrfs_check_leaf(struct btrfs_root *root,
>>> struct extent_buffer *leaf)
>>>               item_end_expected = btrfs_item_offset_nr(leaf,
>>>                                    slot - 1);
>>>           if (btrfs_item_end_nr(leaf, slot) != item_end_expected) {
>>> -            CORRUPT("slot offset bad", leaf, root, slot);
>>> +            generic_err(root, leaf, slot,
>>> +                "discontinious item end, have %u expect %u",
>>
>> s/discontinious/unexpected ?
>>
>>> +                btrfs_item_end_nr(leaf, slot),
>>> +                item_end_expected);
>>>               return -EUCLEAN;
>>>           }
>>>   @@ -291,14 +299,21 @@ int btrfs_check_leaf(struct btrfs_root *root,
>>> struct extent_buffer *leaf)
>>>            */
>>>           if (btrfs_item_end_nr(leaf, slot) >
>>>               BTRFS_LEAF_DATA_SIZE(fs_info)) {
>>> -            CORRUPT("slot end outside of leaf", leaf, root, slot);
>>> +            generic_err(root, leaf, slot,
>>> +                "slot end outside of leaf, have %u expect range [0,
>>> %u]",> +                btrfs_item_end_nr(leaf, slot),
>>> +                BTRFS_LEAF_DATA_SIZE(fs_info));
>>>               return -EUCLEAN;
>>>           }
>>>             /* Also check if the item pointer overlaps with btrfs
>>> item. */
>>>           if (btrfs_item_nr_offset(slot) + sizeof(struct btrfs_item) >
>>>               btrfs_item_ptr_offset(leaf, slot)) {
>>> -            CORRUPT("slot overlap with its data", leaf, root, slot);
>>> +            generic_err(root, leaf, slot,
>>> +                "slot overlap with its data, item end %lu data start
>>> %lu",
>>> +                btrfs_item_nr_offset(slot) +
>>> +                sizeof(struct btrfs_item),
>>> +                btrfs_item_ptr_offset(leaf, slot));
>>>               return -EUCLEAN;
>>>           }
>>>  
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>