From patchwork Wed Mar 9 00:34:19 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Al Viro X-Patchwork-Id: 8540481 Return-Path: X-Original-To: patchwork-linux-fsdevel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id 1F21E9F372 for ; Wed, 9 Mar 2016 01:01:05 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0FE4C2017D for ; Wed, 9 Mar 2016 01:01:04 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id AEAB220121 for ; Wed, 9 Mar 2016 01:01:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752932AbcCIBAj (ORCPT ); Tue, 8 Mar 2016 20:00:39 -0500 Received: from zeniv.linux.org.uk ([195.92.253.2]:36026 "EHLO ZenIV.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751755AbcCIAeY (ORCPT ); Tue, 8 Mar 2016 19:34:24 -0500 Received: from viro by ZenIV.linux.org.uk with local (Exim 4.76 #1 (Red Hat Linux)) id 1adS4x-0003pZ-6X; Wed, 09 Mar 2016 00:34:19 +0000 Date: Wed, 9 Mar 2016 00:34:19 +0000 From: Al Viro To: "Drokin, Oleg" Cc: "Dilger, Andreas" , Linus Torvalds , "" Subject: Re: races in ll_splice_alias() Message-ID: <20160309003416.GY17997@ZenIV.linux.org.uk> References: <20160308160537.GV17997@ZenIV.linux.org.uk> <498D5A19-9E55-48D7-B5CF-34CA5769FF7F@intel.com> <20160308211148.GX17997@ZenIV.linux.org.uk> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-fsdevel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-fsdevel@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=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 Tue, Mar 08, 2016 at 11:18:09PM +0000, Drokin, Oleg wrote: > Rename on server cannot get us to see the same directory in two places, or what's > the scenario? > In Lustre: > thread1: lookup a directory on the server. > get a lock back > instantiate a dentry (guarded by the lock) > make a lock revocable (ll_lookup_finish_locks() in ll_lookup_it()) > thread2: rename the directory moving it somewhere else > server attempts to revoke the lock from thread1 > node that runs thread1 drops the lock and marks all dentries for that > inode invalid > server completes rename and releases the lock it holds > thread3: lookup the directory under new name on the server > this can only return from server when the rename has completed and the > dentry from thread1 marked invalid. thread2 might run on another client; might or might not make any difference, but in any case server going nuts shouldn't corrupt data structures on client... > Ok, let me try my hand at that. Hopefully whatever complications are there would > show themselves right away too. > > > would always either inserted inode reference into a new dentry or dropped it. > > I'm still trying to trace what does iput() in case of error in your current > > code; I understand the one in do_statahead_enter(), but what does it in > > ll_lookup_it_finish()? > > You mean when ll_d_init() fails in ll_splice_alias()? > Hm… It appears that we are indeed leaking the inode in that case, thanks. > I'll try to address that too. > Hm, in fact this was almost noticed, but I guess nobody retested it after > fixing the initial crash we had with 7486bc06ab2c46d6957f0211d09bc549aaf9cc87 If that's the case, I'd try this (on top of the patch from upthread): --- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" 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/drivers/staging/lustre/lustre/llite/namei.c b/drivers/staging/lustre/lustre/llite/namei.c index da5f443..bcc9841 100644 --- a/drivers/staging/lustre/lustre/llite/namei.c +++ b/drivers/staging/lustre/lustre/llite/namei.c @@ -320,81 +320,37 @@ void ll_i2gids(__u32 *suppgids, struct inode *i1, struct inode *i2) } /* - * try to reuse three types of dentry: - * 1. unhashed alias, this one is unhashed by d_invalidate (but it may be valid - * by concurrent .revalidate). - * 2. INVALID alias (common case for no valid ldlm lock held, but this flag may - * be cleared by others calling d_lustre_revalidate). - * 3. DISCONNECTED alias. - */ -static struct dentry *ll_find_alias(struct inode *inode, struct dentry *dentry) -{ - struct dentry *alias, *discon_alias, *invalid_alias; - - if (hlist_empty(&inode->i_dentry)) - return NULL; - - discon_alias = invalid_alias = NULL; - - ll_lock_dcache(inode); - hlist_for_each_entry(alias, &inode->i_dentry, d_u.d_alias) { - LASSERT(alias != dentry); - - spin_lock(&alias->d_lock); - if (alias->d_flags & DCACHE_DISCONNECTED) - /* LASSERT(last_discon == NULL); LU-405, bz 20055 */ - discon_alias = alias; - else if (alias->d_parent == dentry->d_parent && - alias->d_name.hash == dentry->d_name.hash && - alias->d_name.len == dentry->d_name.len && - memcmp(alias->d_name.name, dentry->d_name.name, - dentry->d_name.len) == 0) - invalid_alias = alias; - spin_unlock(&alias->d_lock); - - if (invalid_alias) - break; - } - alias = invalid_alias ?: discon_alias ?: NULL; - if (alias) { - spin_lock(&alias->d_lock); - dget_dlock(alias); - spin_unlock(&alias->d_lock); - } - ll_unlock_dcache(inode); - - return alias; -} - -/* * Similar to d_splice_alias(), but lustre treats invalid alias * similar to DCACHE_DISCONNECTED, and tries to use it anyway. */ struct dentry *ll_splice_alias(struct inode *inode, struct dentry *de) { - struct dentry *new; + struct dentry *alias = NULL; int rc; if (inode) { - new = ll_find_alias(inode, de); - if (new) { - rc = ll_d_init(new); - if (rc < 0) { - dput(new); - return ERR_PTR(rc); - } - d_move(new, de); + alias = d_exact_alias(de, inode); + if (alias) + iput(inode); + } + + if (!alias) { + rc = ll_d_init(de); + if (rc < 0) { iput(inode); - CDEBUG(D_DENTRY, - "Reuse dentry %p inode %p refc %d flags %#x\n", - new, d_inode(new), d_count(new), new->d_flags); - return new; + return ERR_PTR(rc); } + alias = d_splice_alias(inode, de); + if (IS_ERR(alias)) + return alias; + } + + if (alias) { + CDEBUG(D_DENTRY, + "Reuse dentry %p inode %p refc %d flags %#x\n", + alias, d_inode(alias), d_count(alias), alias->d_flags); + return alias; } - rc = ll_d_init(de); - if (rc < 0) - return ERR_PTR(rc); - d_add(de, inode); CDEBUG(D_DENTRY, "Add dentry %p inode %p refc %d flags %#x\n", de, d_inode(de), d_count(de), de->d_flags); return de; diff --git a/drivers/staging/lustre/lustre/llite/statahead.c b/drivers/staging/lustre/lustre/llite/statahead.c index 88ffd8e..6c64de0 100644 --- a/drivers/staging/lustre/lustre/llite/statahead.c +++ b/drivers/staging/lustre/lustre/llite/statahead.c @@ -1576,6 +1576,7 @@ int do_statahead_enter(struct inode *dir, struct dentry **dentryp, alias = ll_splice_alias(inode, *dentryp); if (IS_ERR(alias)) { + entry->se_inode = NULL; ll_sai_unplug(sai, entry); return PTR_ERR(alias); }