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@xxxxxxxxxx> 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.
