Re: [PATCH] NFS: Do not set NFS_INO_INVALID_LABEL unless server supports labeled NFS

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4705 bytes --]

On Thu,  6 Feb 2014 14:53:38 -0500 Trond Myklebust
<trond.myklebust@primarydata.com> wrote:

> Commit aa9c2669626c (NFS: Client implementation of Labeled-NFS) introduces
> a performance regression. When nfs_zap_caches_locked is called, it sets
> the NFS_INO_INVALID_LABEL flag irrespectively of whether or not the
> NFS server supports security labels. Since that flag is never cleared,
> it means that all calls to nfs_revalidate_inode() will now trigger
> an on-the-wire GETATTR call.
> 
> This patch ensures that we never set the NFS_INO_INVALID_LABEL unless the
> server advertises support for labeled NFS.
> It also causes nfs_setsecurity() to clear NFS_INO_INVALID_LABEL when it
> has successfully set the security label for the inode.
> Finally it gets rid of the NFS_INO_INVALID_LABEL cruft from nfs_update_inode,
> which has nothing to do with labeled NFS.
> 
> Reported-by: Neil Brown <neilb@suse.de>
> Cc: stable@vger.kernel.org # 3.11+
> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
> ---
> 
> Hi Neil,
> Does this fix the GETATTR regression that you reported?

Yes, thanks.
With this patch I don't get streams of GETATTR requests after the READDIR
request which I previously fixed with that oneline change in inode.c

Tested-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown


> 
> Cheers
>  Trond
> 
>  fs/nfs/inode.c    | 14 ++++++++++----
>  fs/nfs/internal.h |  9 +++++++++
>  2 files changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 29cb93653b3c..4636b828e957 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -169,7 +169,6 @@ static void nfs_zap_caches_locked(struct inode *inode)
>  	if (S_ISREG(mode) || S_ISDIR(mode) || S_ISLNK(mode)) {
>  		nfs_fscache_invalidate(inode);
>  		nfsi->cache_validity |= NFS_INO_INVALID_ATTR
> -					| NFS_INO_INVALID_LABEL
>  					| NFS_INO_INVALID_DATA
>  					| NFS_INO_INVALID_ACCESS
>  					| NFS_INO_INVALID_ACL
> @@ -177,10 +176,10 @@ static void nfs_zap_caches_locked(struct inode *inode)
>  		nfs_zap_readdir_cookie(nfsi);
>  	} else
>  		nfsi->cache_validity |= NFS_INO_INVALID_ATTR
> -					| NFS_INO_INVALID_LABEL
>  					| NFS_INO_INVALID_ACCESS
>  					| NFS_INO_INVALID_ACL
>  					| NFS_INO_REVAL_PAGECACHE;
> +	nfs_zap_label_cache_locked(nfsi);
>  }
>  
>  void nfs_zap_caches(struct inode *inode)
> @@ -272,6 +271,13 @@ nfs_init_locked(struct inode *inode, void *opaque)
>  }
>  
>  #ifdef CONFIG_NFS_V4_SECURITY_LABEL
> +static void nfs_clear_label_invalid(struct inode *inode)
> +{
> +	spin_lock(&inode->i_lock);
> +	NFS_I(inode)->cache_validity &= ~NFS_INO_INVALID_LABEL;
> +	spin_unlock(&inode->i_lock);
> +}
> +
>  void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
>  					struct nfs4_label *label)
>  {
> @@ -289,6 +295,7 @@ void nfs_setsecurity(struct inode *inode, struct nfs_fattr *fattr,
>  					__func__,
>  					(char *)label->label,
>  					label->len, error);
> +		nfs_clear_label_invalid(inode);
>  	}
>  }
>  
> @@ -1654,7 +1661,7 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  		inode->i_blocks = fattr->du.nfs2.blocks;
>  
>  	/* Update attrtimeo value if we're out of the unstable period */
> -	if (invalid & (NFS_INO_INVALID_ATTR|NFS_INO_INVALID_LABEL)) {
> +	if (invalid & NFS_INO_INVALID_ATTR) {
>  		nfs_inc_stats(inode, NFSIOS_ATTRINVALIDATE);
>  		nfsi->attrtimeo = NFS_MINATTRTIMEO(inode);
>  		nfsi->attrtimeo_timestamp = now;
> @@ -1667,7 +1674,6 @@ static int nfs_update_inode(struct inode *inode, struct nfs_fattr *fattr)
>  		}
>  	}
>  	invalid &= ~NFS_INO_INVALID_ATTR;
> -	invalid &= ~NFS_INO_INVALID_LABEL;
>  	/* Don't invalidate the data if we were to blame */
>  	if (!(S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode)
>  				|| S_ISLNK(inode->i_mode)))
> diff --git a/fs/nfs/internal.h b/fs/nfs/internal.h
> index 8b5cc04a8611..fafdddac8271 100644
> --- a/fs/nfs/internal.h
> +++ b/fs/nfs/internal.h
> @@ -279,9 +279,18 @@ static inline void nfs4_label_free(struct nfs4_label *label)
>  	}
>  	return;
>  }
> +
> +static inline void nfs_zap_label_cache_locked(struct nfs_inode *nfsi)
> +{
> +	if (nfs_server_capable(&nfsi->vfs_inode, NFS_CAP_SECURITY_LABEL))
> +		nfsi->cache_validity |= NFS_INO_INVALID_LABEL;
> +}
>  #else
>  static inline struct nfs4_label *nfs4_label_alloc(struct nfs_server *server, gfp_t flags) { return NULL; }
>  static inline void nfs4_label_free(void *label) {}
> +static inline void nfs_zap_label_cache_locked(struct nfs_inode *nfsi)
> +{
> +}
>  #endif /* CONFIG_NFS_V4_SECURITY_LABEL */
>  
>  /* proc.c */


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]