[PATCH 0/3] NFS/localio: make the LOCALIO probe safe, repeatable and bounded

[PATCH 0/3] NFS/localio: make the LOCALIO probe safe, repeatable and bounded

[Mike Snitzer]

Hi,

Three fixes to the LOCALIO probe.

Patch 1 takes a reference on the client when nfs_local_probe_async()
queues cl_local_probe_work, and drops it once the work has run.  The work
was queued without one, so a client freed with a probe queued (e.g. at
umount after an NFSv3 I/O queued a throttled probe) left the work to run
on freed memory.

Patch 2 probes again when localio_enabled is set back to Y.  A client
that probed while localio_enabled was N was disabled by the probe, and
nothing probed it again, so it stayed on the network.  Setting
localio_enabled to Y now queues a probe for every ready client, in every
network namespace, that does not use LOCALIO.

Patch 3 gives the probe a short soft timeout of its own.  The probe's
NULL ping and UUID_IS_LOCAL used the mount's timeout, so with timeo=600 a
server that does not answer the LOCALIO program held each probe for about
170 s.  The new nfs module parameter localio_timeout (seconds, 10 by
default, at most 600; 0 keeps the mount's timeouts) bounds it: through a
proxy dropping every LOCALIO call the probe gave up after 10.6 s, and
logged nothing.

The series is based on Anna's linux-next (nfs-for-7.4-1).

Thanks,
Mike

Mike Snitzer (3):
  NFS/localio: hold the client across an asynchronous LOCALIO probe
  NFS/localio: probe again for LOCALIO when localio_enabled is set to Y
  NFS/localio: give the LOCALIO probe a short timeout of its own

 Documentation/filesystems/nfs/localio.rst |  12 +++
 fs/nfs/localio.c                          | 118 ++++++++++++++++++++--
 2 files changed, 123 insertions(+), 7 deletions(-)


base-commit: ef30b3efb7e8bae94d790a6503a595e1051323e7
-- 
2.52.0

[PATCH 1/3] NFS/localio: hold the client across an asynchronous LOCALIO probe

[Mike Snitzer]

nfs_local_probe_async() queues the client's own cl_local_probe_work
without a reference on the client, and nfs_free_client() does not cancel
it.  The work takes a reference only once it runs.  If the last
nfs_put_client() comes first, for example at umount after an NFSv3 I/O
queued a throttled probe, the client is freed with its work queued, and
the work then runs on freed memory.

Take the reference when the work is queued, and drop it when the work
has run.  If the work is already queued, it already holds a reference,
and the one just taken is dropped.

Fixes: 76d4cb6345da ("nfs: probe for LOCALIO when v4 client reconnects to server")
Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 fs/nfs/localio.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index cdada6cbb13bf..d5859e8c2a2a8 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -203,15 +203,22 @@ void nfs_local_probe_async_work(struct work_struct *work)
 	struct nfs_client *clp =
 		container_of(work, struct nfs_client, cl_local_probe_work);
 
-	if (!refcount_inc_not_zero(&clp->cl_count))
-		return;
 	nfs_local_probe(clp);
+	/* The reference nfs_local_probe_async() took for this work */
 	nfs_put_client(clp);
 }
 
+/*
+ * The work is part of the client, so it holds a reference on the client
+ * from when it is queued until it has run: the last nfs_put_client()
+ * cannot free the client under a queued probe.
+ */
 void nfs_local_probe_async(struct nfs_client *clp)
 {
-	queue_work(nfsiod_workqueue, &clp->cl_local_probe_work);
+	if (!refcount_inc_not_zero(&clp->cl_count))
+		return;
+	if (!queue_work(nfsiod_workqueue, &clp->cl_local_probe_work))
+		nfs_put_client(clp);	/* already queued, with its reference */
 }
 EXPORT_SYMBOL_GPL(nfs_local_probe_async);
 
-- 
2.52.0

[PATCH 2/3] NFS/localio: probe again for LOCALIO when localio_enabled is set to Y

[Mike Snitzer]

localio_enabled is checked on every I/O.  Setting it to N sends the next
I/O of every client over the network, and setting it back to Y resumes
LOCALIO for a client that uses it.  But a client that probed while
localio_enabled was N was disabled by the probe, and nothing probes it
again: NFSv3 probes again only when nfs3_localio_probe_throttle is set,
and NFSv4 only after state recovery.  Such a client stays on the network
after localio_enabled is set to Y.

Give localio_enabled a set handler that, on a change from N to Y, queues
a probe for every ready client, in every network namespace, that does not
use LOCALIO.  On the module command line there are no clients yet, and
nothing is probed.

Tested with NFSv3 and NFSv4.2 on a loopback mount: a client mounted with
localio_enabled=N sent its writes over the network, and after
localio_enabled was set to Y it sent none.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 Documentation/filesystems/nfs/localio.rst |  3 ++
 fs/nfs/localio.c                          | 51 ++++++++++++++++++++++-
 2 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/Documentation/filesystems/nfs/localio.rst b/Documentation/filesystems/nfs/localio.rst
index 79808b37d7457..e8f44332cc806 100644
--- a/Documentation/filesystems/nfs/localio.rst
+++ b/Documentation/filesystems/nfs/localio.rst
@@ -312,6 +312,9 @@ Module Parameters
 /sys/module/nfs/parameters/localio_enabled (bool)
 controls if LOCALIO is enabled, defaults to Y. If client and server are
 local but 'localio_enabled' is set to N then LOCALIO will not be used.
+It is checked on every I/O: setting it to N sends the next I/O of every
+client over the network, and setting it back to Y resumes LOCALIO.  A
+client that probed while it was N is probed again when it is set to Y.
 
 /sys/module/nfs/parameters/localio_O_DIRECT_semantics (bool)
 controls if O_DIRECT extends down to the underlying filesystem, defaults
diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index d5859e8c2a2a8..80a496d6fe0d6 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -25,6 +25,7 @@
 #include <linux/nfs_xdr.h>
 
 #include "internal.h"
+#include "netns.h"
 #include "pnfs.h"
 #include "nfstrace.h"
 
@@ -55,7 +56,31 @@ struct nfs_local_fsync_ctx {
 };
 
 static bool localio_enabled __read_mostly = true;
-module_param(localio_enabled, bool, 0644);
+
+static void nfs_local_probe_all(void);
+
+/*
+ * localio_enabled is checked on every I/O, so turning it off stops LOCALIO
+ * at once and turning it back on resumes it for clients that use it.  A
+ * client that probed while it was off was disabled, and nothing probes it
+ * again: turning it on probes every client that does not use LOCALIO.
+ */
+static int localio_enabled_set(const char *val, const struct kernel_param *kp)
+{
+	bool was = READ_ONCE(localio_enabled);
+	int ret;
+
+	ret = param_set_bool(val, kp);
+	if (!ret && !was && READ_ONCE(localio_enabled))
+		nfs_local_probe_all();
+	return ret;
+}
+
+static const struct kernel_param_ops localio_enabled_ops = {
+	.set = localio_enabled_set,
+	.get = param_get_bool,
+};
+module_param_cb(localio_enabled, &localio_enabled_ops, &localio_enabled, 0644);
 
 static void nfs_local_do_read(struct nfs_local_kiocb *iocb,
 			      const struct rpc_call_ops *call_ops);
@@ -222,6 +247,30 @@ void nfs_local_probe_async(struct nfs_client *clp)
 }
 EXPORT_SYMBOL_GPL(nfs_local_probe_async);
 
+/* Probe every client, in every namespace, that does not use LOCALIO. */
+static void nfs_local_probe_all(void)
+{
+	struct nfs_client *clp;
+	struct net *net;
+
+	/* Set on the module command line: there are no clients yet. */
+	if (!nfs_net_id)
+		return;
+
+	down_read(&net_rwsem);
+	for_each_net(net) {
+		struct nfs_net *nn = net_generic(net, nfs_net_id);
+
+		spin_lock(&nn->nfs_client_lock);
+		list_for_each_entry(clp, &nn->nfs_client_list, cl_share_link)
+			if (clp->cl_cons_state == NFS_CS_READY &&
+			    !nfs_client_is_local(clp))
+				nfs_local_probe_async(clp);
+		spin_unlock(&nn->nfs_client_lock);
+	}
+	up_read(&net_rwsem);
+}
+
 static inline void nfs_local_file_put(struct nfsd_file *localio)
 {
 	/* nfs_to_nfsd_file_put_local() expects an __rcu pointer
-- 
2.52.0

[PATCH 3/3] NFS/localio: give the LOCALIO probe a short timeout of its own

[Mike Snitzer]

The LOCALIO probe clones the NFS client with rpc_bind_new_program(),
which NULL-pings the new program before the probe sends UUID_IS_LOCAL.
Both calls use the mount's timeout.  With timeo=600 and the default
retrans, a server that does not answer the LOCALIO program holds each
probe for about 170 s.  The client then logs "nfslocalio: server ... not
responding, timed out".  On such a server, one mkdir took about 170 s
of a 202 s run and ended when the probe gave up.

Give the probe a soft timeout of its own, with no retries and no log
message.  The new nfs module parameter localio_timeout sets it in
seconds, 10 by default and at most 600.  Set it on a private clone of
the NFS client before rpc_bind_new_program(), so that the ping uses it
as well.  Clear cl_noretranstimeo, which NFSv4 mounts set, because with
it a sent call does not time out while the connection stays up.  With
localio_timeout set to 0, the probe uses the NFS client's timeouts, as
before.  The probe still shares the NFS client's transport and runs
beside its RPCs.  The NFS client's own timeouts do not change.

Tested through a proxy that drops every call to the LOCALIO program:
the probe gave up after 10.6 s and logged nothing.  With
localio_timeout=0 it took 182.9 s and logged the message above.  The
probe still succeeds against a server that supports LOCALIO: with no
proxy, a 64 MiB O_DIRECT write through LOCALIO sent no WRITE RPCs.

Signed-off-by: Mike Snitzer <snitzer@kernel.org>
---
 Documentation/filesystems/nfs/localio.rst |  9 ++++
 fs/nfs/localio.c                          | 54 +++++++++++++++++++++--
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/Documentation/filesystems/nfs/localio.rst b/Documentation/filesystems/nfs/localio.rst
index e8f44332cc806..45c6981da0d89 100644
--- a/Documentation/filesystems/nfs/localio.rst
+++ b/Documentation/filesystems/nfs/localio.rst
@@ -316,6 +316,15 @@ It is checked on every I/O: setting it to N sends the next I/O of every
 client over the network, and setting it back to Y resumes LOCALIO.  A
 client that probed while it was N is probed again when it is set to Y.
 
+/sys/module/nfs/parameters/localio_timeout (uint)
+sets how many seconds the LOCALIO probe waits for each reply from the
+server, defaults to 10, at most 600.  A server that does not support
+LOCALIO may not answer the probe at all.  If no reply comes in time,
+the probe gives up without a log message.  The client then does not use
+LOCALIO.  The mount's timeouts do not apply to the probe.  Set to 0, the
+probe uses the mount's timeouts instead, as an NFS RPC would.  A change
+applies to the next probe.
+
 /sys/module/nfs/parameters/localio_O_DIRECT_semantics (bool)
 controls if O_DIRECT extends down to the underlying filesystem, defaults
 to N. Application IO must be logical blocksize aligned, otherwise
diff --git a/fs/nfs/localio.c b/fs/nfs/localio.c
index 80a496d6fe0d6..06d0755b1f203 100644
--- a/fs/nfs/localio.c
+++ b/fs/nfs/localio.c
@@ -82,6 +82,22 @@ static const struct kernel_param_ops localio_enabled_ops = {
 };
 module_param_cb(localio_enabled, &localio_enabled_ops, &localio_enabled, 0644);
 
+/* Seconds the LOCALIO probe waits for each reply; 0: the NFS client's own */
+static unsigned int localio_timeout __read_mostly = 10;
+
+static int localio_timeout_set(const char *val, const struct kernel_param *kp)
+{
+	return param_set_uint_minmax(val, kp, 0, 600);
+}
+
+static const struct kernel_param_ops localio_timeout_ops = {
+	.set = localio_timeout_set,
+	.get = param_get_uint,
+};
+module_param_cb(localio_timeout, &localio_timeout_ops, &localio_timeout, 0644);
+MODULE_PARM_DESC(localio_timeout,
+		 "LOCALIO probe timeout in seconds (default 10), 0 to use the mount's default");
+
 static void nfs_local_do_read(struct nfs_local_kiocb *iocb,
 			      const struct rpc_call_ops *call_ops);
 static void nfs_local_do_write(struct nfs_local_kiocb *iocb,
@@ -156,13 +172,45 @@ const struct rpc_program nfslocalio_program = {
 
 /*
  * nfs_init_localioclient - Initialise an NFS localio client connection
+ *
+ * A server that does not support LOCALIO may not answer its RPCs at all.
+ * The probe shares the NFS client's transport and runs beside its RPCs.
+ * With localio_timeout set, it gets a quiet, soft timeout of that many
+ * seconds of its own, whatever timeouts the mount uses.  With 0, it uses
+ * the NFS client's timeouts, as an NFS RPC would.
  */
 static struct rpc_clnt *nfs_init_localioclient(struct nfs_client *clp)
 {
-	struct rpc_clnt *rpcclient_localio;
+	unsigned int timeout_secs = READ_ONCE(localio_timeout);
+	struct rpc_clnt *parent = clp->cl_rpcclient;
+	struct rpc_clnt *probe = NULL, *rpcclient_localio;
 
-	rpcclient_localio = rpc_bind_new_program(clp->cl_rpcclient,
-						 &nfslocalio_program, 1);
+	/*
+	 * rpc_bind_new_program() pings the new program with the timeout of
+	 * the client it clones, so set the probe's timeout on a private
+	 * clone first.  The clone's cl_timeout points at the transport's
+	 * timeout: give it its own copy rather than write through it.
+	 */
+	if (timeout_secs) {
+		probe = rpc_clone_client(parent);
+		if (IS_ERR(probe))
+			return probe;
+		probe->cl_timeout_default = (struct rpc_timeout) {
+			.to_initval	= timeout_secs * HZ,
+			.to_maxval	= timeout_secs * HZ,
+			.to_retries	= 0,
+		};
+		probe->cl_timeout = &probe->cl_timeout_default;
+		probe->cl_softrtry = 1;
+		probe->cl_noretranstimeo = 0;
+		probe->cl_chatty = 0;
+		parent = probe;
+	}
+
+	rpcclient_localio = rpc_bind_new_program(parent, &nfslocalio_program, 1);
+	/* The new client holds a reference to its parent, the probe clone */
+	if (probe)
+		rpc_shutdown_client(probe);
 
 	dprintk_rcu("%s: server (%s) %s NFS LOCALIO.\n",
 		__func__, rpc_peeraddr2str(clp->cl_rpcclient, RPC_DISPLAY_ADDR),
-- 
2.52.0