From patchwork Mon Jul 14 13:00:28 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jeff Layton X-Patchwork-Id: 4546121 Return-Path: X-Original-To: patchwork-linux-nfs@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 197F89F295 for ; Mon, 14 Jul 2014 13:00:48 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 0C9D020136 for ; Mon, 14 Jul 2014 13:00:47 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id D7DB42014A for ; Mon, 14 Jul 2014 13:00:44 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754876AbaGNNAe (ORCPT ); Mon, 14 Jul 2014 09:00:34 -0400 Received: from mail-qa0-f52.google.com ([209.85.216.52]:56989 "EHLO mail-qa0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754847AbaGNNAb (ORCPT ); Mon, 14 Jul 2014 09:00:31 -0400 Received: by mail-qa0-f52.google.com with SMTP id j15so3124298qaq.25 for ; Mon, 14 Jul 2014 06:00:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:date:to:cc:subject:message-id:in-reply-to :references:mime-version:content-type; bh=T4RrofXFQg0vRIQoJy0hHYjF6IQnf+tI6fRAxjm69Zs=; b=WmdDer91432kw/J8lvY3+gXIQPFuub7K5nVnpSKaQrBdG6/UDVNPfj4vuVvaF1ELMY rkK+jI57gyv0AZ4cuGFTj0s0TUNu9iOyX4tyX5KvOO29Ir+qPrIpV2joIxdSSRav+2ab k4Sy1i7i8Hoy8N7QdzVjyfha4bqrIuASxFk4A214rfREpNKZNu7flB5bo5MtfteaMcHg uR/P5lgOFsXvvHe6XzlbQkSd1PLAoKa1+2uLZ9MVaLNkaK8AKBmu/kh20xChY9gi63px DgIkDZvgP2fUW5L+U/3tzP9npMzcK2CR9N4BEdkMOJdGwwvYzqmeHOghaEbiKy4nrLMX CMfA== X-Gm-Message-State: ALoCoQllZagIT3IEdBPYQLRDZrMMkW6+0mfdCYq8Of25/lmAvYbfR6tydkuTgbaYuRA6pQbGL67a X-Received: by 10.224.128.9 with SMTP id i9mr22237535qas.50.1405342830784; Mon, 14 Jul 2014 06:00:30 -0700 (PDT) Received: from tlielax.poochiereds.net ([2001:470:8:d63:3a60:77ff:fe93:a95d]) by mx.google.com with ESMTPSA id o10sm20312086qat.0.2014.07.14.06.00.30 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 14 Jul 2014 06:00:30 -0700 (PDT) From: Jeff Layton X-Google-Original-From: Jeff Layton Date: Mon, 14 Jul 2014 09:00:28 -0400 To: NeilBrown Cc: Jeff Layton , Trond Myklebust , Alexander Viro , NFS Subject: Re: [PATCH] NFS: nfs4_lookup_revalidate need to report STALE inodes. Message-ID: <20140714090028.6f04fd2c@tlielax.poochiereds.net> In-Reply-To: <20140714223513.47807c98@notabene.brown> References: <20140714151405.2fa06dd7@notabene.brown> <20140714081455.69f55224@tlielax.poochiereds.net> <20140714223513.47807c98@notabene.brown> X-Mailer: Claws Mail 3.10.1 (GTK+ 2.24.22; x86_64-redhat-linux-gnu) MIME-Version: 1.0 Sender: linux-nfs-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-nfs@vger.kernel.org X-Spam-Status: No, score=-6.9 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_HI, RP_MATCHES_RCVD, T_TVD_MIME_EPI, 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 Mon, 14 Jul 2014 22:35:13 +1000 NeilBrown wrote: > On Mon, 14 Jul 2014 08:14:55 -0400 Jeff Layton > wrote: > > > On Mon, 14 Jul 2014 15:14:05 +1000 > > NeilBrown wrote: > > > > > > > > If an 'open' of a file in an NFSv4 filesystem finds that the dentry is > > > in cache, but the inode is stale (on the server), the dentry will not > > > be re-validated immediately and may cause ESTALE to be returned to > > > user-space. > > > > > > For a non-create 'open', do_last() calls lookup_fast() and on success > > > will eventually call may_open() which calls into nfs_permission(). > > > If nfs_permission() makes the ACCESS call to the server it will get > > > NFS4ERR_STALE, resulting in ESTALE from may_open() and thence from > > > do_last(). > > > The retry-on-ESTALE in filename_lookup() will repeat exactly the same > > > process because nothing in this path will invalidate the dentry due to > > > the inode being stale, so the ESTALE will be returned. > > > > > > lookup_fast() calls ->d_revalidate(), but for an OPEN on an NFSv4 > > > filesystem, that will succeed for regular files: > > > /* Let f_op->open() actually open (and revalidate) the file */ > > > > > > Unfortunately in the case of a STALE inode, f_op->open() never gets > > > called. If we teach nfs4_lookup_revalidate() to report a failure on > > > NFS_STALE() inodes, then the dentry will be invalidated and a full > > > lookup will be attempted. The ESTALE errors go away. > > > > > > > > > While I think this fix is correct, I'm not convinced that it is > > > sufficient, particularly if lookupcache=none. > > > The current code will fail an "open" is nfs_permission() fails, > > > without having performed a LOOKUP. i.e. it will use the cache. > > > nfs_lookup_revalidate will force a lookup before the permission check > > > if NFS_MOUNT_LOOKUP_CACHE_NONE, but nfs4_lookup_revalidate will not. > > > > > > > This patch should make the code fall through to nfs_lookup_revalidate, > > which would then force the lookup, right? > > Yes ... though maybe that's not what I really want to do. I really wanted to > just return '0', though I would need to check that is right in all cases. > > > > > Also, I'm a little unclear... > > > > Why would may_open fail with ESTALE after the v4 OPEN succeeds? The > > OPEN should be returning a filehandle and attributes for the inode > > actually opened. It seems like we ought to be doing any permission > > checks vs. that inode, not anything we had in cache. Presumably the > > server is then holding it open so it shouldn't be stale. > > may_open is called *before* and v4 OPEN. > > In do_last, if the inode is already in cache, then > lookup_fast is called, which calls d_revalidate > then may_open (calls ->permission) > then finish_open which calls f_op->open > > Yes, we should be doing permission checking against whatever 'open' finds. > But the VFS is structured to the the permission check after d_revalidate and > before ->open. So maybe d_revalidate needs to do the NFS open?? > Ok, I see. Ugh, having the revalidate do the open sounds...messy. A simpler fix might be to fix it so that an -ESTALE return from may_open triggers a retry. Something like this maybe (probably whitespace damaged, so just for discussion purposes): ...though might need to convert the ESTALE to EOPENSTALE there too, not sure... > > > > > Are we not properly updating the dcache (and attrcache) after the > > OPEN reply? > > I think so, yes. But in the problem case, we don't even send an OPEN > request. > > > > > > > > > > Signed-off-by: NeilBrown > > > > > > diff --git a/fs/nfs/dir.c b/fs/nfs/dir.c > > > index 4a3d4ef76127..4f7414afca27 100644 > > > --- a/fs/nfs/dir.c > > > +++ b/fs/nfs/dir.c > > > @@ -1563,6 +1563,8 @@ static int nfs4_lookup_revalidate(struct > > > dentry *dentry, unsigned int flags) /* We cannot do exclusive > > > creation on a positive dentry */ if (flags & LOOKUP_EXCL) > > > goto no_open_dput; > > > + if (NFS_STALE(inode)) > > > + goto no_open_dput; > > > > > > /* Let f_op->open() actually open (and revalidate) the > > > file */ ret = 1; > > > > Looks legit to me too, but it seems like the inode could go stale > > w/o us knowing after this point. > > > > Acked-by: Jeff Layton > > Thanks, > NeilBrown diff --git a/fs/namei.c b/fs/namei.c index 985c6f368485..c1657deea52c 100644 --- a/fs/namei.c +++ b/fs/namei.c @@ -3045,8 +3045,13 @@ finish_open: } finish_open_created: error = may_open(&nd->path, acc_mode, open_flag); - if (error) + if (error) { + if (error == -ESTALE) + goto stale_open; goto out; + } file->f_path.mnt = nd->path.mnt; error = finish_open(file, nd->path.dentry, NULL, opened); if (error) {