Re: [RFC PATCH 0/6] Split up refactoring of fh_verify()

[Mike Snitzer <snitzer@kernel.org>]

On Tue, Aug 27, 2024 at 09:08:32PM -0400, Mike Snitzer wrote:
> On Tue, Aug 27, 2024 at 08:44:39PM -0400, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > These six patches are meant to replace:
> > 
> >   nfsd: factor out __fh_verify to allow NULL rqstp to be passed
> >   nfsd: add nfsd_file_acquire_local()
> > 
> > I've removed the @nfs_vers parameter to __fh_verify(), and tried
> > something a little different with the trace points. Please check
> > my math.
> > 
> > These have been compile-tested, but no more than that. Interested in
> > comments.
> 
> Reviewed quickly just now, nicely done!
> 
> I'll go over it very carefully now by replacing the 2 patches you
> noted and updating the localio patches thaa follow.  I noticed some
> typos and mentioning nfs_vers usage in a header despite you having
> removed the need to pass it. So I'll fix up those nits along the way.
> 
> But I just wanted to immediately say: thanks!

I reviewed and incorporated your 6 patches and successfully tested the
result (by issuing IO with and without LOCALIO enabled with both NFS
v3 and NFS v4.2).

I've pushed the latest code to my branch:
https://git.kernel.org/pub/scm/linux/kernel/git/snitzer/linux.git/log/?h=nfs-localio-for-next

I'm approaching the end of addressing all the v13 review feedback, but
still have to sort through a couple issues that Neil pointed out about
nfs_local_disable() racing with nfsd_open_local_fh().

But in case you're curious here is a preview of the changelog for
what'll be in v14 (hopefully will post by end of day tomorrow):

- Extended the nn->nfsd_serv reference lifetime to be identical to the
  nfsd_file (until after localio's IO is complete), suggested by Neil.
  This is made easier by introducing a 'struct nfs_localio_ctx' that
  contains both the 'nfsd_file' and 'nfsd_net' associated with
  localio.

- Switched nfs_common's 'nfs_to' symbol management locking from using
  mutex to spinlock, suggested by Neil.

- Eliminated nfs_local_file_open() by folding it into
  nfs_local_open_fh(), suggested by Neil.

- Update nfs_uuid_is_local() to get reference on the net, drop it in
  nfs_local_disable(), suggested by Neil.

- Pushed saving/restoring of client's cred down from
  nfsd_open_local_fh() to nfsd_file_acquire_local(), suggested by
  Neil.

- Updated NFSD_LOCALIO in fs/nfsd/Kconfig to explicitly 'default n'
  and improve description, suggested by Chuck. Also made the same
  updates to NFS_LOCALIO in fs/nfs/Kconfig.

- Split out a separate preliminary patch that introduces
  nfsd_serv_try_get() and nfsd_serv_put() and the associated
  percpu_ref, suggested by Chuck.

- Improved "Always allow LOCALIO" comment in check_nfsd_access() and
  added Kdoc above check_nfsd_access(), suggested by Chuck.

- Moved rpcauth_map_clnt_to_svc_cred_local from net/sunrpc/auth.c to
  net/sunrpc/svcauth.c and renamed it to
  svcauth_map_clnt_to_svc_cred_local. Also added kdoc. Suggested by
  Chuck.

- Added Chuck's Acked-by to 2 patches

- Incorporated Chuck's 6 patches that split up and improved the
  __fh_verify and nfsd_file_acquire_local patches.