On Mon, 01 Jul 2024, NeilBrown wrote:
> On Mon, 01 Jul 2024, Jeff Layton wrote:
> > On Sun, 2024-06-30 at 15:55 -0400, Chuck Lever wrote:
> > > On Sun, Jun 30, 2024 at 03:52:51PM -0400, Jeff Layton wrote:
> > > > 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).
> > >
> > > But look carefully:
> > >
> > > -- we're not calling nfsd_file_acquire_gc() here
> > >
> > > -- we're immediately calling nfsd_file_put() on the returned nf
> > >
> > > There's nothing left in the file cache when nfsd_open_local_fh()
> > > returns. Each call here will do a full file open and a full close.
> > >
> > >
> >
> > Good point. This should be calling nfsd_file_acquire_gc(), IMO.
>
> Or the client could do a v4 style acquire, and not call nfsd_file_put()
> until it was done with the file. I don't see a specific problem with
> _gc, but avoiding the heuristic it implies seems best where possible.
>
I'm now wondering if this matters at all.
For NFSv4 the client still calls OPEN and CLOSE over the wire, so the
file will be in the cache whenever it is open so the current code is
fine.
For NFSv3 the client will only do the lookup once on the first IO
request. The struct file is stored in a client data structure and used
subsequently without any interaction with nfsd.
So if the client opens the same file multiple times we might get extra
lookups on the server, but I'm not at all sure that justifies any
complexity.
So my current inclination is to leave this code as is.
NeilBrown
