From patchwork Thu Dec 8 05:01:55 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Oleg Drokin X-Patchwork-Id: 9465829 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id CDBF76071E for ; Thu, 8 Dec 2016 05:03:51 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id C707C28529 for ; Thu, 8 Dec 2016 05:03:51 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id B86A828548; Thu, 8 Dec 2016 05:03:51 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.9 required=2.0 tests=BAYES_00,RCVD_IN_DNSWL_HI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id A0CDA28529 for ; Thu, 8 Dec 2016 05:03:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750758AbcLHFDs convert rfc822-to-8bit (ORCPT ); Thu, 8 Dec 2016 00:03:48 -0500 Received: from linuxhacker.ru ([217.76.32.60]:35802 "EHLO fiona.linuxhacker.ru" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750729AbcLHFDr (ORCPT ); Thu, 8 Dec 2016 00:03:47 -0500 Received: from [192.168.1.140] (c-73-108-203-87.hsd1.tn.comcast.net [73.108.203.87]) (authenticated bits=0) by fiona.linuxhacker.ru (8.15.2/8.14.7) with ESMTPSA id uB851uxE024782 (version=TLSv1 cipher=AES128-SHA bits=128 verify=NO); Thu, 8 Dec 2016 08:01:58 +0300 Subject: Re: Revalidate failure leads to unmount Mime-Version: 1.0 (Apple Message framework v1283) From: Oleg Drokin In-Reply-To: <20161206061747.GN1555@ZenIV.linux.org.uk> Date: Thu, 8 Dec 2016 00:01:55 -0500 Cc: linux-fsdevel , Trond Myklebust , List Linux NFS Mailing , "Eric W. Biederman" Message-Id: References: <37A073FB-726E-4AF8-BC61-0DFBA6C51BD7@linuxhacker.ru> <5B453EA9-676D-4240-BF2F-4827188962E4@linuxhacker.ru> <20161206020059.GL1555@ZenIV.linux.org.uk> <02B48074-7E2E-4DB5-9A88-4FD4E37088FA@linuxhacker.ru> <20161206050254.GM1555@ZenIV.linux.org.uk> <20161206061747.GN1555@ZenIV.linux.org.uk> To: Al Viro X-Mailer: Apple Mail (2.1283) Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Dec 6, 2016, at 1:17 AM, Al Viro wrote: > On Tue, Dec 06, 2016 at 12:45:11AM -0500, Oleg Drokin wrote: > >> Well, certainly if d_splice_alias was working like that so that even non-directory >> dentry would find an alias (not necessarily unhashed even) for that same inode and use that instead, that would make ll_splice_alias/ll_find_alias unnecessary. >> >> We still retain the weird d_compare() that rejects otherwise perfectly valid aliases >> if the lock guarding them is gone, triggering relookup (and necessiating the >> above logic to pick up just rejected alias again now that we have the lock again). > > Why not have ->d_revalidate() kick them, instead? _IF_ we have a way to > do unhash-and-trigger-lookup that way, do you really need those games with > ->d_compare()? Ok, so this does appear to work in mainline, but not in the well known vendor kernels, where they still live in the past with d_invalidate returning non zero and lookup_dcache() dutifuly ignoring the error code from revalidate. Anyway, I can submit the patch doing away with ll_dcompare, but I still need the kludge to always return 1 when revalidating mountpoints, otherwise they would be constantly unmounted, I guess. Is this something you'd like to carry along with the rest of (yet to be) patches that only unmount stuff on lookup failure/different result as we discussed before, or should I shoot this to Greg right away? The patch pretty much amounts to this now: --- 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/drivers/staging/lustre/lustre/llite/dcache.c b/drivers/staging/lustre/lustre/llite/dcache.c index 0e45d8f..f532167 100644 --- a/drivers/staging/lustre/lustre/llite/dcache.c +++ b/drivers/staging/lustre/lustre/llite/dcache.c @@ -69,38 +69,6 @@ static void ll_release(struct dentry *de) call_rcu(&lld->lld_rcu_head, free_dentry_data); } -/* Compare if two dentries are the same. Don't match if the existing dentry - * is marked invalid. Returns 1 if different, 0 if the same. - * - * This avoids a race where ll_lookup_it() instantiates a dentry, but we get - * an AST before calling d_revalidate_it(). The dentry still exists (marked - * INVALID) so d_lookup() matches it, but we have no lock on it (so - * lock_match() fails) and we spin around real_lookup(). - */ -static int ll_dcompare(const struct dentry *dentry, - unsigned int len, const char *str, - const struct qstr *name) -{ - if (len != name->len) - return 1; - - if (memcmp(str, name->name, len)) - return 1; - - CDEBUG(D_DENTRY, "found name %.*s(%p) flags %#x refc %d\n", - name->len, name->name, dentry, dentry->d_flags, - d_count(dentry)); - - /* mountpoint is always valid */ - if (d_mountpoint((struct dentry *)dentry)) - return 0; - - if (d_lustre_invalid(dentry)) - return 1; - - return 0; -} - /** * Called when last reference to a dentry is dropped and dcache wants to know * whether or not it should cache it: @@ -255,6 +223,15 @@ static int ll_revalidate_dentry(struct dentry *dentry, { struct inode *dir = d_inode(dentry->d_parent); + /* mountpoint is always valid */ + if (d_mountpoint((struct dentry *)dentry)) + return 1; + + /* No lock? Bail out */ + if (d_lustre_invalid(dentry)) + return 0; + + /* If this is intermediate component path lookup and we were able to get * to this dentry, then its lock has not been revoked and the * path component is valid. @@ -303,5 +280,4 @@ const struct dentry_operations ll_d_ops = { .d_revalidate = ll_revalidate_nd, .d_release = ll_release, .d_delete = ll_ddelete, - .d_compare = ll_dcompare, };