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@xxxxxxxxxx>
> ---
> 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.
>
> /*
> @@ -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.
>
> /*
> @@ -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().
>
> 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.
> #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 ??
> +
> 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.
> 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.
NeilBrown
