On Mon, 01 Jul 2024, Jeff Layton wrote:

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

quoted

On Sun, Jun 30, 2024 at 03:59:58PM -0400, Jeff Layton wrote:

quoted

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. 

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?

Usually the client is issuing regular COMMITs. If that doesn't happen,
then the flusher threads should get the rest.

Side note: I don't guess COMMIT goes over the localio path yet, does
it? Maybe it should. It would be nice to not tie up an nfsd thread with
writeback.

The documentation certainly claims that COMMIT uses the localio path.  I
haven't double checked the code but I'd be very surprised if it didn't.

NeilBrown

quoted

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 imagine so, which is why I suggest using the filecache. If we get one
READ or WRITE for the file via localio, we're pretty likely to get
more. Why not amortize that file open over several operations?
 

quoted

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.

Typically, the client issues COMMIT calls when the client-side fd is
closed (for CTO). While I think we do need to be able to deal with
flushing files with dirty data that are left "hanging", that shouldn't
be the common case. Most of the time, the client is going to be issuing
regular COMMITs so that it can clean its pages.

IOW, I don't see how localio is any different than the case of normal
v3 IO in this respect.
-- 
Jeff Layton [off-list ref]