From patchwork Mon Jan 28 05:20:41 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mitch Harder X-Patchwork-Id: 2053431 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork2.kernel.org (Postfix) with ESMTP id 60DBBE00DA for ; Mon, 28 Jan 2013 05:20:46 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751201Ab3A1FUo (ORCPT ); Mon, 28 Jan 2013 00:20:44 -0500 Received: from mail-ob0-f179.google.com ([209.85.214.179]:57906 "EHLO mail-ob0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751130Ab3A1FUm (ORCPT ); Mon, 28 Jan 2013 00:20:42 -0500 Received: by mail-ob0-f179.google.com with SMTP id un3so2376993obb.38 for ; Sun, 27 Jan 2013 21:20:41 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=mime-version:x-received:in-reply-to:references:date:message-id :subject:from:to:cc:content-type:x-gm-message-state; bh=nT1OgGWdbkx2wqhGp6EXYFsATXaTVN+15yNBQvt/TBQ=; b=PEzs6RJKnSxN6iuucdST2i6fPcoJBmOIY+h9zP/1SfIzqUtgMhn6mytGeVwdDGdTCe hn892bTZjkVx3/RhAJy+jxIhG3iRVcC7CCg4bVMYb0KNtLdgOKmbldxOcAoJx7GzVX5a QXIEdyW3alDX4Rs6RZFrRFX6bvhDtt5ExkGhUoYyGyZEcRAW394XplJLRNt+mh/i8HtT laJy+ZwvXJVsJdBb2Nc4o0Ej/gXRAc8I7C0IKmKM0KORjJdNMgbcYVCiDH8Ys0t18z21 /7zstz175yUQx82PCE1Q3LAMjJs7Pbxn5q/PHIrMYvmMI135Y+0ZdEWsfldbWkVaj7/J 5c1Q== MIME-Version: 1.0 X-Received: by 10.182.53.3 with SMTP id x3mr10329773obo.87.1359350441341; Sun, 27 Jan 2013 21:20:41 -0800 (PST) Received: by 10.60.133.101 with HTTP; Sun, 27 Jan 2013 21:20:41 -0800 (PST) In-Reply-To: <20130127124154.GA16722@liubo> References: <1358339768-2314-1-git-send-email-bo.li.liu@oracle.com> <20130123075155.GE17162@liubo.jp.oracle.com> <20130124005221.GA28406@liubo> <20130125154236.GA3997@liubo> <20130127124154.GA16722@liubo> Date: Sun, 27 Jan 2013 23:20:41 -0600 Message-ID: Subject: Re: [PATCH V5] Btrfs: snapshot-aware defrag From: Mitch Harder To: bo.li.liu@oracle.com Cc: linux-btrfs@vger.kernel.org, chris.mason@fusionio.com, JBacik@fusionio.com, dave@jikos.cz, kitayama@cl.bb4u.ne.jp, miaox@cn.fujitsu.com X-Gm-Message-State: ALoCoQn6MfdKtQL6/MXO7ZoGmXrCqs3Cp6D/D1ErDbPd32K0KeDpzz1M288pBal3S0esZLNVJR+1 Sender: linux-btrfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-btrfs@vger.kernel.org On Sun, Jan 27, 2013 at 6:41 AM, Liu Bo wrote: > > Hi Mitch, > > Many thanks for testing it! > > Well, after some debugging, I finally figure out the whys: > > (1) btrfs_ioctl_snap_destroy() will free the inode of snapshot and set > root's refs to zero(btrfs_set_root_refs()), if this inode happens to > be the only one in the rbtree of the snapshot's root at this moment, > we add this root to the dead_root list. > > (2) Unfortunately, after (1), our snapshot-aware defrag work may read > another inode in this snapshot into memory during 'relink' stage, and > later after we finish relink work and iput() will force us to add the > snapshot's root to the dead_root list again. > > So that's why we get double list_add and list_del corruption. > > And IMO, it can also take place without snapshot-aware defrag, but it's a > rare case. I'm seeing a smattering of reports that resemble list corruption on the M/L, so that is possible. > > So could you please try this? > > thanks, > liubo > > diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c > index f154946..d4ee66b 100644 > --- a/fs/btrfs/transaction.c > +++ b/fs/btrfs/transaction.c > @@ -885,7 +885,15 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans, > int btrfs_add_dead_root(struct btrfs_root *root) > { > spin_lock(&root->fs_info->trans_lock); > + if (!list_empty(&root->root_list)) { > + struct btrfs_root *tmp; > + list_for_each_entry(tmp, &root->fs_info->dead_roots, root_list) > + if (tmp == root) > + goto unlock; > + } > + > list_add(&root->root_list, &root->fs_info->dead_roots); > +unlock: > spin_unlock(&root->fs_info->trans_lock); > return 0; > } > It feels like we're correcting the problem after-the-fact with this method, instead of addressing the root problem. But I was able to successfully run with this patch. I slightly modified your patch as follows by introducing a WARN_ON in order to get a back trace, and also to give me a positive confirmation that I was triggering the problem. } diff --git a/fs/btrfs/transaction.c b/fs/btrfs/transaction.c index d6b17fa..0c1066e 100644 --- a/fs/btrfs/transaction.c +++ b/fs/btrfs/transaction.c @@ -885,7 +885,18 @@ static noinline int commit_cowonly_roots(struct btrfs_trans_handle *trans, int btrfs_add_dead_root(struct btrfs_root *root) { spin_lock(&root->fs_info->trans_lock); + if (!list_empty(&root->root_list)) { + struct btrfs_root *tmp; + list_for_each_entry(tmp, &root->fs_info->dead_roots, root_list) + if (tmp == root) { + printk(KERN_ERR "btrfs: Duplicate dead root entry.\n"); + WARN_ON(1); + goto unlock; + } + } + list_add(&root->root_list, &root->fs_info->dead_roots); +unlock: spin_unlock(&root->fs_info->trans_lock); return 0;