Re: [for-6.13 PATCH 10/19] nfs_common: move localio_lock to new lock member of nfs_uuid_t

[Mike Snitzer <snitzer@kernel.org>]

On Mon, Nov 11, 2024 at 12:55:24PM +1100, NeilBrown wrote:
> On Sat, 09 Nov 2024, Mike Snitzer wrote:
> > Remove cl_localio_lock from 'struct nfs_client' in favor of adding a
> > lock to the nfs_uuid_t struct (which is embedded in each nfs_client).
> > 
> > Push nfs_local_{enable,disable} implementation down to nfs_common.
> > Those methods now call nfs_localio_{enable,disable}_client.
> > 
> > This allows implementing nfs_localio_invalidate_clients in terms of
> > nfs_localio_disable_client.
> > 
> > Signed-off-by: Mike Snitzer <snitzer@kernel.org>
> > ---
> >  fs/nfs/client.c            |  1 -
> >  fs/nfs/localio.c           | 18 ++++++------
> >  fs/nfs_common/nfslocalio.c | 57 ++++++++++++++++++++++++++------------
> >  include/linux/nfs_fs_sb.h  |  1 -
> >  include/linux/nfslocalio.h |  8 +++++-
> >  5 files changed, 55 insertions(+), 30 deletions(-)
> > 
> > diff --git a/fs/nfs/client.c b/fs/nfs/client.c
> > index 03ecc7765615..124232054807 100644
> > --- a/fs/nfs/client.c
> > +++ b/fs/nfs/client.c
> > @@ -182,7 +182,6 @@ struct nfs_client *nfs_alloc_client(const struct nfs_client_initdata *cl_init)
> >  	seqlock_init(&clp->cl_boot_lock);
> >  	ktime_get_real_ts64(&clp->cl_nfssvc_boot);
> >  	nfs_uuid_init(&clp->cl_uuid);
> > -	spin_lock_init(&clp->cl_localio_lock);
> >  #endif /* CONFIG_NFS_LOCALIO */
> >  
> >  	clp->cl_principal = "*";
> > diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
> > index cab2a8819259..4c75ffc5efa2 100644
> > --- a/fs/nfs/localio.c
> > +++ b/fs/nfs/localio.c
> > @@ -125,10 +125,8 @@ const struct rpc_program nfslocalio_program = {
> >   */
> >  static void nfs_local_enable(struct nfs_client *clp)
> >  {
> > -	spin_lock(&clp->cl_localio_lock);
> > -	set_bit(NFS_CS_LOCAL_IO, &clp->cl_flags);
> >  	trace_nfs_local_enable(clp);
> > -	spin_unlock(&clp->cl_localio_lock);
> > +	nfs_localio_enable_client(clp);
> >  }
> 
> Why do we need this function?  The one caller could call
> nfs_localio_enable_client() directly instead.  The tracepoint could be
> placed in that one caller.

Yeah, I saw that too but felt it useful to differentiate between calls
that occur during NFS client initialization and those that happen as a
side-effect of callers from other contexts (in later patch this
manifests as nfs_localio_disable_client vs nfs_local_disable).

Hence my adding secondary tracepoints for nfs_common (see "[PATCH
17/19] nfs_common: add nfs_localio trace events).

But sure, we can just eliminate nfs_local_{enable,disable} and the
corresponding tracepoints (which will have moved down to nfs_common).

> >  /*
> > @@ -136,12 +134,8 @@ static void nfs_local_enable(struct nfs_client *clp)
> >   */
> >  void nfs_local_disable(struct nfs_client *clp)
> >  {
> > -	spin_lock(&clp->cl_localio_lock);
> > -	if (test_and_clear_bit(NFS_CS_LOCAL_IO, &clp->cl_flags)) {
> > -		trace_nfs_local_disable(clp);
> > -		nfs_localio_disable_client(&clp->cl_uuid);
> > -	}
> > -	spin_unlock(&clp->cl_localio_lock);
> > +	trace_nfs_local_disable(clp);
> > +	nfs_localio_disable_client(clp);
> >  }
> 
> Ditto.  Though there are more callers so the tracepoint solution isn't
> quite so obvious.

Right... as I just explained: that's why I preserved nfs_local_disable
(and the tracepoint).


> >  /*
> > @@ -183,8 +177,12 @@ static bool nfs_server_uuid_is_local(struct nfs_client *clp)
> >  	rpc_shutdown_client(rpcclient_localio);
> >  
> >  	/* Server is only local if it initialized required struct members */
> > -	if (status || !clp->cl_uuid.net || !clp->cl_uuid.dom)
> > +	rcu_read_lock();
> > +	if (status || !rcu_access_pointer(clp->cl_uuid.net) || !clp->cl_uuid.dom) {
> > +		rcu_read_unlock();
> >  		return false;
> > +	}
> > +	rcu_read_unlock();
> 
> What value does RCU provide here?  I don't think this change is needed.
> rcu_access_pointer does not require rcu_read_lock().

OK, not sure why I though RCU read-side needed for rcu_access_pointer()...

> >  	return true;
> >  }
> > diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
> > index 904439e4bb85..cf2f47ea4f8d 100644
> > --- a/fs/nfs_common/nfslocalio.c
> > +++ b/fs/nfs_common/nfslocalio.c
> > @@ -7,6 +7,9 @@
> >  #include <linux/module.h>
> >  #include <linux/list.h>
> >  #include <linux/nfslocalio.h>
> > +#include <linux/nfs3.h>
> > +#include <linux/nfs4.h>
> > +#include <linux/nfs_fs_sb.h>
> 
> I don't feel good about adding this nfs client knowledge in to nfs_common.

I hear you.. I was "OK with it".

> >  #include <net/netns/generic.h>
> >  
> >  MODULE_LICENSE("GPL");
> > @@ -25,6 +28,7 @@ void nfs_uuid_init(nfs_uuid_t *nfs_uuid)
> >  	nfs_uuid->net = NULL;
> >  	nfs_uuid->dom = NULL;
> >  	INIT_LIST_HEAD(&nfs_uuid->list);
> > +	spin_lock_init(&nfs_uuid->lock);
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_uuid_init);
> >  
> > @@ -94,12 +98,23 @@ void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list,
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_uuid_is_local);
> >  
> > +void nfs_localio_enable_client(struct nfs_client *clp)
> > +{
> > +	nfs_uuid_t *nfs_uuid = &clp->cl_uuid;
> > +
> > +	spin_lock(&nfs_uuid->lock);
> > +	set_bit(NFS_CS_LOCAL_IO, &clp->cl_flags);
> > +	spin_unlock(&nfs_uuid->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(nfs_localio_enable_client);
> 
> And I don't feel good about nfs_local accessing nfs_client directly.
> It only uses it for NFS_CS_LOCAL_IO.  Can we ditch that flag and instead
> so something like testing nfs_uuid.net ??

That'd probably be OK for the equivalent of NFS_CS_LOCAL_IO but the last
patch in this series ("nfs: probe for LOCALIO when v3 client
reconnects to server") adds NFS_CS_LOCAL_IO_CAPABLE to provide a hint
that the client and server successfully established themselves local
via LOCALIO protocol.  This is needed so that NFSv3 (stateless) has a
hint that reestablishing LOCALIO needed if/when the client loses
connectivity to the server (because it was shutdown and restarted).

Could introduce flags local to nfs_uuid_t structure as a means of
nfs_common/nfslocalio.c not needing to know internals of nfs_client at
all -- conversely: probably best to not bloat nfs_uuid_t (and
nfs_client) further, so should just ensure nfs_client treated as an
opaque pointer in nfs_common by introducing accessors?

> > +
> >  static void nfs_uuid_put_locked(nfs_uuid_t *nfs_uuid)
> >  {
> > -	if (nfs_uuid->net) {
> > -		module_put(nfsd_mod);
> > -		nfs_uuid->net = NULL;
> > -	}
> > +	if (!nfs_uuid->net)
> > +		return;
> > +	module_put(nfsd_mod);
> > +	rcu_assign_pointer(nfs_uuid->net, NULL);
> > +
> 
> I much prefer RCU_INIT_POINTER for assigning NULL as there is no need
> for ordering here.

OK.

> >  	if (nfs_uuid->dom) {
> >  		auth_domain_put(nfs_uuid->dom);
> >  		nfs_uuid->dom = NULL;
> > @@ -107,27 +122,35 @@ static void nfs_uuid_put_locked(nfs_uuid_t *nfs_uuid)
> >  	list_del_init(&nfs_uuid->list);
> >  }
> >  
> > -void nfs_localio_invalidate_clients(struct list_head *list)
> > +void nfs_localio_disable_client(struct nfs_client *clp)
> > +{
> > +	nfs_uuid_t *nfs_uuid = &clp->cl_uuid;
> > +
> > +	spin_lock(&nfs_uuid->lock);
> > +	if (test_and_clear_bit(NFS_CS_LOCAL_IO, &clp->cl_flags)) {
> > +		spin_lock(&nfs_uuid_lock);
> > +		nfs_uuid_put_locked(nfs_uuid);
> > +		spin_unlock(&nfs_uuid_lock);
> > +	}
> > +	spin_unlock(&nfs_uuid->lock);
> > +}
> > +EXPORT_SYMBOL_GPL(nfs_localio_disable_client);
> > +
> > +void nfs_localio_invalidate_clients(struct list_head *cl_uuid_list)
> >  {
> >  	nfs_uuid_t *nfs_uuid, *tmp;
> >  
> >  	spin_lock(&nfs_uuid_lock);
> > -	list_for_each_entry_safe(nfs_uuid, tmp, list, list)
> > -		nfs_uuid_put_locked(nfs_uuid);
> > +	list_for_each_entry_safe(nfs_uuid, tmp, cl_uuid_list, list) {
> > +		struct nfs_client *clp =
> > +			container_of(nfs_uuid, struct nfs_client, cl_uuid);
> > +
> > +		nfs_localio_disable_client(clp);
> > +	}
> >  	spin_unlock(&nfs_uuid_lock);
> >  }
> >  EXPORT_SYMBOL_GPL(nfs_localio_invalidate_clients);
> >  
> > -void nfs_localio_disable_client(nfs_uuid_t *nfs_uuid)
> > -{
> > -	if (nfs_uuid->net) {
> > -		spin_lock(&nfs_uuid_lock);
> > -		nfs_uuid_put_locked(nfs_uuid);
> > -		spin_unlock(&nfs_uuid_lock);
> > -	}
> > -}
> > -EXPORT_SYMBOL_GPL(nfs_localio_disable_client);
> > -
> >  struct nfsd_file *nfs_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)
> > diff --git a/include/linux/nfs_fs_sb.h b/include/linux/nfs_fs_sb.h
> > index b804346a9741..239d86ef166c 100644
> > --- a/include/linux/nfs_fs_sb.h
> > +++ b/include/linux/nfs_fs_sb.h
> > @@ -132,7 +132,6 @@ struct nfs_client {
> >  	struct timespec64	cl_nfssvc_boot;
> >  	seqlock_t		cl_boot_lock;
> >  	nfs_uuid_t		cl_uuid;
> > -	spinlock_t		cl_localio_lock;
> >  #endif /* CONFIG_NFS_LOCALIO */
> >  };
> >  
> > diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
> > index a05d1043f2b0..4d5583873f41 100644
> > --- a/include/linux/nfslocalio.h
> > +++ b/include/linux/nfslocalio.h
> > @@ -6,6 +6,7 @@
> >  #ifndef __LINUX_NFSLOCALIO_H
> >  #define __LINUX_NFSLOCALIO_H
> >  
> > +
> >  /* nfsd_file structure is purposely kept opaque to NFS client */
> >  struct nfsd_file;
> >  
> > @@ -19,6 +20,8 @@ struct nfsd_file;
> >  #include <linux/nfs.h>
> >  #include <net/net_namespace.h>
> >  
> > +struct nfs_client;
> > +
> >  /*
> >   * Useful to allow a client to negotiate if localio
> >   * possible with its server.
> > @@ -27,6 +30,8 @@ struct nfsd_file;
> >   */
> >  typedef struct {
> >  	uuid_t uuid;
> > +	/* sadly this struct is just over a cacheline, avoid bouncing */
> > +	spinlock_t ____cacheline_aligned lock;
> >  	struct list_head list;
> >  	struct net __rcu *net; /* nfsd's network namespace */
> >  	struct auth_domain *dom; /* auth_domain for localio */
> > @@ -38,7 +43,8 @@ void nfs_uuid_end(nfs_uuid_t *);
> >  void nfs_uuid_is_local(const uuid_t *, struct list_head *,
> >  		       struct net *, struct auth_domain *, struct module *);
> >  
> > -void nfs_localio_disable_client(nfs_uuid_t *nfs_uuid);
> > +void nfs_localio_enable_client(struct nfs_client *clp);
> > +void nfs_localio_disable_client(struct nfs_client *clp);
> >  void nfs_localio_invalidate_clients(struct list_head *list);
> >  
> >  /* localio needs to map filehandle -> struct nfsd_file */
> > -- 
> > 2.44.0
> > 
> > 
> 
> I think this is a good refactoring to do, but I don't like some of the
> details, or some of the RCU code.

Sure, I'll clean it up further.

Thanks for your review.

Mike