| Message ID | 20240819181750.70570-1-snitzer@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | nfs/nfsd: add support for localio | expand |
> On Aug 19, 2024, at 2:17 PM, Mike Snitzer <snitzer@kernel.org> wrote: > > 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). LOCALIO is enabled, but the tests I run (except for pynfs, which uses a synthetic client) all target a remote NFS server . There wasn't a convenient way to hack the workflows to run the NFS server locally. So, these tests act as a regression test with LOCALIO enabled and a remote NFS server -- ie, the traditional NFS deployment scenario. I thought this would be OK because Kent's rig is already handling LOCALIO-specific testing. When I re-enable the ltp NFS suite, that does use a local NFS server. That suite hasn't ever been reliable for me, so I don't use it for now. -- Chuck Lever
On Mon, Aug 19, 2024 at 06:29:57PM +0000, Chuck Lever III wrote: > > > > On Aug 19, 2024, at 2:17 PM, Mike Snitzer <snitzer@kernel.org> wrote: > > > > 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). > > LOCALIO is enabled, but the tests I run (except for pynfs, which > uses a synthetic client) all target a remote NFS server . > > There wasn't a convenient way to hack the workflows to run the > NFS server locally. So, these tests act as a regression test with > LOCALIO enabled and a remote NFS server -- ie, the traditional > NFS deployment scenario. I thought this would be OK because > Kent's rig is already handling LOCALIO-specific testing. Makes sense, thanks for clarifying. > When I re-enable the ltp NFS suite, that does use a local NFS > server. That suite hasn't ever been reliable for me, so I don't > use it for now. OK.
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.
On Mon, Aug 19, 2024 at 02:17:05PM -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 > > -- > 2.44.0 > > Hi Mike- I've started familiarizing myself with v12. I'm happy to see that the fake svc_rqst code is gone, and I thank you, Neil, and Trond for your effort replacing that code. I think I understand why we have to use an ephemeral garbage- collected nfsd_file rather than having the client hold one open while it has an open file descriptor for that file. Next I want to start auditing the client's open path for security issues. I will hold any specific comments until I've done that. It's great that Jeff has reviewed these already! I concur that a little patch re-organization will be helpful.
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. 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. Thanks, Mike
On Wed, Aug 21, 2024 at 03:56:44PM -0400, Chuck Lever wrote: > On Mon, Aug 19, 2024 at 02:17:05PM -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 > > > > -- > > 2.44.0 > > > > > > Hi Mike- > > I've started familiarizing myself with v12. I'm happy to see that > the fake svc_rqst code is gone, and I thank you, Neil, and Trond > for your effort replacing that code. > > I think I understand why we have to use an ephemeral garbage- > collected nfsd_file rather than having the client hold one open > while it has an open file descriptor for that file. > > Next I want to start auditing the client's open path for security > issues. I will hold any specific comments until I've done that. Sounds good, thanks. > It's great that Jeff has reviewed these already! I concur that a > little patch re-organization will be helpful. Sure, I just replied to Jeff. I can do the work to rebase the use of nfsd_file so that the series has it from the start if you agree it best that way. Please advise if you've noticed anything else that stands out for me to handle at the same time. Thanks, Mike
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 ;) Thanks, Mike
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.
On Wed, 2024-08-21 at 22:00 -0400, Mike Snitzer 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! > You're very welcome. Thanks for doing this work! > > 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 ;) > That looks much cleaner than the last posting. Once you post the new version I'll give a more thorough review, but I don't see any major problems at first glance.
> 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. Can you describe what the client and application see in this case? Do you have test cases for this scenario? 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. -- Chuck Lever
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.
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