From patchwork Wed Jun 3 00:19:14 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Omar Sandoval X-Patchwork-Id: 6531741 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.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 9C20CC0020 for ; Wed, 3 Jun 2015 00:19:27 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id A308A2068A for ; Wed, 3 Jun 2015 00:19:26 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 81A282068D for ; Wed, 3 Jun 2015 00:19:25 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751378AbbFCATT (ORCPT ); Tue, 2 Jun 2015 20:19:19 -0400 Received: from mail-pd0-f175.google.com ([209.85.192.175]:35536 "EHLO mail-pd0-f175.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751597AbbFCATR (ORCPT ); Tue, 2 Jun 2015 20:19:17 -0400 Received: by pdbnf5 with SMTP id nf5so82763302pdb.2 for ; Tue, 02 Jun 2015 17:19:16 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-type:content-disposition:in-reply-to :user-agent; bh=fxEJw3Fh0du1SZeiGJGv5yChYvEaqwgkRRUIY5LabZo=; b=QrkF+tQNLSpUBfBWKGpZ6/F0N9jlSFjWFk7MqQ1RlxOfuVfjU0sfVeayxSCnDMOdVF fxH28UD6q8YjdVRPdj7P2netjbufQB0aFKevPD0yYs0FKXLqwYZmcSgRRVSiVEEoyv2t bYOrERJqPWivMwzThQpODoL7a0q2YJzz+yw3qXyo/FYEB1g6f5+G7aH6y2lWltcwgvGf pD2j4w7b1w/aBoMfifS0K7flLNXG86B6nv5doiRNLYyZv307Qpxv0iQRYvhhUzXirQBn Gzw1uCRjbG3reNJQibX9fC1wSfYppmExAfs0Inx8p2xYKVGvTJDZbjQvaDeU5dXierdR QsNw== X-Gm-Message-State: ALoCoQkIyd9f/FDXqXEWY7V/0KZbjQ17I/x0aJx0dIDCwQItlh+4Q+8DTITCvmrzQ8KMzCrtSaJI X-Received: by 10.68.68.203 with SMTP id y11mr16859174pbt.34.1433290756751; Tue, 02 Jun 2015 17:19:16 -0700 (PDT) Received: from mew (c-76-104-211-44.hsd1.wa.comcast.net. [76.104.211.44]) by mx.google.com with ESMTPSA id hj11sm18649228pbd.33.2015.06.02.17.19.15 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 02 Jun 2015 17:19:15 -0700 (PDT) Date: Tue, 2 Jun 2015 17:19:14 -0700 From: Omar Sandoval To: Filipe David Manana Cc: "linux-btrfs@vger.kernel.org" , Markus Schauler , stable@vger.kernel.org Subject: Re: [PATCH] Btrfs: don't invalidate root dentry when subvolume deletion fails Message-ID: <20150603001914.GA21606@mew> References: MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) 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, 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 On Mon, Jun 01, 2015 at 05:56:43PM +0100, Filipe David Manana wrote: > On Sat, May 30, 2015 at 9:59 AM, Omar Sandoval wrote: > > Since commit bafc9b754f75 ("vfs: More precise tests in d_invalidate"), > > mounted subvolumes can be deleted because d_invalidate() won't fail. > > However, we run into problems when we attempt to delete the default > > subvolume while it is mounted as the root filesystem: > > > > # btrfs subvol list / > > ID 257 gen 306 top level 5 path rootvol > > ID 267 gen 334 top level 5 path snap1 > > # btrfs subvol get-default / > > ID 267 gen 334 top level 5 path snap1 > > # btrfs inspect-internal rootid / > > 267 > > # mount -o subvol=/ /dev/vda1 /mnt > > # btrfs subvol del /mnt/snap1 > > Delete subvolume (no-commit): '/mnt/snap1' > > ERROR: cannot delete '/mnt/snap1' - Operation not permitted > > # findmnt / > > findmnt: can't read /proc/mounts: No such file or directory > > # ls /proc > > # > > > > Markus reported that this same scenario simply led to a kernel oops. > > > > This happens because in btrfs_ioctl_snap_destroy(), we call > > d_invalidate() before we check may_destroy_subvol(), which means that we > > detach the submounts and drop the dentry before erroring out. Instead, > > we should only invalidate the dentry once we know that we're going > > through with the deletion. > > > > Cc: > > Fixes: bafc9b754f75 ("vfs: More precise tests in d_invalidate") > > Reported-by: Markus Schauler > > Signed-off-by: Omar Sandoval > > --- > > The other fix for preventing all mounted subvolumes from being deleted > > would preclude this, but it sounded like we were leaning towards > > enforcing that in userspace once subvolume info becomes available in > > /proc/mounts, so this should be fixed separately. > > > > fs/btrfs/ioctl.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > > index 1c22c6518504..8edb8544088b 100644 > > --- a/fs/btrfs/ioctl.c > > +++ b/fs/btrfs/ioctl.c > > @@ -2413,14 +2413,14 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, > > goto out_unlock_inode; > > } > > > > - d_invalidate(dentry); > > - > > down_write(&root->fs_info->subvol_sem); > > > > err = may_destroy_subvol(dest); > > if (err) > > goto out_up_write; > > > > + d_invalidate(dentry); > > + > > Any reason why not calling d_invalidate() only if the call > btrfs_unlink_subvol() succeeds? Not seeing a reason why we should > invalidate before doing the actual deletion successfully (before that > metadata reservation can fail or failure to start/join a transaction, > etc). Good point, it's probably best to put it here: ---- ---- I also can't figure out what that shrink_dcache_sb() is doing there. d_invalidate() already prunes the dentry cache under the deleted subvolume, but this clears the dcache for the whole filesystem, which could incur unnecessary overhead. The call was added by efefb1438be2 ("Btrfs: remove negative dentry when deleting subvolumne"), which fixes a problem in btrfs_dentry_delete(), but the commit message doesn't explain what shrink_dcache_sb() had to do with it. I'll send in an updated version with d_invalidate() moved and shrink_dcache_sb() removed and see if anyone can enlighten me. > Also, would you consider making an xfstest for this? No problem. > thanks Thanks for the review! diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 1c22c6518504..5a225cd0af65 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -2413,8 +2413,6 @@ static noinline int btrfs_ioctl_snap_destroy(struct file *file, goto out_unlock_inode; } - d_invalidate(dentry); - down_write(&root->fs_info->subvol_sem); err = may_destroy_subvol(dest); @@ -2508,6 +2506,7 @@ out_up_write: out_unlock_inode: mutex_unlock(&inode->i_mutex); if (!err) { + d_invalidate(dentry); shrink_dcache_sb(root->fs_info->sb); btrfs_invalidate_inodes(dest); d_delete(dentry);