Re: [PATCH v15 16/26] nfsd: add LOCALIO support

[Mike Snitzer <snitzer@kernel.org>]

On Tue, Sep 03, 2024 at 10:40:28AM -0400, Jeff Layton wrote:
> On Tue, 2024-09-03 at 10:34 -0400, Chuck Lever wrote:
> > On Sat, Aug 31, 2024 at 06:37:36PM -0400, Mike Snitzer wrote:
> > > From: Weston Andros Adamson <dros@primarydata.com>
> > > 
> > > Add server support for bypassing NFS for localhost reads, writes, and
> > > commits. This is only useful when both the client and server are
> > > running on the same host.
> > > 
> > > If nfsd_open_local_fh() fails then the NFS client will both retry and
> > > fallback to normal network-based read, write and commit operations if
> > > localio is no longer supported.
> > > 
> > > Care is taken to ensure the same NFS security mechanisms are used
> > > (authentication, etc) regardless of whether localio or regular NFS
> > > access is used.  The auth_domain established as part of the traditional
> > > NFS client access to the NFS server is also used for localio.  Store
> > > auth_domain for localio in nfsd_uuid_t and transfer it to the client
> > > if it is local to the server.
> > > 
> > > Relative to containers, localio gives the client access to the network
> > > namespace the server has.  This is required to allow the client to
> > > access the server's per-namespace nfsd_net struct.
> > > 
> > > This commit also introduces the use of NFSD's percpu_ref to interlock
> > > nfsd_destroy_serv and nfsd_open_local_fh, to ensure nn->nfsd_serv is
> > > not destroyed while in use by nfsd_open_local_fh and other LOCALIO
> > > client code.
> > > 
> > > CONFIG_NFS_LOCALIO enables NFS server support for LOCALIO.
> > > 
> > > Signed-off-by: Weston Andros Adamson <dros@primarydata.com>
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > Co-developed-by: Mike Snitzer <snitzer@kernel.org>
> > > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > > Co-developed-by: NeilBrown <neilb@suse.de>
> > > Signed-off-by: NeilBrown <neilb@suse.de>
> > > 
> > > Not-Acked-by: Chuck Lever <chuck.lever@oracle.com>
> > > Not-Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > > ---
> > >  fs/nfsd/Makefile           |   1 +
> > >  fs/nfsd/filecache.c        |   2 +-
> > >  fs/nfsd/localio.c          | 112 +++++++++++++++++++++++++++++++++++++
> > >  fs/nfsd/netns.h            |   4 ++
> > >  fs/nfsd/nfsctl.c           |  25 ++++++++-
> > >  fs/nfsd/trace.h            |   3 +-
> > >  fs/nfsd/vfs.h              |   2 +
> > >  include/linux/nfslocalio.h |   8 +++
> > >  8 files changed, 154 insertions(+), 3 deletions(-)
> > >  create mode 100644 fs/nfsd/localio.c
> > > 
> > > diff --git a/fs/nfsd/Makefile b/fs/nfsd/Makefile
> > > index b8736a82e57c..18cbd3fa7691 100644
> > > --- a/fs/nfsd/Makefile
> > > +++ b/fs/nfsd/Makefile
> > > @@ -23,3 +23,4 @@ nfsd-$(CONFIG_NFSD_PNFS) += nfs4layouts.o
> > >  nfsd-$(CONFIG_NFSD_BLOCKLAYOUT) += blocklayout.o blocklayoutxdr.o
> > >  nfsd-$(CONFIG_NFSD_SCSILAYOUT) += blocklayout.o blocklayoutxdr.o
> > >  nfsd-$(CONFIG_NFSD_FLEXFILELAYOUT) += flexfilelayout.o flexfilelayoutxdr.o
> > > +nfsd-$(CONFIG_NFS_LOCALIO) += localio.o
> > > diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
> > > index 89ff380ec31e..348c1b97092e 100644
> > > --- a/fs/nfsd/filecache.c
> > > +++ b/fs/nfsd/filecache.c
> > > @@ -52,7 +52,7 @@
> > >  #define NFSD_FILE_CACHE_UP		     (0)
> > >  
> > >  /* We only care about NFSD_MAY_READ/WRITE for this cache */
> > > -#define NFSD_FILE_MAY_MASK	(NFSD_MAY_READ|NFSD_MAY_WRITE)
> > > +#define NFSD_FILE_MAY_MASK	(NFSD_MAY_READ|NFSD_MAY_WRITE|NFSD_MAY_LOCALIO)
> > >  
> > >  static DEFINE_PER_CPU(unsigned long, nfsd_file_cache_hits);
> > >  static DEFINE_PER_CPU(unsigned long, nfsd_file_acquisitions);
> > > diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
> > > new file mode 100644
> > > index 000000000000..75df709c6903
> > > --- /dev/null
> > > +++ b/fs/nfsd/localio.c
> > > @@ -0,0 +1,112 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * NFS server support for local clients to bypass network stack
> > > + *
> > > + * Copyright (C) 2014 Weston Andros Adamson <dros@primarydata.com>
> > > + * Copyright (C) 2019 Trond Myklebust <trond.myklebust@hammerspace.com>
> > > + * Copyright (C) 2024 Mike Snitzer <snitzer@hammerspace.com>
> > > + * Copyright (C) 2024 NeilBrown <neilb@suse.de>
> > > + */
> > > +
> > > +#include <linux/exportfs.h>
> > > +#include <linux/sunrpc/svcauth.h>
> > > +#include <linux/sunrpc/clnt.h>
> > > +#include <linux/nfs.h>
> > > +#include <linux/nfs_common.h>
> > > +#include <linux/nfslocalio.h>
> > > +#include <linux/string.h>
> > > +
> > > +#include "nfsd.h"
> > > +#include "vfs.h"
> > > +#include "netns.h"
> > > +#include "filecache.h"
> > > +
> > > +static const struct nfsd_localio_operations nfsd_localio_ops = {
> > > +	.nfsd_open_local_fh = nfsd_open_local_fh,
> > > +	.nfsd_file_put_local = nfsd_file_put_local,
> > > +	.nfsd_file_file = nfsd_file_file,
> > > +};
> > > +
> > > +void nfsd_localio_ops_init(void)
> > > +{
> > > +	memcpy(&nfs_to, &nfsd_localio_ops, sizeof(nfsd_localio_ops));
> > > +}
> > 
> > Same comment as Neil: this should surface a pointer to the
> > localio_ops struct. Copying the whole set of function pointers is
> > generally unnecessary.
> > 
> > 
> > > +
> > > +/**
> > > + * nfsd_open_local_fh - lookup a local filehandle @nfs_fh and map to nfsd_file
> > > + *
> > > + * @uuid: nfs_uuid_t which provides the 'struct net' to get the proper nfsd_net
> > > + *        and the 'struct auth_domain' required for LOCALIO access
> > > + * @rpc_clnt: rpc_clnt that the client established, used for sockaddr and cred
> > > + * @cred: cred that the client established
> > > + * @nfs_fh: filehandle to lookup
> > > + * @fmode: fmode_t to use for open
> > > + *
> > > + * This function maps a local fh to a path on a local filesystem.
> > > + * This is useful when the nfs client has the local server mounted - it can
> > > + * avoid all the NFS overhead with reads, writes and commits.
> > > + *
> > > + * On successful return, returned nfsd_file will have its nf_net member
> > > + * set. Caller (NFS client) is responsible for calling nfsd_serv_put and
> > > + * nfsd_file_put (via nfs_to.nfsd_file_put_local).
> > > + */
> > > +struct nfsd_file *
> > > +nfsd_open_local_fh(nfs_uuid_t *uuid,
> > > +		   struct rpc_clnt *rpc_clnt, const struct cred *cred,
> > > +		   const struct nfs_fh *nfs_fh, const fmode_t fmode)
> > > +	__must_hold(rcu)
> > > +{
> > > +	int mayflags = NFSD_MAY_LOCALIO;
> > > +	struct nfsd_net *nn = NULL;
> > > +	struct net *net;
> > > +	struct svc_cred rq_cred;
> > > +	struct svc_fh fh;
> > > +	struct nfsd_file *localio;
> > > +	__be32 beres;
> > > +
> > > +	if (nfs_fh->size > NFS4_FHSIZE)
> > > +		return ERR_PTR(-EINVAL);
> > > +
> > > +	/*
> > > +	 * Not running in nfsd context, so must safely get reference on nfsd_serv.
> > > +	 * But the server may already be shutting down, if so disallow new localio.
> > > +	 * uuid->net is NOT a counted reference, but caller's rcu_read_lock() ensures
> > > +	 * that if uuid->net is not NULL, then calling nfsd_serv_try_get() is safe
> > > +	 * and if it succeeds we will have an implied reference to the net.
> > > +	 */
> > > +	net = rcu_dereference(uuid->net);
> > > +	if (net)
> > > +		nn = net_generic(net, nfsd_net_id);
> > > +	if (unlikely(!nn || !nfsd_serv_try_get(nn)))
> > > +		return ERR_PTR(-ENXIO);
> > > +
> > > +	/* Drop the rcu lock for nfsd_file_acquire_local() */
> > > +	rcu_read_unlock();
> > 
> > I'm struggling with the locking logistics. Caller takes the RCU read
> > lock, this function drops the lock, then takes it again. So:
> > 
> >  - A caller might rely on the lock being held continuously, but
> >  - The API contract documented above doesn't indicate that this
> >    function drops that lock
> >  - The __must_hold(rcu) annotation doesn't indicate that this
> >    function drops that lock, IIUC
> > 
> > Dropping and retaking the lock in here is an anti-pattern that
> > should be avoided. I suggest we are better off in the long run if
> > the caller does not need to take the RCU read lock, but instead,
> > nfsd_open_local_fh takes it right here just for the rcu_dereference.

I thought so too when I first saw how Neil approached fixing this to
be safe.  It was only after putting further time to it (and having the
benefit of being so close to all this) that I realized the nuance at
play (please see my reply to Jeff below for the nuance I'm speaking
of). 

> > 
> > OTOH, Why drop the lock before calling nfsd_file_acquire_local()?
> > The RCU read lock can safely be taken more than once in succession.
> > 
> > Let's rethink the locking strategy.
> > 

Yes, _that_ is a very valid point.  I did wonder the same: it seems
perfectly fine to simply retain the RCU throughout the entirety of
nfsd_open_local_fh().

> Agreed. The only caller does this:
> 
>         rcu_read_lock();
>         if (!rcu_access_pointer(uuid->net)) {
>                 rcu_read_unlock();
>                 return ERR_PTR(-ENXIO);
>         }
>         localio = nfs_to.nfsd_open_local_fh(uuid, rpc_clnt, cred,
>                                             nfs_fh, fmode);
>         rcu_read_unlock();
> 
> Maybe just move the check for uuid->net down into nfsd_open_local_fh,
> and it can acquire the rcu_read_lock for itself?

No, sorry we cannot.  The call to nfs_to.nfsd_open_local_fh (which is
a symbol provided by nfsd) is only safe if the RCU protected pre-check
shows the uuid->net valid.

Mike