On Mon, Feb 24, 2014 at 7:38 PM, Wang Shilong <wangsl.f...@cn.fujitsu.com> 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.f...@cn.fujitsu.com> 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.f...@cn.fujitsu.com> >>> Signed-off-by: Wang Shilong <wangsl.f...@cn.fujitsu.com> >>> --- >>> 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html