On Mon, Feb 24, 2014 at 7:38 PM, Wang Shilong
<wangsl.fnst@xxxxxxxxxxxxxx> wrote:
> Hi Mitch,
>
>
> On 02/25/2014 07:03 AM, Mitch Harder wrote:
>>
>> On Mon, Feb 24, 2014 at 5:55 AM, Wang Shilong
>> <wangsl.fnst@xxxxxxxxxxxxxx> wrote:
>>>
>>> We found btrfsck will output backrefs mismatch while the filesystem
>>> is defenitely ok.
>>>
>>> The problem is that check_block() don't return right value,which
>>> makes btrfsck won't walk all tree blocks thus we don't get a consistent
>>> filesystem, we will fail to check extent refs etc.
>>>
>>> Reported-by: Gui Hecheng <guihc.fnst@xxxxxxxxxxxxxx>
>>> Signed-off-by: Wang Shilong <wangsl.fnst@xxxxxxxxxxxxxx>
>>> ---
>>> cmds-check.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/cmds-check.c b/cmds-check.c
>>> index a2afae6..253569f 100644
>>> --- a/cmds-check.c
>>> +++ b/cmds-check.c
>>> @@ -2477,7 +2477,7 @@ static int check_block(struct btrfs_trans_handle
>>> *trans,
>>> struct cache_extent *cache;
>>> struct btrfs_key key;
>>> enum btrfs_tree_block_status status;
>>> - int ret = 1;
>>> + int ret = 0;
>>> int level;
>>>
>>> cache = lookup_cache_extent(extent_cache, buf->start, buf->len);
>>> --
>>
>> I tried this fix on a broken btrfs volume I've been trying to repair,
>> and it seemed to put me in an infinite loop.
>>
>> I agree that something seems wrong with the way the caller of
>> check_block uses the return value, and I also noticed that it seemed
>> to exit before walking all the tree blocks.
>>
>> But I think the problem is more subtle than flipping the default ret
>> value from 1 to 0.
>
> No, not really even though i know there are other problems with fsck repair
> mode.
> But this problem should be fixed and pushed into btrfs-progsv3.13.(Notice,
> the below problem did not exist in btrfs-progsv3.12)
>
> An easy way to trigger this problem:
>
> # mkfs.btrfs -f /dev/sda9
> # mount /dev/sda9 /mnt
> # dd if=/dev/zero of=/mnt/data bs=4k count=10240 oflag=direct
> # btrfs sub snapshot /mnt /mnt/snap1
> # btrfs sub snapshot /mnt /mnt/snap2
> # umount /mnt
> # btrfs check /dev/sda9
>
> After applying this patch, the above problems did not exist.
> Feel free to correct me if i miss something here.^_^
>
I took a closer look at the check_block function today, and it looks
to me like the problem is that the return value is not modified when
BTRFS_BLOCK_FLAG_FULL_BACKREF is set.
@@ -2521,14 +2521,17 @@ static int check_block(struct btrfs_trans_handle *trans,
}
} else {
rec->content_checked = 1;
- if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+ if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
rec->owner_ref_checked = 1;
+ ret = 0;
+ }
else {
ret = check_owner_ref(root, rec, buf);
if (!ret)
rec->owner_ref_checked = 1;
}
For me, in this function I would lean towards an initial return value
that must be updated by having check_block() make an affirmative
PASS/FAIL decision on the block.
What do you think about something like this?
diff --git a/cmds-check.c b/cmds-check.c
index ffc5d3e..55070da 100644
--- a/cmds-check.c
+++ b/cmds-check.c
@@ -2477,7 +2477,7 @@ static int check_block(struct btrfs_trans_handle *trans,
struct cache_extent *cache;
struct btrfs_key key;
enum btrfs_tree_block_status status;
- int ret = 1;
+ int ret = -EINVAL;
int level;
cache = lookup_cache_extent(extent_cache, buf->start, buf->len);
@@ -2521,14 +2521,17 @@ static int check_block(struct btrfs_trans_handle *trans,
}
} else {
rec->content_checked = 1;
- if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF)
+ if (flags & BTRFS_BLOCK_FLAG_FULL_BACKREF) {
rec->owner_ref_checked = 1;
+ ret = 0;
+ }
else {
ret = check_owner_ref(root, rec, buf);
if (!ret)
rec->owner_ref_checked = 1;
}
}
+ BUG_ON(ret == -EINVAL);
if (!ret)
maybe_free_extent_rec(extent_cache, rec);
return ret;
--
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
