From patchwork Thu Jan 16 16:44:11 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "J. Bruce Fields" X-Patchwork-Id: 3499591 Return-Path: X-Original-To: patchwork-linux-nfs@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 0CDB8C02DC for ; Thu, 16 Jan 2014 16:44:19 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id B4F3120171 for ; Thu, 16 Jan 2014 16:44:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AD3A92010B for ; Thu, 16 Jan 2014 16:44:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752277AbaAPQoQ (ORCPT ); Thu, 16 Jan 2014 11:44:16 -0500 Received: from fieldses.org ([174.143.236.118]:52823 "EHLO fieldses.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751970AbaAPQoP (ORCPT ); Thu, 16 Jan 2014 11:44:15 -0500 Received: from bfields by fieldses.org with local (Exim 4.76) (envelope-from ) id 1W3q39-0004g7-Mq; Thu, 16 Jan 2014 11:44:11 -0500 Date: Thu, 16 Jan 2014 11:44:11 -0500 From: "J. Bruce Fields" To: Steven Whitehouse Cc: Al Viro , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-nfs@vger.kernel.org, Miklos Szeredi Subject: Re: [PATCH] dcache: fix d_splice_alias handling of aliases Message-ID: <20140116164411.GD16829@fieldses.org> References: <20140115151749.GF23999@fieldses.org> <20140116161015.GC16829@fieldses.org> <1389888942.2779.34.camel@menhir> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <1389888942.2779.34.camel@menhir> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-7.2 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 Thu, Jan 16, 2014 at 04:15:42PM +0000, Steven Whitehouse wrote: > Hi, > > On Thu, 2014-01-16 at 11:10 -0500, J. Bruce Fields wrote: > > On Wed, Jan 15, 2014 at 10:17:49AM -0500, bfields wrote: > > > From: "J. Bruce Fields" > > > > > > d_splice_alias can create duplicate directory aliases (in the !new > > > case), or (in the new case) d_move without holding appropriate locks. > > > > > > d_materialise_unique deals with both of these problems. (The latter > > > seems to be dealt by trylocks (see __d_unalias), which look like they > > > could cause spurious lookup failures--but that's at least better than > > > corrupting the dcache.) > > > > > > Signed-off-by: J. Bruce Fields > > > --- > > > fs/dcache.c | 25 +------------------------ > > > 1 file changed, 1 insertion(+), 24 deletions(-) > > > > > > Only lightly tested.... If this is right, then we can also just ditch > > > d_splice_alias completely, and clean up the various d_find_alias's. > > > > > > I think the only reason we have both d_splice_alias and > > > d_materialise_unique is that the former was written for exportable > > > filesystems and the latter for distributed filesystems. > > > > > > But we have at least one exportable filesystem (fuse) using > > > d_materialise_unique. And I doubt d_splice_alias was ever completely > > > correct even for on-disk filesystems. > > > > > > Am I missing some subtlety? > > > > Hm, I just noticed: > > > > commit 0d0d110720d7960b77c03c9f2597faaff4b484ae > > Author: Miklos Szeredi > > Date: Mon Sep 16 14:52:00 2013 +0200 > > > > GFS2: d_splice_alias() can't return error > > > > unless it was given an IS_ERR(inode), which isn't the case here. So clean > > up the unnecessary error handling in gfs2_create_inode(). > > > > This paves the way for real fixes (hence the stable Cc). > > > > Signed-off-by: Miklos Szeredi > > Signed-off-by: Steven Whitehouse > > Cc: stable@vger.kernel.org > > > > While the statement is true for the current implementation of > > d_splice_alias, I don't think it's actually true for any correct > > implementation of d_splice_alias, which must be able to return at least > > -ELOOP in the directory case. Does gfs2 need fixing? > > > > --b. > > Yes, in that case, probably in two places, Something like this? (Except: is the inode cleanup right in the first chunk? And in the second chunk the cleanup could maybe be organized better even if I got it right....) --b. --- To unsubscribe from this list: send the line "unsubscribe linux-nfs" 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/fs/gfs2/inode.c b/fs/gfs2/inode.c index 7119504..19e0924 100644 --- a/fs/gfs2/inode.c +++ b/fs/gfs2/inode.c @@ -585,6 +585,9 @@ static int gfs2_create_inode(struct inode *dir, struct dentry *dentry, error = PTR_ERR(inode); if (!IS_ERR(inode)) { d = d_splice_alias(inode, dentry); + error = PTR_ERR(d); + if (IS_ERR(d)) + goto fail_gunlock; error = 0; if (file) { if (S_ISREG(inode->i_mode)) { @@ -779,6 +782,11 @@ static struct dentry *__gfs2_lookup(struct inode *dir, struct dentry *dentry, } d = d_splice_alias(inode, dentry); + if (IS_ERR(d)) { + iput(inode); + gfs2_glock_dq_uninit(&gh); + return ERR_PTR(error); + } if (file && S_ISREG(inode->i_mode)) error = finish_open(file, dentry, gfs2_open_common, opened);