On Mon, 01 Jul 2024, Jeff Layton wrote:

On Sun, 2024-06-30 at 15:55 -0400, Chuck Lever wrote:

quoted

On Sun, Jun 30, 2024 at 03:52:51PM -0400, Jeff Layton wrote:

quoted

On Sun, 2024-06-30 at 15:44 -0400, Mike Snitzer wrote:

quoted

On Sun, Jun 30, 2024 at 10:49:51AM -0400, Chuck Lever wrote:

quoted

On Sat, Jun 29, 2024 at 06:18:42PM -0400, Chuck Lever wrote:

quoted

quoted

+
+	/* 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?

quoted

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.

NeilBrown