mbox series

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

Message ID 20240819181750.70570-1-snitzer@kernel.org (mailing list archive)
Headers show
Series nfs/nfsd: add support for localio | expand

Message

Mike Snitzer Aug. 19, 2024, 6:17 p.m. UTC
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

Comments

Chuck Lever Aug. 19, 2024, 6:29 p.m. UTC | #1
> 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
Mike Snitzer Aug. 19, 2024, 6:43 p.m. UTC | #2
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.
Jeff Layton Aug. 21, 2024, 7:20 p.m. UTC | #3
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.
Chuck Lever Aug. 21, 2024, 7:56 p.m. UTC | #4
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.
Mike Snitzer Aug. 21, 2024, 8:05 p.m. UTC | #5
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
Mike Snitzer Aug. 21, 2024, 8:10 p.m. UTC | #6
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
Mike Snitzer Aug. 22, 2024, 2 a.m. UTC | #7
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
Jeff Layton Aug. 22, 2024, 12:35 p.m. UTC | #8
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 Aug. 22, 2024, 12:50 p.m. UTC | #9
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.
Chuck Lever Aug. 22, 2024, 3:18 p.m. UTC | #10
> 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
Mike Snitzer Aug. 22, 2024, 3:42 p.m. UTC | #11
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.