Re: [PATCH v12 00/24] nfs/nfsd: add support for localio

[Mike Snitzer <snitzer@kernel.org>]

On Thu, Aug 22, 2024 at 03:18:56PM +0000, Chuck Lever III wrote:
> 
> 
> > On Aug 21, 2024, at 10:00 PM, Mike Snitzer <snitzer@kernel.org> wrote:
> > 
> > Hey Jeff,
> > 
> > On Wed, Aug 21, 2024 at 03:20:55PM -0400, Jeff Layton wrote:
> >> 
> >> This looks much improved. I didn't see anything that stood out at me as
> >> being problematic code-wise with the design or final product, aside
> >> from a couple of minor things.
> > 
> > BTW, thanks for this feedback, much appreciated!
> > 
> >> But...this patchset is hard to review. My main gripe is that there is a
> >> lot of "churn" -- places where you add code, just to rework it in a new
> >> way in a later patch.
> >> 
> >> For instance, the nfsd_file conversion should be integrated into the
> >> new infrastructure much earlier instead of having a patch that later
> >> does that conversion. Those kinds extraneous changes make this much
> >> harder to review than it would be if this were done in a way that
> >> avoided that churn.
> > 
> > I think I've addressed all your v12 review comments from earlier
> > today.  I've pushed the new series out to my git repo here:
> > https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next
> > 
> > No code changes, purely a bunch of rebasing to clean up like you
> > suggested.  Only outstanding thing is the nfsd tracepoints handling of
> > NULL rqstp (would like to get Chuck's expert feedback on that point).
> > 
> > Please feel free to have a look at my branch while I wait for any
> > other v12 feedback from Chuck and/or others before I send out v13.
> > I'd like to avoid spamming the list like I did in the past ;)
> 
> Was looking for an appropriate place to reply with this question,
> but didn't see one. So here goes:
> 
> One of the issues Neil mentioned was dealing with the case where
> a server file system is unexported and perhaps unmounted while
> there is LOCALIO ongoing.

Yes, that was a problem with v11 and prior because the client was
holding a reference on the nfsd_file's underlying file (nf->nf_file)
for the duration of the open (within nfs_open_ctx).  That client-side
caching of the open file was done for performance reasons, but with
client-side use of nfsd_file we can claw back ~50% of that performance
by using the filecache's GC (patch 23 in this series details the
comparative performance numbers I'm talking about).

So I removed that extra client-side reference entirely, and just rely
on nfsd's filecache to hold the file open by nfsd_file_acquire_local()
taking reference on nfsd_file (so server does the normal thing on
shutdown where it walks the filecache's hlist during shutdown).

> Can you describe what the client and application see in this
> case? Do you have test cases for this scenario?

No, I've been testing it manually:

Client is using localio for /mnt/test2:

# python3
>>> f = open("/mnt/test2/testfile", encoding = "ISO-8859-1")
>>> s = f.read(8)
>>>

Server (happens to be running in container) is shutdown:

podman exec -ti nfs_12 /bin/bash
[root@6baf567b559a /]# systemctl stop nfs-server.service
[root@6baf567b559a /]# umount /export/cvol_12_0

(even easier to test without server in a container)

> Obviously we don't want a crash or deadlock, but I would guess the
> client/app should behave just like remote NFSv3 -- there, the
> server returns ESTALE on a READ or WRITE, and read(2) or write(2)
> on the client returns EIO. Ie, behavior should be deterministic.

Yes, that is how it works: exactly like remote NFSv3.