From patchwork Wed Oct 15 18:37:26 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Filipe Manana X-Patchwork-Id: 5086521 Return-Path: X-Original-To: patchwork-linux-btrfs@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 4EE5E9F30B for ; Wed, 15 Oct 2014 17:37:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 5E04020109 for ; Wed, 15 Oct 2014 17:37:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 4FC3C20120 for ; Wed, 15 Oct 2014 17:37:55 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751116AbaJORhw (ORCPT ); Wed, 15 Oct 2014 13:37:52 -0400 Received: from mail-wi0-f174.google.com ([209.85.212.174]:64119 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750775AbaJORhu (ORCPT ); Wed, 15 Oct 2014 13:37:50 -0400 Received: by mail-wi0-f174.google.com with SMTP id h11so10608412wiw.13 for ; Wed, 15 Oct 2014 10:37:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=from:to:cc:subject:date:message-id; bh=9bgoGUu7LP0My0y6CS1XXx+umAkXPPlZTds3sJw1JlE=; b=gryR3QMXB9f7BJBEXVHBU+m4SkadQeZ3K+O5CAiDo0SbaV9Ta7l3sVJTTx0/pe6szz LB3K4l0hCqJQgPECkBlQsPN15LSagVimArul6wVBzhfOGrygJaVWO5OFQCllOcXv6DNl ckcrcQ1BzcxjYc0KIfkaBvx6KGoFkgGy/RL3+8wNAFyBh6ii993ApKmFl/fD/N791RK5 3g0GWSU+OKidQuW3uDujFpi3B8QsJIEwxjzHP5Z9dZkERA0RzwzDLxlxyPfYlWBRJNQ7 DpXTRYnfn/53dQwjFFzckYnK2RtCPB1glXtac9r98ZmvNenduLs5/ERKCthg+JWCZAB8 o4Kg== X-Received: by 10.194.122.231 with SMTP id lv7mr14249968wjb.27.1413394668089; Wed, 15 Oct 2014 10:37:48 -0700 (PDT) Received: from debian-vm3.lan (bl9-95-210.dsl.telepac.pt. [85.242.95.210]) by mx.google.com with ESMTPSA id gt7sm20093694wib.18.2014.10.15.10.37.46 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Wed, 15 Oct 2014 10:37:46 -0700 (PDT) From: Filipe Manana X-Google-Original-From: Filipe Manana To: linux-btrfs@vger.kernel.org Cc: stable@vger.kernel.org, clm@fb.com, Filipe Manana Subject: [PATCH] Revert "Btrfs: race free update of commit root for ro snapshots" Date: Wed, 15 Oct 2014 19:37:26 +0100 Message-Id: <1413398246-17324-1-git-send-email-fdmanana@suse.com> X-Mailer: git-send-email 1.9.1 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.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable 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 This reverts commit 9c3b306e1c9e6be4be09e99a8fe2227d1005effc. Switching only one commit root during a transaction is wrong because it leads the fs into an inconsistent state. All commit roots should be switched at once, at transaction commit time, otherwise backref walking can often miss important references that were only accessible through the old commit root. Plus, the root item for the snapshot's root wasn't getting updated and preventing the next transaction commit to do it. This made several users get into random corruption issues after creation of readonly snapshots. A regression test for xfstests will follow soon. Cc: stable@vger.kernel.org # 3.17 Signed-off-by: Filipe Manana --- fs/btrfs/inode.c | 36 ------------------------------------ fs/btrfs/ioctl.c | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 36 deletions(-) diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index fc9c043..d23362f 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -5261,42 +5261,6 @@ struct inode *btrfs_lookup_dentry(struct inode *dir, struct dentry *dentry) iput(inode); inode = ERR_PTR(ret); } - /* - * If orphan cleanup did remove any orphans, it means the tree - * was modified and therefore the commit root is not the same as - * the current root anymore. This is a problem, because send - * uses the commit root and therefore can see inode items that - * don't exist in the current root anymore, and for example make - * calls to btrfs_iget, which will do tree lookups based on the - * current root and not on the commit root. Those lookups will - * fail, returning a -ESTALE error, and making send fail with - * that error. So make sure a send does not see any orphans we - * have just removed, and that it will see the same inodes - * regardless of whether a transaction commit happened before - * it started (meaning that the commit root will be the same as - * the current root) or not. - */ - if (sub_root->node != sub_root->commit_root) { - u64 sub_flags = btrfs_root_flags(&sub_root->root_item); - - if (sub_flags & BTRFS_ROOT_SUBVOL_RDONLY) { - struct extent_buffer *eb; - - /* - * Assert we can't have races between dentry - * lookup called through the snapshot creation - * ioctl and the VFS. - */ - ASSERT(mutex_is_locked(&dir->i_mutex)); - - down_write(&root->fs_info->commit_root_sem); - eb = sub_root->commit_root; - sub_root->commit_root = - btrfs_root_node(sub_root); - up_write(&root->fs_info->commit_root_sem); - free_extent_buffer(eb); - } - } } return inode; diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index e732274..33c80f5 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -713,6 +713,39 @@ static int create_snapshot(struct btrfs_root *root, struct inode *dir, if (ret) goto fail; + ret = btrfs_orphan_cleanup(pending_snapshot->snap); + if (ret) + goto fail; + + /* + * If orphan cleanup did remove any orphans, it means the tree was + * modified and therefore the commit root is not the same as the + * current root anymore. This is a problem, because send uses the + * commit root and therefore can see inode items that don't exist + * in the current root anymore, and for example make calls to + * btrfs_iget, which will do tree lookups based on the current root + * and not on the commit root. Those lookups will fail, returning a + * -ESTALE error, and making send fail with that error. So make sure + * a send does not see any orphans we have just removed, and that it + * will see the same inodes regardless of whether a transaction + * commit happened before it started (meaning that the commit root + * will be the same as the current root) or not. + */ + if (readonly && pending_snapshot->snap->node != + pending_snapshot->snap->commit_root) { + trans = btrfs_join_transaction(pending_snapshot->snap); + if (IS_ERR(trans) && PTR_ERR(trans) != -ENOENT) { + ret = PTR_ERR(trans); + goto fail; + } + if (!IS_ERR(trans)) { + ret = btrfs_commit_transaction(trans, + pending_snapshot->snap); + if (ret) + goto fail; + } + } + inode = btrfs_lookup_dentry(dentry->d_parent->d_inode, dentry); if (IS_ERR(inode)) { ret = PTR_ERR(inode);