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

[Jeff Layton <jlayton@kernel.org>]

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@kernel.org>