NFS: state manager thread must start running.

If the server restarts at an awkward time it is possible for write requests to block waiting for the state manager to run. If the state manager isn't already running a new thread will need to be started which requires a GFP_KERNEL allocation (for do_fork).

If memory is short, that GFP_KERNEL allocation could block on the writes going out via NFS, resulting in a deadlock.

The easiest solution is to keep the manager thread running always.

In this patch the manager thread no longer holds a reference on the client. Instead nfs4_shutdown_client() signals the thread to shutdown and waits for it to do so using a completion.

--
I suggested in an earlier email that it might be possible to use a work-queue for this. I concluded that it wasn't a good idea. If a server was down, the manager cold block indefinitely. If memory was tight and only one workqueue thread was active, this would block other NFS mounts.

It is unfortunate that the task comm[] array only allow 16 chars. A non-trivial IPv6 address doesn't fit in that at ALL. I wonder if there is a better way to manage that.

NeilBrown

index ba2affa51941..0b7bc4da4035 100644
@@ -432,6 +432,8 @@ extern void nfs4_schedule_lease_recovery(struct nfs_client *);
 extern int nfs4_wait_clnt_recover(struct nfs_client *clp);
 extern int nfs4_client_recover_expired_lease(struct nfs_client *clp);
 extern void nfs4_schedule_state_manager(struct nfs_client *);
+extern int nfs4_start_state_manager(struct nfs_client *);
+extern void nfs4_shutdown_state_manager(struct nfs_client *);
 extern void nfs4_schedule_path_down_recovery(struct nfs_client *clp);
 extern int nfs4_schedule_stateid_recovery(const struct nfs_server *, struct nfs4_state *);
 extern int nfs4_schedule_migration_recovery(const struct nfs_server *);

index aa9ef4876046..24c80a52e633 100644
@@ -219,6 +219,8 @@ static void nfs4_shutdown_client(struct nfs_client *clp)
 {
 	if (__test_and_clear_bit(NFS_CS_RENEWD, &clp->cl_res_state))
 		nfs4_kill_renewd(clp);
+	if (__test_and_clear_bit(NFS_CS_MANAGER, &clp->cl_res_state))
+		nfs4_shutdown_state_manager(clp);
 	clp->cl_mvops->shutdown_client(clp);
 	nfs4_destroy_callback(clp);
 	if (__test_and_clear_bit(NFS_CS_IDMAP, &clp->cl_res_state))

@@ -401,6 +403,11 @@ struct nfs_client *nfs4_init_client(struct nfs_client *clp,
 	}
 	__set_bit(NFS_CS_IDMAP, &clp->cl_res_state);
 
+	error = nfs4_start_state_manager(clp);
+	if (error < 0)
+		goto error;
+	__set_bit(NFS_CS_MANAGER, &clp->cl_res_state);
+
 	error = nfs4_init_client_minor_version(clp);
 	if (error < 0)
 		goto error;

index 1150ea41b626..ee7483022c46 100644
@@ -36,6 +36,7 @@ struct nfs_client {
 #define NFS_CS_RENEWD		3		/* - renewd started */
 #define NFS_CS_STOP_RENEW	4		/* no more state to renew */
 #define NFS_CS_CHECK_LEASE_TIME	5		/* need to check lease time */
+#define NFS_CS_MANAGER		6		/* NFSv4 manager thread running */
 	unsigned long		cl_flags;	/* behavior switches */
 #define NFS_CS_NORESVPORT	0		/* - use ephemeral src port */
 #define NFS_CS_DISCRTRY		1		/* - disconnect on RPC retry */

@@ -69,6 +70,7 @@ struct nfs_client {
 	struct delayed_work	cl_renewd;
 
 	struct rpc_wait_queue	cl_rpcwaitq;
+	struct completion	*cl_manager_done;
 
 	/* idmapper */
 	struct idmap *		cl_idmap;

So allocate one workqueue per mount?

Could do that (or per-client) but it doesn't really buy us anything does it? The state manager assumes it is single threads, so it would need to be a single-threaded workqueue with always at least one thread running. That is much the same as a kthread.

And then there is that fact that the current code explicitly enabled SIGKILL and maybe that is important.

NeilBrown

Hello, Neil.

It does buy some.

1. The kworker threads are more likely to be cache-hot than explicit kthreads.

2. Workqueue is a lot eaiser to get right in terms of synchronization and freezing.

3. Workqueue mandates well-defined boundaries between separate execution instances which often makes it a lot easier to implement and update kernel-wide features such as like freezer and runtime kernel patching.

If SIGKILL handling is mandatory (really?), kthread_worker can be used for #2 and #3.

Thanks.

tejun

(kthread_worker doesn't seem to be very well documented, but I think I see what it does).

The only reason I can think for that SIGKILL might be important is that when a server is not responding, a process that it trying to talk to it will only give up if it gets a fatal signal. So if state recovery starts for a server that cannot be contacted, the thread doing the recovery will block until the server comes back or until it received SIGKILL.

I cannot see anything that would generate such a SIGKILL except the broadcast SIGKILL at shutdown.
So maybe the purpose of

	allow_signal(SIGKILL);

is to ensure that when the machine is shutdown, the -manager thread actually dies.
But I'm not confident of this explanation. If this were the issue I would expect nfs_umount_begin to be sending SIGKILL too. But it just does rpc_killall_tasks- maybe that is enough. If so, is SIGKILL really needed.

Trond: can you provide some wisdom? Is SIGKILL important for the manager threads?
If so, would you prefer a thread that uses kthread_worker, or one that works more like the current code?
If not, would you be happy with a fully workqueue based solution?

Thanks,
NeilBrown