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. -- Jeff Layton <jlayton@xxxxxxxxxx>
