On Sun, Jun 30, 2024 at 03:59:58PM -0400, 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.
So that goes to my point yesterday about writeback ramifications.
If these open files linger in the file cache, then when will they
get written back to storage and by whom? Is it going to be an nfsd
thread writing them back as part of garbage collection?
So, you're saying that the local I/O client will always behave like
NFSv3 in this regard, and open/read/close, open/write/close instead
of hanging on to the open file? That seems... suboptimal... and not
expected for a local file. That needs to be documented in the
LOCALIO design doc.
I'm also concerned about local applications closing a file but
having an open file handle linger in the file cache -- that can
prevent other accesses to the file until the GC ejects that open
file, as we've seen in the field.
IMHO nfsd_file_acquire_gc() is going to have some unwanted side
effects.
--
Chuck Lever
