Re: [PATCH v12 22/24] nfs: push localio nfsd_file_put call out to client

[Jeff Layton <jlayton@kernel.org>]

On Mon, 2024-08-19 at 14:17 -0400, Mike Snitzer wrote:
> Change nfsd_open_local_fh() to return an nfsd_file, instead of file,
> and move associated reference counting to the nfs client's
> nfs_local_open_fh().
> 
> This is the first step in allowing proper use of nfsd_file for localio
> (such that the benefits of nfsd's filecache can be realized).
> 
> Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> ---
>  fs/nfs/localio.c           |  7 +++++--
>  fs/nfs_common/nfslocalio.c |  2 +-
>  fs/nfsd/localio.c          | 11 ++++-------
>  fs/nfsd/vfs.h              |  2 +-
>  include/linux/nfslocalio.h |  2 +-
>  5 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> index 38303427e0b3..7d63d7e34643 100644
> --- a/fs/nfs/localio.c
> +++ b/fs/nfs/localio.c
> @@ -237,13 +237,14 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
>  		  struct nfs_fh *fh, const fmode_t mode)
>  {
>  	struct file *filp;
> +	struct nfsd_file *nf;
>  	int status;
>  
>  	if (mode & ~(FMODE_READ | FMODE_WRITE))
>  		return ERR_PTR(-EINVAL);
>  
>  	status = nfs_to.nfsd_open_local_fh(clp->cl_nfssvc_net, clp->cl_nfssvc_dom,
> -					   clp->cl_rpcclient, cred, fh, mode, &filp);
> +					   clp->cl_rpcclient, cred, fh, mode, &nf);
>  	if (status < 0) {
>  		trace_nfs_local_open_fh(fh, mode, status);
>  		switch (status) {
> @@ -255,8 +256,10 @@ nfs_local_open_fh(struct nfs_client *clp, const struct cred *cred,
>  		case -ETIMEDOUT:
>  			status = -EAGAIN;
>  		}
> -		filp = ERR_PTR(status);
> +		return ERR_PTR(status);
>  	}
> +	filp = get_file(nfs_to.nfsd_file_file(nf));
> +	nfs_to.nfsd_file_put(nf);
>  	return filp;
>  }
>  EXPORT_SYMBOL_GPL(nfs_local_open_fh);
> diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> index 087649911b52..f59167e596d3 100644
> --- a/fs/nfs_common/nfslocalio.c
> +++ b/fs/nfs_common/nfslocalio.c
> @@ -98,7 +98,7 @@ static void put_##NFSD_SYMBOL(void)			\
>  /* The nfs localio code needs to call into nfsd to map filehandle -> struct nfsd_file */
>  extern int nfsd_open_local_fh(struct net *, struct auth_domain *, struct rpc_clnt *,
>  			      const struct cred *, const struct nfs_fh *,
> -			      const fmode_t, struct file **);
> +			      const fmode_t, struct nfsd_file **);
>  DEFINE_NFS_TO_NFSD_SYMBOL(nfsd_open_local_fh);
>  
>  /* The nfs localio code needs to call into nfsd to acquire the nfsd_file */
> diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> index 008b935a3a6c..2ceab49f3cb6 100644
> --- a/fs/nfsd/localio.c
> +++ b/fs/nfsd/localio.c
> @@ -32,13 +32,13 @@
>   * @cred: cred that the client established
>   * @nfs_fh: filehandle to lookup
>   * @fmode: fmode_t to use for open
> - * @pfilp: returned file pointer that maps to @nfs_fh
> + * @pnf: returned nfsd_file pointer that maps to @nfs_fh
>   *
>   * This function maps a local fh to a path on a local filesystem.
>   * This is useful when the nfs client has the local server mounted - it can
>   * avoid all the NFS overhead with reads, writes and commits.
>   *
> - * On successful return, caller is responsible for calling path_put. Also
> + * On successful return, caller is responsible for calling nfsd_file_put. Also
>   * note that this is called from nfs.ko via find_symbol() to avoid an explicit
>   * dependency on knfsd. So, there is no forward declaration in a header file
>   * for it that is shared with the client.
> @@ -49,7 +49,7 @@ int nfsd_open_local_fh(struct net *cl_nfssvc_net,
>  			 const struct cred *cred,
>  			 const struct nfs_fh *nfs_fh,
>  			 const fmode_t fmode,
> -			 struct file **pfilp)
> +			 struct nfsd_file **pnf)

I think I'd prefer to see this change made earlier in the series, when
nfsd_open_local_fh is first introduced (patch #10), and then just adapt
the later patches to deal with the change.

>  {
>  	int mayflags = NFSD_MAY_LOCALIO;
>  	int status = 0;
> @@ -57,7 +57,6 @@ int nfsd_open_local_fh(struct net *cl_nfssvc_net,
>  	const struct cred *save_cred;
>  	struct svc_cred rq_cred;
>  	struct svc_fh fh;
> -	struct nfsd_file *nf;
>  	__be32 beres;
>  
>  	if (nfs_fh->size > NFS4_FHSIZE)
> @@ -91,13 +90,11 @@ int nfsd_open_local_fh(struct net *cl_nfssvc_net,
>  	rpcauth_map_clnt_to_svc_cred_local(rpc_clnt, cred, &rq_cred);
>  
>  	beres = nfsd_file_acquire_local(cl_nfssvc_net, &rq_cred, rpc_clnt->cl_vers,
> -					cl_nfssvc_dom, &fh, mayflags, &nf);
> +					cl_nfssvc_dom, &fh, mayflags, pnf);
>  	if (beres) {
>  		status = nfs_stat_to_errno(be32_to_cpu(beres));
>  		goto out_fh_put;
>  	}
> -	*pfilp = get_file(nf->nf_file);
> -	nfsd_file_put(nf);

I do like eliminating the unnecessary refcount shuffle here. Much
simpler.

>  out_fh_put:
>  	fh_put(&fh);
>  	if (rq_cred.cr_group_info)
> diff --git a/fs/nfsd/vfs.h b/fs/nfsd/vfs.h
> index 9720951c49a0..ec8a8aae540b 100644
> --- a/fs/nfsd/vfs.h
> +++ b/fs/nfsd/vfs.h
> @@ -166,7 +166,7 @@ int		nfsd_open_local_fh(struct net *net,
>  				   const struct cred *cred,
>  				   const struct nfs_fh *nfs_fh,
>  				   const fmode_t fmode,
> -				   struct file **pfilp);
> +				   struct nfsd_file **pnf);
>  
>  static inline int fh_want_write(struct svc_fh *fh)
>  {
> diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
> index 6302d36f9112..7e09ff621d93 100644
> --- a/include/linux/nfslocalio.h
> +++ b/include/linux/nfslocalio.h
> @@ -35,7 +35,7 @@ struct nfsd_file;
>  typedef int (*nfs_to_nfsd_open_local_fh_t)(struct net *, struct auth_domain *,
>  				struct rpc_clnt *, const struct cred *,
>  				const struct nfs_fh *, const fmode_t,
> -				struct file **);
> +				struct nfsd_file **);
>  typedef struct nfsd_file * (*nfs_to_nfsd_file_get_t)(struct nfsd_file *);
>  typedef void (*nfs_to_nfsd_file_put_t)(struct nfsd_file *);
>  typedef struct file * (*nfs_to_nfsd_file_file_t)(struct nfsd_file *);

-- 
Jeff Layton <jlayton@kernel.org>