On Wed, 2024-08-21 at 16:05 -0400, Mike Snitzer wrote: > On Wed, Aug 21, 2024 at 03:20:55PM -0400, Jeff Layton wrote: > > On Mon, 2024-08-19 at 14:17 -0400, Mike Snitzer wrote: > > > These latest changes are available in my git tree here: > > > https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next > > > > > > Significant progress was made on the entire series, I implemented all > > > 3 changes NeilBrown suggested here: > > > https://marc.info/?l=linux-nfs&m=172004547710968&w=2 > > > > > > And Neil kindly provided review of a preliminary v12 that I then used > > > to refine this final v12 further. Neil found the series much cleaner > > > and approachable. > > > > > > This v12 also switches the NFS client's localio code over to driving > > > IO in terms of nfsd's nfsd_file (rather than a simple file pointer). > > > I checked with Jeff Layton and he likes the switch to using nfsd_file > > > (Jeff did suggest I make sure to keep struct nfsd_file completely > > > opaque to the client). Proper use of nfsd_file provides a solid > > > performance improvement (as detailed in the last patch's commit > > > header) thanks especially to the nfsd filecache's GC feature (which > > > localio now makes use of). > > > > > > Testing: > > > - Chuck's kdevops NFS testing has been operating against the > > > nfs-localio-for-next branch for a while now (not sure if LOCALIO is > > > enabled or if Chuck is just verifying the branch works with LOCALIO > > > disabled). > > > - Verified all of Hammerspace's various sanity tests pass (this > > > includes numerous pNFS and flexfiles tests). > > > > > > Please review, I'm hopeful I've addressed any outstanding issues and > > > that these changes worthy of being merged for v6.12. If you see > > > something, say something ;) > > > > > > Changes since v11: > > > - The required localio specific changes in fs/nfsd/ are much simpler > > > (thanks to the prelim patches that update common code to support the > > > the localio case, fs/nfsd/localio.c in particular is now very lean) > > > - Improved the localio protocol to address NeilBrown's issue #1. > > > Replaced GETUUID with UUID_IS_LOCAL RPC, which inverts protocol such > > > that client generates a nonce (shortlived single-use UUID) and > > > proceeds to verify the server sees it in nfs_common. > > > - this eliminated the need to add 'struct nfsd_uuid' to nfsd_net > > > - Finished the RFC series NeilBrown started to introduce > > > nfsd_file_acquire_local(), enables the use of a "fake" svc_rqst to > > > be eliminated: https://marc.info/?l=linux-nfs&m=171980269529965&w=2 ; > > > (uses auth_domain as suggested, addresses NeilBrown's issue #2) > > > - rpcauth_map_clnt_to_svc_cred_local now uses userns of client and > > > from_{kuid,kgid}_munged (hopefully addresses NeilBrown's issue #3) > > > - Updated nfs_local_call_write() to override_creds() with the cred > > > used by the client to open the localio file. > > > - To avoid localio hitting writeback deadlock (same as is done for > > > existing loopback NFSD support in nfsd_vfs_write() function): set > > > PF_LOCAL_THROTTLE | PF_MEMALLOC_NOIO in nfs_local_call_write() and > > > restore current->flags before return. > > > - Factored nfs_stat_to_errno and nfs4_stat_to_errno out to nfs_common > > > to eliminate localio code creating yet another copy of them. > > > (eliminates existing duplication between fs/nfs/nfs[23]xdr.c) > > > - Simplified Kconfig so that NFS_LOCALIO depends on NFS_FS and > > > NFSD_LOCALIO depends on NFSD. > > > - Only support localio if UNIX Authentication (AUTH_UNIX) is used. > > > - Improved workqueue patch to not use wait_for_completion(). > > > - Dropped 2 prelim fs/nfs/ patches that weren't actually needed. > > > - Updated localio.rst to reflect the various changes listed above, > > > also added a new "FAQ" section from Trond (which was informed by an > > > in-person discussion about localio that Trond had with Christoph). > > > - Fixed "nfsd: add nfsd_file_acquire_local()" commit to work with > > > NFSv3 (had been testing with NFSv4.2 and the fact that NFSv3 > > > regressed due to 'nfs_ver' not being properly initialized for > > > non-LOCALIO callers was missed. > > > - Fixed issue Neil reported where "When using localio, if I open, > > > read, don't close, then try to stop the server and umount the > > > exported filesystem I get EBUSY for the umount." > > > - fix by removing refcount on localio file (no longer cache open > > > localio file in the client). > > > - Fixed nfsd tracepoints that were impacted by the possibility they'd > > > be passed a NULL rqstp when using localio. > > > - Rebased on cel/nfsd-next (based on v6.11-rc4) to layer upon Neil's > > > various changes that were originally motivated by LOCALIO, reduces > > > footprint of this patchset. > > > - Exported nfsd_file interfaces needed to switch the nfs client's > > > localio code over to using it. > > > - Switched the the nfs client's localio code over to using nfsd_file. > > > > > > Thanks, > > > Mike > > > > > > Mike Snitzer (13): > > > nfs_common: factor out nfs_errtbl and nfs_stat_to_errno > > > nfs_common: factor out nfs4_errtbl and nfs4_stat_to_errno > > > nfs: factor out {encode,decode}_opaque_fixed to nfs_xdr.h > > > nfsd: fix nfsfh tracepoints to properly handle NULL rqstp > > > SUNRPC: remove call_allocate() BUG_ONs > > > nfs_common: add NFS LOCALIO auxiliary protocol enablement > > > nfsd: implement server support for NFS_LOCALIO_PROGRAM > > > nfs: implement client support for NFS_LOCALIO_PROGRAM > > > nfs: add Documentation/filesystems/nfs/localio.rst > > > nfsd: use GC for nfsd_file returned by nfsd_file_acquire_local > > > nfs_common: expose localio's required nfsd symbols to nfs client > > > nfs: push localio nfsd_file_put call out to client > > > nfs: switch client to use nfsd_file for localio > > > > > > NeilBrown (3): > > > nfsd: factor out __fh_verify to allow NULL rqstp to be passed > > > nfsd: add nfsd_file_acquire_local() > > > SUNRPC: replace program list with program array > > > > > > Trond Myklebust (4): > > > nfs: enable localio for non-pNFS IO > > > pnfs/flexfiles: enable localio support > > > nfs/localio: use dedicated workqueues for filesystem read and write > > > nfs: add FAQ section to Documentation/filesystems/nfs/localio.rst > > > > > > Weston Andros Adamson (4): > > > SUNRPC: add rpcauth_map_clnt_to_svc_cred_local > > > nfsd: add localio support > > > nfs: pass struct file to nfs_init_pgio and nfs_init_commit > > > nfs: add localio support > > > > > > Documentation/filesystems/nfs/localio.rst | 255 +++++++ > > > fs/Kconfig | 3 + > > > fs/nfs/Kconfig | 15 + > > > fs/nfs/Makefile | 1 + > > > fs/nfs/client.c | 15 +- > > > fs/nfs/filelayout/filelayout.c | 6 +- > > > fs/nfs/flexfilelayout/flexfilelayout.c | 142 +++- > > > fs/nfs/flexfilelayout/flexfilelayout.h | 2 + > > > fs/nfs/flexfilelayout/flexfilelayoutdev.c | 6 + > > > fs/nfs/inode.c | 57 +- > > > fs/nfs/internal.h | 61 +- > > > fs/nfs/localio.c | 784 ++++++++++++++++++++++ > > > fs/nfs/nfs2xdr.c | 70 +- > > > fs/nfs/nfs3xdr.c | 108 +-- > > > fs/nfs/nfs4xdr.c | 84 +-- > > > fs/nfs/nfstrace.h | 61 ++ > > > fs/nfs/pagelist.c | 16 +- > > > fs/nfs/pnfs_nfs.c | 2 +- > > > fs/nfs/write.c | 12 +- > > > fs/nfs_common/Makefile | 5 + > > > fs/nfs_common/common.c | 134 ++++ > > > fs/nfs_common/nfslocalio.c | 194 ++++++ > > > fs/nfsd/Kconfig | 15 + > > > fs/nfsd/Makefile | 1 + > > > fs/nfsd/export.c | 8 +- > > > fs/nfsd/filecache.c | 90 ++- > > > fs/nfsd/filecache.h | 5 + > > > fs/nfsd/localio.c | 183 +++++ > > > fs/nfsd/netns.h | 8 +- > > > fs/nfsd/nfsctl.c | 2 +- > > > fs/nfsd/nfsd.h | 6 +- > > > fs/nfsd/nfsfh.c | 114 ++-- > > > fs/nfsd/nfsfh.h | 5 + > > > fs/nfsd/nfssvc.c | 100 ++- > > > fs/nfsd/trace.h | 39 +- > > > fs/nfsd/vfs.h | 10 + > > > include/linux/nfs.h | 9 + > > > include/linux/nfs_common.h | 17 + > > > include/linux/nfs_fs_sb.h | 10 + > > > include/linux/nfs_xdr.h | 20 +- > > > include/linux/nfslocalio.h | 56 ++ > > > include/linux/sunrpc/auth.h | 4 + > > > include/linux/sunrpc/svc.h | 7 +- > > > net/sunrpc/auth.c | 22 + > > > net/sunrpc/clnt.c | 6 - > > > net/sunrpc/svc.c | 68 +- > > > net/sunrpc/svc_xprt.c | 2 +- > > > net/sunrpc/svcauth_unix.c | 3 +- > > > 48 files changed, 2428 insertions(+), 415 deletions(-) > > > create mode 100644 Documentation/filesystems/nfs/localio.rst > > > create mode 100644 fs/nfs/localio.c > > > create mode 100644 fs/nfs_common/common.c > > > create mode 100644 fs/nfs_common/nfslocalio.c > > > create mode 100644 fs/nfsd/localio.c > > > create mode 100644 include/linux/nfs_common.h > > > create mode 100644 include/linux/nfslocalio.h > > > > > > > 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. > > > > 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 thought about folding the nfsd_file changes in from the beginning > but stopped short of doing so because the churn you speak of really > only smacks you in the face when reviewing the > fs/nfs/flexfilelayout/flexfilelayout.c changes (which happen to be > featured prominently at the top of patch 23). > > Otherwise, there really is very little change/churn from the nfsd_file > switchover. And the general statement that the series hard to review > over needless churn seems strong. Only place is with nfsd_file. > That's the part I focused on initially. I only did some cursory looks at the client-side bits. To give you an example: I was confused as to why you were passing back a struct file in nfsd_open_local_fh in patch #9, and it wasn't until I got into the later part of the series that I figured out that you changed that to a struct nfsd_file pointer later (which makes sense). I wouldn't have had to do that if the rework around nfsd_file had been done from the get-go. > Anyway, I can rebase to fold the nfsd_file changes in (so that its > done that way from the start) if you and others want it done that way. > Please just have one last think about it and let me know. I think that would be best. The historical commits that show the interim stages of development just aren't terribly useful when we're merging brand new functionality like this. -- Jeff Layton <jlayton@xxxxxxxxxx>
