From: Mike Snitzer <snitzer@kernel.org>
To: linux-nfs@vger.kernel.org
Cc: Anna Schumaker <anna@kernel.org>,
Trond Myklebust <trondmy@hammerspace.com>,
Chuck Lever <chuck.lever@oracle.com>,
Jeff Layton <jlayton@kernel.org>, NeilBrown <neilb@suse.de>
Subject: [for-6.13 PATCH 16/19] nfs_common: track all open nfsd_files per LOCALIO nfs_client
Date: Fri, 8 Nov 2024 18:39:59 -0500 [thread overview]
Message-ID: <20241108234002.16392-17-snitzer@kernel.org> (raw)
In-Reply-To: <20241108234002.16392-1-snitzer@kernel.org>
This tracking enables __nfsd_file_cache_purge() to call
nfs_localio_invalidate_clients(), upon shutdown or export change, to
nfs_close_local_fh() all open nfsd_files that are still cached by the
LOCALIO nfs clients associated with nfsd_net that is being shutdown.
Now that the client must track all open nfsd_files there was more work
than necessary being done with the global nfs_uuids_lock contended.
This manifested in various RCU issues, e.g.:
hrtimer: interrupt took 47969440 ns
rcu: INFO: rcu_sched detected stalls on CPUs/tasks:
Use nfs_uuid->lock to protect all nfs_uuid_t members, instead of
nfs_uuids_lock, once nfs_uuid_is_local() adds the client to
nn->local_clients.
Also add 'local_clients_lock' to 'struct nfsd_net' to protect
nn->local_clients. And store a pointer to spinlock in the 'list_lock'
member of nfs_uuid_t so nfs_localio_disable_client() can use it to
avoid taking the global nfs_uuids_lock.
In combination, these split out locks eliminate the use of the single
nfslocalio.c global nfs_uuids_lock in the IO paths (open and close).
Also refactored associated fs/nfs_common/nfslocalio.c methods' locking
to reduce work performed with spinlocks held in general.
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
fs/nfs_common/nfslocalio.c | 166 +++++++++++++++++++++++++++----------
fs/nfsd/filecache.c | 9 ++
fs/nfsd/localio.c | 1 +
fs/nfsd/netns.h | 1 +
fs/nfsd/nfsctl.c | 4 +-
include/linux/nfslocalio.h | 8 +-
6 files changed, 143 insertions(+), 46 deletions(-)
diff --git a/fs/nfs_common/nfslocalio.c b/fs/nfs_common/nfslocalio.c
index e58b5b4b4c3a..c8d18f671bcb 100644
--- a/fs/nfs_common/nfslocalio.c
+++ b/fs/nfs_common/nfslocalio.c
@@ -23,27 +23,49 @@ static DEFINE_SPINLOCK(nfs_uuids_lock);
*/
static LIST_HEAD(nfs_uuids);
+/*
+ * Lock ordering:
+ * 1: nfs_uuid->lock
+ * 2: nfs_uuids_lock
+ * 3: nfs_uuid->list_lock (aka nn->local_clients_lock)
+ *
+ * May skip locks in select cases, but never hold multiple
+ * locks out of order.
+ */
+
void nfs_uuid_init(nfs_uuid_t *nfs_uuid)
{
nfs_uuid->net = NULL;
nfs_uuid->dom = NULL;
+ nfs_uuid->list_lock = NULL;
INIT_LIST_HEAD(&nfs_uuid->list);
+ INIT_LIST_HEAD(&nfs_uuid->files);
spin_lock_init(&nfs_uuid->lock);
}
EXPORT_SYMBOL_GPL(nfs_uuid_init);
bool nfs_uuid_begin(nfs_uuid_t *nfs_uuid)
{
+ spin_lock(&nfs_uuid->lock);
+ if (nfs_uuid->net) {
+ /* This nfs_uuid is already in use */
+ spin_unlock(&nfs_uuid->lock);
+ return false;
+ }
+
spin_lock(&nfs_uuids_lock);
- /* Is this nfs_uuid already in use? */
if (!list_empty(&nfs_uuid->list)) {
+ /* This nfs_uuid is already in use */
spin_unlock(&nfs_uuids_lock);
+ spin_unlock(&nfs_uuid->lock);
return false;
}
- uuid_gen(&nfs_uuid->uuid);
list_add_tail(&nfs_uuid->list, &nfs_uuids);
spin_unlock(&nfs_uuids_lock);
+ uuid_gen(&nfs_uuid->uuid);
+ spin_unlock(&nfs_uuid->lock);
+
return true;
}
EXPORT_SYMBOL_GPL(nfs_uuid_begin);
@@ -51,11 +73,15 @@ EXPORT_SYMBOL_GPL(nfs_uuid_begin);
void nfs_uuid_end(nfs_uuid_t *nfs_uuid)
{
if (nfs_uuid->net == NULL) {
- spin_lock(&nfs_uuids_lock);
- if (nfs_uuid->net == NULL)
+ spin_lock(&nfs_uuid->lock);
+ if (nfs_uuid->net == NULL) {
+ /* Not local, remove from nfs_uuids */
+ spin_lock(&nfs_uuids_lock);
list_del_init(&nfs_uuid->list);
- spin_unlock(&nfs_uuids_lock);
- }
+ spin_unlock(&nfs_uuids_lock);
+ }
+ spin_unlock(&nfs_uuid->lock);
+ }
}
EXPORT_SYMBOL_GPL(nfs_uuid_end);
@@ -73,28 +99,39 @@ static nfs_uuid_t * nfs_uuid_lookup_locked(const uuid_t *uuid)
static struct module *nfsd_mod;
void nfs_uuid_is_local(const uuid_t *uuid, struct list_head *list,
- struct net *net, struct auth_domain *dom,
- struct module *mod)
+ spinlock_t *list_lock, struct net *net,
+ struct auth_domain *dom, struct module *mod)
{
nfs_uuid_t *nfs_uuid;
spin_lock(&nfs_uuids_lock);
nfs_uuid = nfs_uuid_lookup_locked(uuid);
- if (nfs_uuid) {
- kref_get(&dom->ref);
- nfs_uuid->dom = dom;
- /*
- * We don't hold a ref on the net, but instead put
- * ourselves on a list so the net pointer can be
- * invalidated.
- */
- list_move(&nfs_uuid->list, list);
- rcu_assign_pointer(nfs_uuid->net, net);
-
- __module_get(mod);
- nfsd_mod = mod;
+ if (!nfs_uuid) {
+ spin_unlock(&nfs_uuids_lock);
+ return;
}
+
+ /*
+ * We don't hold a ref on the net, but instead put
+ * ourselves on @list (nn->local_clients) so the net
+ * pointer can be invalidated.
+ */
+ spin_lock(list_lock); /* list_lock is nn->local_clients_lock */
+ list_move(&nfs_uuid->list, list);
+ spin_unlock(list_lock);
+
spin_unlock(&nfs_uuids_lock);
+ /* Once nfs_uuid is parented to @list, avoid global nfs_uuids_lock */
+ spin_lock(&nfs_uuid->lock);
+
+ __module_get(mod);
+ nfsd_mod = mod;
+
+ nfs_uuid->list_lock = list_lock;
+ kref_get(&dom->ref);
+ nfs_uuid->dom = dom;
+ rcu_assign_pointer(nfs_uuid->net, net);
+ spin_unlock(&nfs_uuid->lock);
}
EXPORT_SYMBOL_GPL(nfs_uuid_is_local);
@@ -108,55 +145,96 @@ void nfs_localio_enable_client(struct nfs_client *clp)
}
EXPORT_SYMBOL_GPL(nfs_localio_enable_client);
-static void nfs_uuid_put_locked(nfs_uuid_t *nfs_uuid)
+/*
+ * Cleanup the nfs_uuid_t embedded in an nfs_client.
+ * This is the long-form of nfs_uuid_init().
+ */
+static void nfs_uuid_put(nfs_uuid_t *nfs_uuid)
{
- if (!nfs_uuid->net)
+ LIST_HEAD(local_files);
+ struct nfs_file_localio *nfl, *tmp;
+
+ spin_lock(&nfs_uuid->lock);
+ if (unlikely(!nfs_uuid->net)) {
+ spin_unlock(&nfs_uuid->lock);
return;
- module_put(nfsd_mod);
+ }
rcu_assign_pointer(nfs_uuid->net, NULL);
if (nfs_uuid->dom) {
auth_domain_put(nfs_uuid->dom);
nfs_uuid->dom = NULL;
}
- list_del_init(&nfs_uuid->list);
+
+ list_splice_init(&nfs_uuid->files, &local_files);
+ spin_unlock(&nfs_uuid->lock);
+
+ /* Walk list of files and ensure their last references dropped */
+ list_for_each_entry_safe(nfl, tmp, &local_files, list) {
+ nfs_close_local_fh(nfl);
+ cond_resched();
+ }
+
+ spin_lock(&nfs_uuid->lock);
+ BUG_ON(!list_empty(&nfs_uuid->files));
+
+ /* Remove client from nn->local_clients */
+ if (nfs_uuid->list_lock) {
+ spin_lock(nfs_uuid->list_lock);
+ BUG_ON(list_empty(&nfs_uuid->list));
+ list_del_init(&nfs_uuid->list);
+ spin_unlock(nfs_uuid->list_lock);
+ nfs_uuid->list_lock = NULL;
+ }
+
+ module_put(nfsd_mod);
+ spin_unlock(&nfs_uuid->lock);
}
void nfs_localio_disable_client(struct nfs_client *clp)
{
- nfs_uuid_t *nfs_uuid = &clp->cl_uuid;
+ nfs_uuid_t *nfs_uuid = NULL;
- spin_lock(&nfs_uuid->lock);
+ spin_lock(&clp->cl_uuid.lock); /* aka &nfs_uuid->lock */
if (test_and_clear_bit(NFS_CS_LOCAL_IO, &clp->cl_flags)) {
- spin_lock(&nfs_uuids_lock);
- nfs_uuid_put_locked(nfs_uuid);
- spin_unlock(&nfs_uuids_lock);
+ /* &clp->cl_uuid is always not NULL, using as bool here */
+ nfs_uuid = &clp->cl_uuid;
}
- spin_unlock(&nfs_uuid->lock);
+ spin_unlock(&clp->cl_uuid.lock);
+
+ if (nfs_uuid)
+ nfs_uuid_put(nfs_uuid);
}
EXPORT_SYMBOL_GPL(nfs_localio_disable_client);
-void nfs_localio_invalidate_clients(struct list_head *cl_uuid_list)
+void nfs_localio_invalidate_clients(struct list_head *nn_local_clients,
+ spinlock_t *nn_local_clients_lock)
{
+ LIST_HEAD(local_clients);
nfs_uuid_t *nfs_uuid, *tmp;
+ struct nfs_client *clp;
- spin_lock(&nfs_uuids_lock);
- 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);
-
+ spin_lock(nn_local_clients_lock);
+ list_splice_init(nn_local_clients, &local_clients);
+ spin_unlock(nn_local_clients_lock);
+ list_for_each_entry_safe(nfs_uuid, tmp, &local_clients, list) {
+ if (WARN_ON(nfs_uuid->list_lock != nn_local_clients_lock))
+ break;
+ clp = container_of(nfs_uuid, struct nfs_client, cl_uuid);
nfs_localio_disable_client(clp);
}
- spin_unlock(&nfs_uuids_lock);
}
EXPORT_SYMBOL_GPL(nfs_localio_invalidate_clients);
static void nfs_uuid_add_file(nfs_uuid_t *nfs_uuid, struct nfs_file_localio *nfl)
{
- spin_lock(&nfs_uuids_lock);
- if (!nfl->nfs_uuid)
+ /* Add nfl to nfs_uuid->files if it isn't already */
+ spin_lock(&nfs_uuid->lock);
+ if (list_empty(&nfl->list)) {
rcu_assign_pointer(nfl->nfs_uuid, nfs_uuid);
- spin_unlock(&nfs_uuids_lock);
+ list_add_tail(&nfl->list, &nfs_uuid->files);
+ }
+ spin_unlock(&nfs_uuid->lock);
}
/*
@@ -217,14 +295,16 @@ void nfs_close_local_fh(struct nfs_file_localio *nfl)
ro_nf = rcu_access_pointer(nfl->ro_file);
rw_nf = rcu_access_pointer(nfl->rw_file);
if (ro_nf || rw_nf) {
- spin_lock(&nfs_uuids_lock);
+ spin_lock(&nfs_uuid->lock);
if (ro_nf)
ro_nf = rcu_dereference_protected(xchg(&nfl->ro_file, NULL), 1);
if (rw_nf)
rw_nf = rcu_dereference_protected(xchg(&nfl->rw_file, NULL), 1);
+ /* Remove nfl from nfs_uuid->files list */
rcu_assign_pointer(nfl->nfs_uuid, NULL);
- spin_unlock(&nfs_uuids_lock);
+ list_del_init(&nfl->list);
+ spin_unlock(&nfs_uuid->lock);
rcu_read_unlock();
if (ro_nf)
diff --git a/fs/nfsd/filecache.c b/fs/nfsd/filecache.c
index ab9942e42054..c9ab64e3732c 100644
--- a/fs/nfsd/filecache.c
+++ b/fs/nfsd/filecache.c
@@ -39,6 +39,7 @@
#include <linux/fsnotify.h>
#include <linux/seq_file.h>
#include <linux/rhashtable.h>
+#include <linux/nfslocalio.h>
#include "vfs.h"
#include "nfsd.h"
@@ -836,6 +837,14 @@ __nfsd_file_cache_purge(struct net *net)
struct nfsd_file *nf;
LIST_HEAD(dispose);
+#if IS_ENABLED(CONFIG_NFS_LOCALIO)
+ if (net) {
+ struct nfsd_net *nn = net_generic(net, nfsd_net_id);
+ nfs_localio_invalidate_clients(&nn->local_clients,
+ &nn->local_clients_lock);
+ }
+#endif
+
rhltable_walk_enter(&nfsd_file_rhltable, &iter);
do {
rhashtable_walk_start(&iter);
diff --git a/fs/nfsd/localio.c b/fs/nfsd/localio.c
index 2ae07161b919..238647fa379e 100644
--- a/fs/nfsd/localio.c
+++ b/fs/nfsd/localio.c
@@ -116,6 +116,7 @@ static __be32 localio_proc_uuid_is_local(struct svc_rqst *rqstp)
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
nfs_uuid_is_local(&argp->uuid, &nn->local_clients,
+ &nn->local_clients_lock,
net, rqstp->rq_client, THIS_MODULE);
return rpc_success;
diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
index 8faef59d7122..187c4140b191 100644
--- a/fs/nfsd/netns.h
+++ b/fs/nfsd/netns.h
@@ -220,6 +220,7 @@ struct nfsd_net {
#if IS_ENABLED(CONFIG_NFS_LOCALIO)
/* Local clients to be invalidated when net is shut down */
+ spinlock_t local_clients_lock;
struct list_head local_clients;
#endif
};
diff --git a/fs/nfsd/nfsctl.c b/fs/nfsd/nfsctl.c
index 727904d8a4d0..70347b0ecdc4 100644
--- a/fs/nfsd/nfsctl.c
+++ b/fs/nfsd/nfsctl.c
@@ -2259,6 +2259,7 @@ static __net_init int nfsd_net_init(struct net *net)
seqlock_init(&nn->writeverf_lock);
nfsd_proc_stat_init(net);
#if IS_ENABLED(CONFIG_NFS_LOCALIO)
+ spin_lock_init(&nn->local_clients_lock);
INIT_LIST_HEAD(&nn->local_clients);
#endif
return 0;
@@ -2283,7 +2284,8 @@ static __net_exit void nfsd_net_pre_exit(struct net *net)
{
struct nfsd_net *nn = net_generic(net, nfsd_net_id);
- nfs_localio_invalidate_clients(&nn->local_clients);
+ nfs_localio_invalidate_clients(&nn->local_clients,
+ &nn->local_clients_lock);
}
#endif
diff --git a/include/linux/nfslocalio.h b/include/linux/nfslocalio.h
index aa2b5c6561ab..c68a529230c1 100644
--- a/include/linux/nfslocalio.h
+++ b/include/linux/nfslocalio.h
@@ -30,19 +30,23 @@ typedef struct {
/* sadly this struct is just over a cacheline, avoid bouncing */
spinlock_t ____cacheline_aligned lock;
struct list_head list;
+ spinlock_t *list_lock; /* nn->local_clients_lock */
struct net __rcu *net; /* nfsd's network namespace */
struct auth_domain *dom; /* auth_domain for localio */
+ /* Local files to close when net is shut down or exports change */
+ struct list_head files;
} nfs_uuid_t;
void nfs_uuid_init(nfs_uuid_t *);
bool nfs_uuid_begin(nfs_uuid_t *);
void nfs_uuid_end(nfs_uuid_t *);
-void nfs_uuid_is_local(const uuid_t *, struct list_head *,
+void nfs_uuid_is_local(const uuid_t *, struct list_head *, spinlock_t *,
struct net *, struct auth_domain *, struct module *);
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);
+void nfs_localio_invalidate_clients(struct list_head *nn_local_clients,
+ spinlock_t *nn_local_clients_lock);
/* localio needs to map filehandle -> struct nfsd_file */
extern struct nfsd_file *
--
2.44.0
next prev parent reply other threads:[~2024-11-08 23:40 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-11-08 23:39 [for-6.13 PATCH 00/19] nfs/nfsd: fixes and improvements for LOCALIO Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 01/19] nfs/localio: must clear res.replen in nfs_local_read_done Mike Snitzer
2024-11-11 0:36 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 02/19] nfs_common: must not hold RCU while calling nfsd_file_put_local Mike Snitzer
2024-11-11 1:01 ` NeilBrown
2024-11-13 14:58 ` Jeff Layton
2024-11-13 16:51 ` Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 03/19] nfs/localio: remove redundant suid/sgid handling Mike Snitzer
2024-11-11 1:09 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 04/19] nfs/localio: eliminate unnecessary kref in nfs_local_fsync_ctx Mike Snitzer
2024-11-11 1:15 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 05/19] nfs/localio: remove extra indirect nfs_to call to check {read,write}_iter Mike Snitzer
2024-11-11 1:20 ` NeilBrown
2024-11-11 15:09 ` Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 06/19] nfs/localio: eliminate need for nfs_local_fsync_work forward declaration Mike Snitzer
2024-11-11 1:21 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 07/19] nfs/localio: add direct IO enablement with sync and async IO support Mike Snitzer
2024-11-11 1:31 ` NeilBrown
2024-11-12 14:31 ` Chuck Lever
2024-11-08 23:39 ` [for-6.13 PATCH 08/19] nfsd: add nfsd_file_{get,put} to 'nfs_to' nfsd_localio_operations Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 09/19] nfs_common: rename functions that invalidate LOCALIO nfs_clients Mike Snitzer
2024-11-11 1:32 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 10/19] nfs_common: move localio_lock to new lock member of nfs_uuid_t Mike Snitzer
2024-11-11 1:55 ` NeilBrown
2024-11-11 15:33 ` Mike Snitzer
2024-11-11 20:35 ` NeilBrown
2024-11-11 22:27 ` Mike Snitzer
2024-11-11 23:23 ` NeilBrown
2024-11-12 0:16 ` Mike Snitzer
2024-11-12 0:49 ` NeilBrown
2024-11-12 14:36 ` Chuck Lever
2024-11-12 23:13 ` NeilBrown
2024-11-13 0:07 ` Chuck Lever III
2024-11-13 0:32 ` NeilBrown
2024-11-08 23:39 ` [for-6.13 PATCH 11/19] nfs: cache all open LOCALIO nfsd_file(s) in client Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 12/19] nfsd: update percpu_ref to manage references on nfsd_net Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 13/19] nfsd: rename nfsd_serv_ prefixed methods and variables with nfsd_net_ Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 14/19] nfsd: nfsd_file_acquire_local no longer returns GC'd nfsd_file Mike Snitzer
2024-11-08 23:39 ` [for-6.13 PATCH 15/19] nfs_common: rename nfslocalio nfs_uuid_lock to nfs_uuids_lock Mike Snitzer
2024-11-08 23:39 ` Mike Snitzer [this message]
2024-11-08 23:40 ` [for-6.13 PATCH 17/19] nfs_common: add nfs_localio trace events Mike Snitzer
2024-11-08 23:40 ` [for-6.13 PATCH 18/19] nfs: probe for LOCALIO when v4 client reconnects to server Mike Snitzer
2024-11-08 23:40 ` [for-6.13 PATCH 19/19] nfs: probe for LOCALIO when v3 " Mike Snitzer
2024-11-11 3:06 ` NeilBrown
2024-11-10 15:49 ` [for-6.13 PATCH 00/19] nfs/nfsd: fixes and improvements for LOCALIO Chuck Lever III
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241108234002.16392-17-snitzer@kernel.org \
--to=snitzer@kernel.org \
--cc=anna@kernel.org \
--cc=chuck.lever@oracle.com \
--cc=jlayton@kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=neilb@suse.de \
--cc=trondmy@hammerspace.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.