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

[Mike Snitzer <snitzer@kernel.org>]

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