Re: [PATCH RFC 04/16] btrfs-progs: fsck: Introduce function to check referencer of a backref

From: Qu Wenruo <quwenruo@cn.fujitsu.com>
To: Josef Bacik <jbacik@fb.com>, <linux-btrfs@vger.kernel.org>
Cc: <dsterba@suse.cz>, Lu Fengqi <lufq.fnst@cn.fujitsu.com>
Subject: Re: [PATCH RFC 04/16] btrfs-progs: fsck: Introduce function to check referencer of a backref
Date: Fri, 29 Apr 2016 13:31:42 +0800	[thread overview]
Message-ID: <12c81894-9495-fa86-63ce-4ce38def5e0e@cn.fujitsu.com> (raw)
In-Reply-To: <1d696d31-da61-7e99-6639-692c7b05240f@fb.com>

Josef Bacik wrote on 2016/04/28 10:17 -0400:
> On 04/25/2016 11:48 PM, Qu Wenruo wrote:
>> From: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>>
>> Introduce a new function check_tree_block_backref() to check if a
>> backref points to correct referencer.
>>
>> Signed-off-by: Lu Fengqi <lufq.fnst@cn.fujitsu.com>
>> Signed-off-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
>> ---
>>  cmds-check.c | 95
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 95 insertions(+)
>>
>> diff --git a/cmds-check.c b/cmds-check.c
>> index 6633b6e..81dd4f3 100644
>> --- a/cmds-check.c
>> +++ b/cmds-check.c
>> @@ -323,6 +323,8 @@ struct root_item_info {
>>  #define MISSING_BACKREF    (1 << 0) /* Completely no backref in
>> extent tree */
>>  #define BAD_BACKREF    (1 << 1) /* Backref mismatch */
>>  #define UNALIGNED_BYTES    (1 << 2) /* Some bytes are not aligned */
>> +#define MISSING_REFERENCER (1 << 3) /* Referencer not found */
>> +#define BAD_REFERENCER    (1 << 4) /* Referencer found, but not
>> mismatch */
>>
>>  static void *print_status_check(void *p)
>>  {
>> @@ -8736,6 +8738,99 @@ out:
>>      return ret;
>>  }
>>
>> +/*
>> + * Check if a tree block backref is valid (points to valid tree block)
>> + * if level == -1, level will be resolved
>> + */
>> +static int check_tree_block_backref(struct btrfs_fs_info *fs_info,
>> u64 root_id,
>> +                    u64 bytenr, int level)
>> +{
>> +    struct btrfs_root *root;
>> +    struct btrfs_key key;
>> +    struct btrfs_path path;
>> +    struct extent_buffer *eb;
>> +    struct extent_buffer *node;
>> +    u32 nodesize = btrfs_super_nodesize(fs_info->super_copy);
>> +    int err = 0;
>> +    int ret;
>> +
>> +    /* Query level for level == -1 special case */
>> +    if (level == -1)
>> +        level = query_tree_block_level(fs_info, bytenr);
>> +    if (level < 0) {
>> +        err = MISSING_REFERENCER;
>> +        goto out;
>> +    }
>> +
>> +    key.objectid = root_id;
>> +    key.type = BTRFS_ROOT_ITEM_KEY;
>> +    key.offset = (u64)-1;
>> +
>> +    root = btrfs_read_fs_root(fs_info, &key);
>> +    if (IS_ERR(root)) {
>> +        err |= MISSING_REFERENCER;
>> +        goto out;
>> +    }
>> +
>> +    /* Read out the tree block to get item/node key */
>> +    eb = read_tree_block(root, bytenr, root->nodesize, 0);
>> +    /* Impossible, as tree block query has read out the tree block */
>> +    if (!extent_buffer_uptodate(eb)) {
>> +        err |= MISSING_REFERENCER;
>> +        free_extent_buffer(eb);
>> +        goto out;
>> +    }
>> +
>> +    /* Empty tree, no need to check key */
>> +    if (!btrfs_header_nritems(eb) && !level) {
>> +        free_extent_buffer(eb);
>> +        goto out;
>> +    }
>
> Actually this is interesting, because we don't actually catch where we
> would have btrfs_header_nritems(eb) == 0 and !level.  So maybe integrate
> that check into check_block, to make sure we have items in node level
> extent buffers.  Thanks,
>
> Josef
>
>
Unfortunately, we can't integrate it into check_tree_block() (or caller 
read_tree_block() and its variants).

As this is a valid case.
(And that's why we don't report error here)

For trees like csum tree and uuid tree, they can be empty, and it 
doesn't break any thing.

So it's a not something we need to integrate into check_tree_block().

And if you mean check_block() in old fsck codes, we won't use it in new 
fsck, so still not a big problem.

Thanks,
Qu