From patchwork Tue Feb 25 21:39:34 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mitch Harder X-Patchwork-Id: 3719021 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 9BFB6BF13A for ; Tue, 25 Feb 2014 21:39:41 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A1EBB201ED for ; Tue, 25 Feb 2014 21:39:40 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 940D6201EC for ; Tue, 25 Feb 2014 21:39:39 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752635AbaBYVjg (ORCPT ); Tue, 25 Feb 2014 16:39:36 -0500 Received: from mail-ob0-f175.google.com ([209.85.214.175]:55959 "EHLO mail-ob0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751019AbaBYVjf (ORCPT ); Tue, 25 Feb 2014 16:39:35 -0500 Received: by mail-ob0-f175.google.com with SMTP id va2so6260259obc.34 for ; Tue, 25 Feb 2014 13:39:34 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:in-reply-to:references:date :message-id:subject:from:to:cc:content-type; bh=T5AWoEzJMAwC1JjimTR42DIM5ZB1EXjc3c3P/9eOlPs=; b=jchAlGLu/ajHZSZS8BmHQJpi113dcy31vzXo39Kz8xT3NNEgCTIwETGFCZHCM1g73L tuYk3AGbdam24JZtaDlyp/Vbq66QdZtgDo2CBdNlekxw1j+et7WuqCPeFjRt1uKMXoha AnPAEoViLDKDhSogUMapukf3gQOXQJbJudy2qeP57Mp0eW+ldNDf/BEbFsL+IU9Flr2Z KO6FwvmpIHIjr93oFaJwGTDy9VwLZI+93xnNHHCS2HnTYpb00aH3wCPLgzgoWyDZzx4j 73xukyUC7ZZDDpA3/jpx+E9n9KQWhJOlOliOxZyJvDblukDab8FXBYoBUQGZkbmnLGKT NhTA== X-Gm-Message-State: ALoCoQk2x8oySdaN/ACWVlGMY6XaM3mqyPqeYjy1+v9zmWw6aS7D/OOKCMjw3W5sJlcZgCuneigw MIME-Version: 1.0 X-Received: by 10.60.134.166 with SMTP id pl6mr3403723oeb.16.1393364374427; Tue, 25 Feb 2014 13:39:34 -0800 (PST) Received: by 10.60.7.195 with HTTP; Tue, 25 Feb 2014 13:39:34 -0800 (PST) In-Reply-To: <530BF42E.4010006@cn.fujitsu.com> References: <1393242914-21867-1-git-send-email-wangsl.fnst@cn.fujitsu.com> <530BF42E.4010006@cn.fujitsu.com> Date: Tue, 25 Feb 2014 15:39:34 -0600 Message-ID: Subject: Re: [PATCH] Btrfs-progs: fsck: fix wrong return value in check_block() From: Mitch Harder To: Wang Shilong Cc: linux-btrfs Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Mon, Feb 24, 2014 at 7:38 PM, Wang Shilong wrote: > Hi Mitch, > > > On 02/25/2014 07:03 AM, Mitch Harder wrote: >> >> On Mon, Feb 24, 2014 at 5:55 AM, Wang Shilong >> 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 >>> Signed-off-by: Wang Shilong >>> --- >>> 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? --- -- 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 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;