Re: [PATCH v9 13/19] nfsd: add "localio" support

[Jeff Layton <jlayton@kernel.org>]

On Sun, 2024-06-30 at 15:44 -0400, Mike Snitzer wrote:
> On Sun, Jun 30, 2024 at 10:49:51AM -0400, Chuck Lever wrote:
> > On Sat, Jun 29, 2024 at 06:18:42PM -0400, Chuck Lever wrote:
> > > > +
> > > > +	/* nfs_fh -> svc_fh */
> > > > +	if (nfs_fh->size > NFS4_FHSIZE) {
> > > > +		status = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +	fh_init(&fh, NFS4_FHSIZE);
> > > > +	fh.fh_handle.fh_size = nfs_fh->size;
> > > > +	memcpy(fh.fh_handle.fh_raw, nfs_fh->data, nfs_fh->size);
> > > > +
> > > > +	if (fmode & FMODE_READ)
> > > > +		mayflags |= NFSD_MAY_READ;
> > > > +	if (fmode & FMODE_WRITE)
> > > > +		mayflags |= NFSD_MAY_WRITE;
> > > > +
> > > > +	beres = nfsd_file_acquire(rqstp, &fh, mayflags, &nf);
> > > > +	if (beres) {
> > > > +		status = nfs_stat_to_errno(be32_to_cpu(beres));
> > > > +		goto out_fh_put;
> > > > +	}
> > > 
> > > So I'm wondering whether just calling fh_verify() and then
> > > nfsd_open_verified() would be simpler and/or good enough here. Is
> > > there a strong reason to use the file cache for locally opened
> > > files? Jeff, any thoughts?
> > 
> > > Will there be writeback ramifications for
> > > doing this? Maybe we need a comment here explaining how these files
> > > are garbage collected (just an fput by the local I/O client, I would
> > > guess).
> > 
> > OK, going back to this: Since right here we immediately call 
> > 
> > 	nfsd_file_put(nf);
> > 
> > There are no writeback ramifications nor any need to comment about
> > garbage collection. But this still seems like a lot of (possibly
> > unnecessary) overhead for simply obtaining a struct file.
> 
> Easy enough change, probably best to avoid the filecache but would like
> to verify with Jeff before switching:
> 
> diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> index 1d6508aa931e..85ebf63789fb 100644
> --- a/fs/nfsd/localio.c
> +++ b/fs/nfsd/localio.c
> @@ -197,7 +197,6 @@ int nfsd_open_local_fh(struct net *cl_nfssvc_net,
>         const struct cred *save_cred;
>         struct svc_rqst *rqstp;
>         struct svc_fh fh;
> -       struct nfsd_file *nf;
>         __be32 beres;
> 
>         if (nfs_fh->size > NFS4_FHSIZE)
> @@ -235,13 +234,12 @@ int nfsd_open_local_fh(struct net *cl_nfssvc_net,
>         if (fmode & FMODE_WRITE)
>                 mayflags |= NFSD_MAY_WRITE;
> 
> -       beres = nfsd_file_acquire(rqstp, &fh, mayflags, &nf);
> +       beres = fh_verify(rqstp, &fh, S_IFREG, mayflags);
>         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);
> +       status = nfsd_open_verified(rqstp, &fh, mayflags, pfilp);
>  out_fh_put:
>         fh_put(&fh);
>         nfsd_local_fakerqst_destroy(rqstp);
> 

My suggestion would be to _not_ do this. I think you do want to use the
filecache (mostly for performance reasons).
-- 
Jeff Layton <jlayton@kernel.org>