Re: [PATCH 1/1] NFSv4: make cache consistency bitmask dynamic

[Trond Myklebust <trondmy@hammerspace.com>]

Hi Olga,

On Mon, 2020-09-14 at 17:05 -0400, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <kolga@netapp.com>
> 
> Client uses static bitmask for GETATTR on CLOSE/WRITE/DELEGRETURN
> and ignores the fact that it might have some attributes marked
> invalid in its cache. Compared to v3 where all attributes are
> retrieved in postop attributes, v4's cache is frequently out of
> sync and leads to standalone GETATTRs being sent to the server.
> 
> Instead, in addition to the minimum cache consistency attributes
> also check cache_validity and adjust the GETATTR request accordingly.
> 
> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> ---
>  fs/nfs/nfs4proc.c       | 45 ++++++++++++++++++++++++++++++++++++++-
> --
>  include/linux/nfs_xdr.h |  6 +++---
>  2 files changed, 45 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> index 6e95c85fe395..d7434a3697d9 100644
> --- a/fs/nfs/nfs4proc.c
> +++ b/fs/nfs/nfs4proc.c
> @@ -107,6 +107,9 @@ static int nfs41_test_stateid(struct nfs_server
> *, nfs4_stateid *,
>  static int nfs41_free_stateid(struct nfs_server *, const
> nfs4_stateid *,
>  		const struct cred *, bool);
>  #endif
> +static void nfs4_bitmask_adjust(__u32 *bitmask, struct inode *inode,
> +		struct nfs_server *server,
> +		struct nfs4_label *label);
>  
>  #ifdef CONFIG_NFS_V4_SECURITY_LABEL
>  static inline struct nfs4_label *
> @@ -3632,9 +3635,10 @@ static void nfs4_close_prepare(struct rpc_task
> *task, void *data)
>  
>  	if (calldata->arg.fmode == 0 || calldata->arg.fmode ==
> FMODE_READ) {
>  		/* Close-to-open cache consistency revalidation */
> -		if (!nfs4_have_delegation(inode, FMODE_READ))
> +		if (!nfs4_have_delegation(inode, FMODE_READ)) {
>  			calldata->arg.bitmask = NFS_SERVER(inode)-
> >cache_consistency_bitmask;
> -		else
> +			nfs4_bitmask_adjust(calldata->arg.bitmask,
> inode, NFS_SERVER(inode), NULL);
> +		} else
>  			calldata->arg.bitmask = NULL;
>  	}
>  
> @@ -5360,6 +5364,38 @@ bool
> nfs4_write_need_cache_consistency_data(struct nfs_pgio_header *hdr)
>  	return nfs4_have_delegation(hdr->inode, FMODE_READ) == 0;
>  }
>  
> +static void nfs4_bitmask_adjust(__u32 *bitmask, struct inode *inode,
> +				struct nfs_server *server,
> +				struct nfs4_label *label)
> +{
> +
> +	unsigned long cache_validity = READ_ONCE(NFS_I(inode)-
> >cache_validity);
> +
> +	if ((cache_validity & NFS_INO_INVALID_DATA) ||
> +		(cache_validity & NFS_INO_REVAL_PAGECACHE) ||
> +		(cache_validity & NFS_INO_REVAL_FORCED) ||
> +		(cache_validity & NFS_INO_INVALID_OTHER))
> +		nfs4_bitmap_copy_adjust(bitmask, nfs4_bitmask(server,
> label), inode);
> +
> +	if (cache_validity & NFS_INO_INVALID_ATIME)
> +		bitmask[1] |= FATTR4_WORD1_TIME_ACCESS;
> +	if (cache_validity & NFS_INO_INVALID_ACCESS)
> +		bitmask[0] |= FATTR4_WORD1_MODE | FATTR4_WORD1_OWNER |
> +				FATTR4_WORD1_OWNER_GROUP;
> +	if (cache_validity & NFS_INO_INVALID_ACL)
> +		bitmask[0] |= FATTR4_WORD0_ACL;
> +	if (cache_validity & NFS_INO_INVALID_LABEL)
> +		bitmask[2] |= FATTR4_WORD2_SECURITY_LABEL;
> +	if (cache_validity & NFS_INO_INVALID_CTIME)
> +		bitmask[0] |= FATTR4_WORD0_CHANGE;
> +	if (cache_validity & NFS_INO_INVALID_MTIME)
> +		bitmask[1] |= FATTR4_WORD1_TIME_MODIFY;
> +	if (cache_validity & NFS_INO_INVALID_SIZE)
> +		bitmask[0] |= FATTR4_WORD0_SIZE;
> +	if (cache_validity & NFS_INO_INVALID_BLOCKS)
> +		bitmask[1] |= FATTR4_WORD1_SPACE_USED;

If we hold a delegation (which we could do when called
from nfs4_proc_write_setup()) then we only want to get extra attributes
if the NFS_INO_REVAL_FORCED flag is also set.

> +}
> +
>  static void nfs4_proc_write_setup(struct nfs_pgio_header *hdr,
>  				  struct rpc_message *msg,
>  				  struct rpc_clnt **clnt)
> @@ -5369,8 +5405,10 @@ static void nfs4_proc_write_setup(struct
> nfs_pgio_header *hdr,
>  	if (!nfs4_write_need_cache_consistency_data(hdr)) {
>  		hdr->args.bitmask = NULL;
>  		hdr->res.fattr = NULL;
> -	} else
> +	} else {
>  		hdr->args.bitmask = server->cache_consistency_bitmask;
> +		nfs4_bitmask_adjust(hdr->args.bitmask, hdr->inode,
> server, NULL);
> +	}
>  
>  	if (!hdr->pgio_done_cb)
>  		hdr->pgio_done_cb = nfs4_write_done_cb;
> @@ -6406,6 +6444,7 @@ static int _nfs4_proc_delegreturn(struct inode
> *inode, const struct cred *cred,
>  	data->args.fhandle = &data->fh;
>  	data->args.stateid = &data->stateid;
>  	data->args.bitmask = server->cache_consistency_bitmask;
> +	nfs4_bitmask_adjust(data->args.bitmask, inode, server, NULL);
>  	nfs_copy_fh(&data->fh, NFS_FH(inode));
>  	nfs4_stateid_copy(&data->stateid, stateid);
>  	data->res.fattr = &data->fattr;
> diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h
> index 9408f3252c8e..bafbf6695796 100644
> --- a/include/linux/nfs_xdr.h
> +++ b/include/linux/nfs_xdr.h
> @@ -525,7 +525,7 @@ struct nfs_closeargs {
>  	struct nfs_seqid *	seqid;
>  	fmode_t			fmode;
>  	u32			share_access;
> -	const u32 *		bitmask;
> +	u32 *			bitmask;
>  	struct nfs4_layoutreturn_args *lr_args;
>  };
>  
> @@ -608,7 +608,7 @@ struct nfs4_delegreturnargs {
>  	struct nfs4_sequence_args	seq_args;
>  	const struct nfs_fh *fhandle;
>  	const nfs4_stateid *stateid;
> -	const u32 * bitmask;
> +	u32 * bitmask;
>  	struct nfs4_layoutreturn_args *lr_args;
>  };
>  
> @@ -648,7 +648,7 @@ struct nfs_pgio_args {
>  	union {
>  		unsigned int		replen;			/*
> used by read */
>  		struct {
> -			const u32 *		bitmask;	/*
> used by write */
> +			u32 *			bitmask;	/*
> used by write */
>  			enum nfs3_stable_how	stable;		/*
> used by write */
>  		};
>  	};

Otherwise this looks good. Thanks!

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com