Attachment 278361 Details for Bug 254195 – patch -- Add lockd reference counting and clean up lockd startup/shutdown

[patch] patch -- Add lockd reference counting and clean up lockd startup/shutdown

0001-NLM-Add-lockd-reference-counting-and-clean-up-lockd.patch (text/plain), 7.88 KB, created by Jeff Layton on 2007-12-05 14:34:01 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-12-05 14:34:01 UTC

Size: 7.88 KB

patch

obsolete

>From 36219c493d491d60d8a33eb1d6fa5520a26cc34d Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Wed, 5 Dec 2007 09:21:36 -0500
>Subject: [PATCH] NLM: Add lockd reference counting and clean up lockd startup/shutdown
>
>When a lock that a client is blocking on comes free, lockd does this in
>nlmsvc_grant_blocked():
>
>    nlm_async_call(block->b_call, NLMPROC_GRANTED_MSG, &nlmsvc_grant_ops);
>
>the callback from this call is nlmsvc_grant_callback(). That function
>does this at the end to wake up lockd:
>
>    svc_wake_up(block->b_daemon);
>
>However there is no guarantee that lockd will be up when this happens.
>If someone shuts down or restarts lockd before the async call completes,
>then the b_daemon pointer will point to freed memory.
>
>This patch is based on Trond's suggestion to add a new reference counter
>to lockd. lockd_down/up increment this counter when the nlmsvc_users
>count transitions to/from 0. The code also takes a reference when the
>nlm_blocked list gets its first entry, and drops the reference when it
>goes empty. Finally, it takes a reference when doing a nlm_granted
>callback and drops the reference when after waking up the daemon in the
>RPC callback.
>
>This patch changes the semantics for lockd_up/down as well. It only
>shuts down lockd if the nlmsvc_ref goes to 0. When checking for this,
>lockd takes the nlmsvc_mutex and holds on to it until lockd is down.
>This prevents multiple lockd's from running at any given time since
>lockd_up won't be able to complete until lockd is all the way down.
>Also, with this patch, there's no real reason to have lockd_down() wait
>for lockd to come down, so this patch has it return after sending the
>signal to lockd.
>---
> fs/lockd/svc.c              |   73 ++++++++++++++++++++++---------------------
> fs/lockd/svclock.c          |    9 +++++-
> include/linux/lockd/lockd.h |    1 +
> 3 files changed, 46 insertions(+), 37 deletions(-)
>
>diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>index 82e2192..8859d4a 100644
>--- a/fs/lockd/svc.c
>+++ b/fs/lockd/svc.c
>@@ -49,12 +49,12 @@ EXPORT_SYMBOL(nlmsvc_ops);
> static DEFINE_MUTEX(nlmsvc_mutex);
> static unsigned int		nlmsvc_users;
> static pid_t			nlmsvc_pid;
>+atomic_t			nlmsvc_ref = ATOMIC_INIT(0);
> static struct svc_serv		*nlmsvc_serv;
> int				nlmsvc_grace_period;
> unsigned long			nlmsvc_timeout;
> 
> static DECLARE_COMPLETION(lockd_start_done);
>-static DECLARE_WAIT_QUEUE_HEAD(lockd_exit);
> 
> /*
>  * These can be set at insmod time (useful for NFS as root filesystem),
>@@ -148,13 +148,17 @@ lockd(struct svc_rqst *rqstp)
> 
> 	/*
> 	 * The main request loop. We don't terminate until the last
>-	 * NFS mount or NFS daemon has gone away, and we've been sent a
>-	 * signal, or else another process has taken over our job.
>+	 * NFS mount or NFS daemon has gone away, and the nlm_blocked
>+	 * list is empty. The nlmsvc_mutex ensures that we prevent 
>+	 * a new lockd from being started before the old lockd is
>+	 * down when lockd is restarted.
> 	 */
>-	while ((nlmsvc_users || !signalled()) && nlmsvc_pid == current->pid) {
>+	mutex_lock(&nlmsvc_mutex);
>+	while (atomic_read(&nlmsvc_ref) != 0) {
> 		long timeout = MAX_SCHEDULE_TIMEOUT;
> 		char buf[RPC_MAX_ADDRBUFLEN];
> 
>+		mutex_unlock(&nlmsvc_mutex);
> 		if (signalled()) {
> 			flush_signals(current);
> 			if (nlmsvc_ops) {
>@@ -180,11 +184,11 @@ lockd(struct svc_rqst *rqstp)
> 		 */
> 		err = svc_recv(rqstp, timeout);
> 		if (err == -EAGAIN || err == -EINTR)
>-			continue;
>+			goto again;
> 		if (err < 0) {
> 			printk(KERN_WARNING
>-			       "lockd: terminating on error %d\n",
>-			       -err);
>+			       "lockd: terminating on error %d\n", -err);
>+			mutex_lock(&nlmsvc_mutex);
> 			break;
> 		}
> 
>@@ -192,24 +196,23 @@ lockd(struct svc_rqst *rqstp)
> 				svc_print_addr(rqstp, buf, sizeof(buf)));
> 
> 		svc_process(rqstp);
>+again:
>+		mutex_lock(&nlmsvc_mutex);
> 	}
> 
>-	flush_signals(current);
>-
> 	/*
>-	 * Check whether there's a new lockd process before
>-	 * shutting down the hosts and clearing the slot.
>+	 * at this point lockd is committed to going down. We hold the
>+	 * nlmsvc_mutex until just before exit to prevent a new one
>+	 * from starting before it's down.
> 	 */
>-	if (!nlmsvc_pid || current->pid == nlmsvc_pid) {
>-		if (nlmsvc_ops)
>-			nlmsvc_invalidate_all();
>-		nlm_shutdown_hosts();
>-		nlmsvc_pid = 0;
>-		nlmsvc_serv = NULL;
>-	} else
>-		printk(KERN_DEBUG
>-			"lockd: new process, skipping host shutdown\n");
>-	wake_up(&lockd_exit);
>+	flush_signals(current);
>+
>+	if (nlmsvc_ops)
>+		nlmsvc_invalidate_all();
>+	nlm_shutdown_hosts();
>+	nlmsvc_pid = 0;
>+	nlmsvc_serv = NULL;
>+	mutex_unlock(&nlmsvc_mutex);
> 
> 	/* Exit the RPC thread */
> 	svc_exit_thread(rqstp);
>@@ -270,6 +273,10 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
> 	int			error = 0;
> 
> 	mutex_lock(&nlmsvc_mutex);
>+
>+	if (!nlmsvc_users)
>+		atomic_inc(&nlmsvc_ref);
>+
> 	/*
> 	 * Check whether we're already up and running.
> 	 */
>@@ -283,9 +290,10 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
> 	 * Sanity check: if there's no pid,
> 	 * we should be the first user ...
> 	 */
>-	if (nlmsvc_users)
>+	if (nlmsvc_users) {
> 		printk(KERN_WARNING
> 			"lockd_up: no pid, %d users??\n", nlmsvc_users);
>+	}
> 
> 	error = -ENOMEM;
> 	serv = svc_create(&nlmsvc_program, LOCKD_BUFSIZE, NULL);
>@@ -315,6 +323,12 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
> destroy_and_out:
> 	svc_destroy(serv);
> out:
>+	/*
>+	 * don't leave refcount high if there were already users, or if there
>+	 * was an error in starting up lockd
>+	 */
>+	if (!nlmsvc_users && error)
>+		atomic_dec(&nlmsvc_ref);
> 	if (!error)
> 		nlmsvc_users++;
> 	mutex_unlock(&nlmsvc_mutex);
>@@ -344,21 +358,8 @@ lockd_down(void)
> 	}
> 	warned = 0;
> 
>+	atomic_dec(&nlmsvc_ref);
> 	kill_proc(nlmsvc_pid, SIGKILL, 1);
>-	/*
>-	 * Wait for the lockd process to exit, but since we're holding
>-	 * the lockd semaphore, we can't wait around forever ...
>-	 */
>-	clear_thread_flag(TIF_SIGPENDING);
>-	interruptible_sleep_on_timeout(&lockd_exit, HZ);
>-	if (nlmsvc_pid) {
>-		printk(KERN_WARNING 
>-			"lockd_down: lockd failed to exit, clearing pid\n");
>-		nlmsvc_pid = 0;
>-	}
>-	spin_lock_irq(&current->sighand->siglock);
>-	recalc_sigpending();
>-	spin_unlock_irq(&current->sighand->siglock);
> out:
> 	mutex_unlock(&nlmsvc_mutex);
> }
>diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
>index d120ec3..4010dae 100644
>--- a/fs/lockd/svclock.c
>+++ b/fs/lockd/svclock.c
>@@ -61,6 +61,9 @@ nlmsvc_insert_block(struct nlm_block *block, unsigned long when)
> 	struct list_head *pos;
> 
> 	dprintk("lockd: nlmsvc_insert_block(%p, %ld)\n", block, when);
>+	if (list_empty(&nlm_blocked))
>+		atomic_inc(&nlmsvc_ref);
>+
> 	if (list_empty(&block->b_list)) {
> 		kref_get(&block->b_count);
> 	} else {
>@@ -239,6 +242,8 @@ static int nlmsvc_unlink_block(struct nlm_block *block)
> 	/* Remove block from list */
> 	status = posix_unblock_lock(block->b_file->f_file, &block->b_call->a_args.lock.fl);
> 	nlmsvc_remove_block(block);
>+	if (list_empty(&nlm_blocked))
>+		atomic_dec(&nlmsvc_ref);
> 	return status;
> }
> 
>@@ -762,7 +767,8 @@ callback:
> 	/* Schedule next grant callback in 30 seconds */
> 	nlmsvc_insert_block(block, 30 * HZ);
> 
>-	/* Call the client */
>+	/* Call the client, but bump lockd refcount first */
>+	atomic_inc(&nlmsvc_ref);
> 	nlm_async_call(block->b_call, NLMPROC_GRANTED_MSG, &nlmsvc_grant_ops);
> }
> 
>@@ -794,6 +800,7 @@ static void nlmsvc_grant_callback(struct rpc_task *task, void *data)
> 	}
> 	nlmsvc_insert_block(block, timeout);
> 	svc_wake_up(block->b_daemon);
>+	atomic_dec(&nlmsvc_ref);
> }
> 
> static void nlmsvc_grant_release(void *data)
>diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>index e2d1ce3..a165193 100644
>--- a/include/linux/lockd/lockd.h
>+++ b/include/linux/lockd/lockd.h
>@@ -151,6 +151,7 @@ extern struct svc_procedure	nlmsvc_procedures[];
> #ifdef CONFIG_LOCKD_V4
> extern struct svc_procedure	nlmsvc_procedures4[];
> #endif
>+extern atomic_t			nlmsvc_ref;
> extern int			nlmsvc_grace_period;
> extern unsigned long		nlmsvc_timeout;
> extern int			nsm_use_hostnames;
>-- 
>1.5.3.3
>

Actions: View | Diff

Attachments on bug 254195: 277081 | 278361 | 280141 | 280151 | 280161 | 306390 | 306391 | 306392