From patchwork Tue Apr 15 14:24:52 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Trond Myklebust X-Patchwork-Id: 3993391 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 02BDE9F336 for ; Tue, 15 Apr 2014 14:25:01 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id F3D202022A for ; Tue, 15 Apr 2014 14:24:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id E2545201B4 for ; Tue, 15 Apr 2014 14:24:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751352AbaDOOY5 (ORCPT ); Tue, 15 Apr 2014 10:24:57 -0400 Received: from mail-ob0-f173.google.com ([209.85.214.173]:32978 "EHLO mail-ob0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751037AbaDOOY4 (ORCPT ); Tue, 15 Apr 2014 10:24:56 -0400 Received: by mail-ob0-f173.google.com with SMTP id wn1so3275501obc.4 for ; Tue, 15 Apr 2014 07:24:55 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:organization:content-type:mime-version :content-transfer-encoding; bh=O9nqkSPYz6TrLovNe5ORjUlpij9MDqsOeRFG58YaFCI=; b=ikfT6vGGxXrKF5L6sQGZxqv6ODOPINSsYYVbv1Ikh4Qgki8UWLFnaOJEJtxPkHoVw3 HXAqxkd5rNJCAmORTMcsYNuDZAeQlsOBMoapmRIio5dlyJskjLSnpdh6aOPaIjpaUHMC dznw1V/O6HB8SN6wMVwXTTQARN5H5YK2WOT0NAAdnXw+PSQjcG0XppqwUehCQDTEa6KU AnqCOE50pLXOVE0IGF5Vs1tyEyQWx+1j7ZKEMW2kfPZ2DYVS3N2mdeNxNmxE0NYWEKoN CyoFLJxSpM1RBumL0l3LdBMtA2bTOxFJAaRwfH2j3wh1jJRfNZewVTDXmul2Epf6wPWG S/mA== X-Gm-Message-State: ALoCoQl4ip5ciIPTgbRBvMYlTTs8PGA91INi4d8DIB3C2LdAIpUlRPAqyrimFBUzESjxTjgXAkxj X-Received: by 10.183.1.34 with SMTP id bd2mr1799635obd.4.1397571895874; Tue, 15 Apr 2014 07:24:55 -0700 (PDT) Received: from [172.16.74.156] (50-124-254-125.drr02.mskg.mi.frontiernet.net. [50.124.254.125]) by mx.google.com with ESMTPSA id e8sm86119704oed.7.2014.04.15.07.24.53 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 15 Apr 2014 07:24:54 -0700 (PDT) Message-ID: <1397571892.4709.1.camel@leira.trondhjem.org> Subject: Re: [PATCH] NFS: add FATTR4_WORD1_MODE flags for cache_consistency_bitmask From: Trond Myklebust To: Kinglong Mee Cc: linux-nfs@vger.kernel.org, Dr Fields James Bruce Date: Tue, 15 Apr 2014 10:24:52 -0400 In-Reply-To: <1FB7545D-81C5-46D5-9A42-322553F122D8@primarydata.com> References: <534A8CEF.6000609@gmail.com> <505345D4-4F7C-4970-9EBF-7CFC367575B2@primarydata.com> <534AA4FD.3060202@gmail.com> <1397402659.6306.5.camel@leira.trondhjem.org> <534BDBB6.5030306@gmail.com> <4B0A3D7E-5DD0-47A2-B1C8-A2F64E37D781@primarydata.com> <534BE338.2030001@gmail.com> <1397487639.7280.4.camel@leira.trondhjem.org> <534CBD76.1030109@gmail.com> <1FB7545D-81C5-46D5-9A42-322553F122D8@primarydata.com> Organization: PrimaryData Inc X-Mailer: Evolution 3.10.4 (3.10.4-2.fc20) 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=-7.6 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 Tue, 2014-04-15 at 09:22 -0400, Trond Myklebust wrote: > On Apr 15, 2014, at 1:02, Kinglong Mee wrote: > > Next, the "stat testfile" gets data from cache, > > because NFS_INO_INVALID_ATTR flags is cleared below. > > > > Calltrace, > > [ 4883.997254] nfs4_proc_write_setup > > [ 4884.006885] NFS: 1365 nfs_writeback_done (status 11) > > [ 4884.008215] nfs4_write_done > > [ 4884.009273] nfs4_write_done_cb > > [ 4884.010013] nfs_post_op_update_inode_force_wcc > > [ 4884.011221] nfs_update_inode > > [ 4884.012001] nfs_update_inode > > [ 4884.012952] nfs_writeback_done: before nfs_should_remove_suid > > [ 4884.014722] nfs_writeback_done: in nfs_should_remove_suid > > [ 4884.016549] nfs4_close_done > > [ 4884.017614] nfs_refresh_inode > > [ 4884.018645] nfs_update_inode > > [ 4884.019693] nfs_update_inode > > > > But, if getting status before close, the mode can be update to latest. > > Argh. That is a bug in nfs_update_inode(). It is not supposed to clear NFS_INO_INVALID_ATTR if nfs_fattr does not contain a complete set of attributes. > > Thanks for testing, Kinglong. This is extremely helpful... Can you please see if the following patch fixes the above issue? 8<-------------------------------------------------------------- From 12c9c63a005889d70fbcbdb746cd7e7d127b0f01 Mon Sep 17 00:00:00 2001 From: Trond Myklebust Date: Tue, 15 Apr 2014 10:07:57 -0400 Subject: [PATCH] NFS: Don't declare inode uptodate unless all attributes were checked Fix a bug, whereby nfs_update_inode() was declaring the inode to be up to date despite not having checked all the attributes. The bug occurs because the temporary variable in which we cache the validity information is 'sanitised' before reapplying to nfsi->cache_validity. Reported-by: Kinglong Mee Cc: stable@vger.kernel.org # 3.5+ Signed-off-by: Trond Myklebust --- fs/nfs/inode.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c index c0f7b1d0b814..a6f9c8581228 100644 --- a/fs/nfs/inode.c +++ b/fs/nfs/inode.c @@ -1581,18 +1581,20 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) inode->i_version = fattr->change_attr; } } else if (server->caps & NFS_CAP_CHANGE_ATTR) - invalid |= save_cache_validity; + nfsi->cache_validity |= save_cache_validity; if (fattr->valid & NFS_ATTR_FATTR_MTIME) { memcpy(&inode->i_mtime, &fattr->mtime, sizeof(inode->i_mtime)); } else if (server->caps & NFS_CAP_MTIME) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_REVAL_FORCED); if (fattr->valid & NFS_ATTR_FATTR_CTIME) { memcpy(&inode->i_ctime, &fattr->ctime, sizeof(inode->i_ctime)); } else if (server->caps & NFS_CAP_CTIME) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_REVAL_FORCED); /* Check if our cached file size is stale */ @@ -1614,7 +1616,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) (long long)new_isize); } } else - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_REVAL_PAGECACHE | NFS_INO_REVAL_FORCED); @@ -1622,7 +1625,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) if (fattr->valid & NFS_ATTR_FATTR_ATIME) memcpy(&inode->i_atime, &fattr->atime, sizeof(inode->i_atime)); else if (server->caps & NFS_CAP_ATIME) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATIME + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATIME | NFS_INO_REVAL_FORCED); if (fattr->valid & NFS_ATTR_FATTR_MODE) { @@ -1633,7 +1637,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) invalid |= NFS_INO_INVALID_ATTR|NFS_INO_INVALID_ACCESS|NFS_INO_INVALID_ACL; } } else if (server->caps & NFS_CAP_MODE) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_INVALID_ACCESS | NFS_INO_INVALID_ACL | NFS_INO_REVAL_FORCED); @@ -1644,7 +1649,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) inode->i_uid = fattr->uid; } } else if (server->caps & NFS_CAP_OWNER) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_INVALID_ACCESS | NFS_INO_INVALID_ACL | NFS_INO_REVAL_FORCED); @@ -1655,7 +1661,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) inode->i_gid = fattr->gid; } } else if (server->caps & NFS_CAP_OWNER_GROUP) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_INVALID_ACCESS | NFS_INO_INVALID_ACL | NFS_INO_REVAL_FORCED); @@ -1668,7 +1675,8 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr) set_nlink(inode, fattr->nlink); } } else if (server->caps & NFS_CAP_NLINK) - invalid |= save_cache_validity & (NFS_INO_INVALID_ATTR + nfsi->cache_validity |= save_cache_validity & + (NFS_INO_INVALID_ATTR | NFS_INO_REVAL_FORCED); if (fattr->valid & NFS_ATTR_FATTR_SPACE_USED) {