Attachment 277081 Details for Bug 254195 – patch -- add reference counting to lockd

[patch] patch -- add reference counting to lockd

0001-Add-lockd-reference-counting-for-NLM-async-callbacks.patch (text/plain), 7.32 KB, created by Jeff Layton on 2007-12-04 15:24:59 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jeff Layton

Created: 2007-12-04 15:24:59 UTC

Size: 7.32 KB

patch

obsolete

>From 5281842028d2539359b133c049db4298ebb3bd84 Mon Sep 17 00:00:00 2001
>From: Jeff Layton <jlayton@redhat.com>
>Date: Tue, 4 Dec 2007 09:38:45 -0500
>Subject: [PATCH] Add lockd reference counting for NLM async callbacks
>
>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. In this patch, the reference counter is protected by the
>nlmsvc_mutex. 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.
>
>This patch doesn't seem to cure the issue, however. It's possible for
>the nlmsvc_ref to dip to 0 prematurely somehow (and I've not tracked it
>down all of the way yet). This patch also causes a lockdep warning
>since we take nlmsvc_mutex and nlm_host_mutex in different orders.
>---
> fs/lockd/svc.c              |   76 ++++++++++++++++++++++---------------------
> fs/lockd/svclock.c          |   11 ++++++
> include/linux/lockd/lockd.h |    2 +
> 3 files changed, 52 insertions(+), 37 deletions(-)
>
>diff --git a/fs/lockd/svc.c b/fs/lockd/svc.c
>index 82e2192..833c021 100644
>--- a/fs/lockd/svc.c
>+++ b/fs/lockd/svc.c
>@@ -46,15 +46,15 @@ static struct svc_program	nlmsvc_program;
> struct nlmsvc_binding *		nlmsvc_ops;
> EXPORT_SYMBOL(nlmsvc_ops);
> 
>-static DEFINE_MUTEX(nlmsvc_mutex);
>+DEFINE_MUTEX(nlmsvc_mutex);
> static unsigned int		nlmsvc_users;
> static pid_t			nlmsvc_pid;
>+int				nlmsvc_ref;
> 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 protects the nlmsvc_ref var
>+	 * and makes sure that if lockd is going down that a new one
>+	 * won't be started until the old one is down.
> 	 */
>-	while ((nlmsvc_users || !signalled()) && nlmsvc_pid == current->pid) {
>+	mutex_lock(&nlmsvc_mutex);
>+	while (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,11 @@ lockd_up(int proto) /* Maybe add a 'family' option when IPv6 is supported ?? */
> 	int			error = 0;
> 
> 	mutex_lock(&nlmsvc_mutex);
>+
>+	/* artificially bump reference counter to make sure lockd doesn't
>+	 * go down immediately */
>+	nlmsvc_ref++;
>+
> 	/*
> 	 * Check whether we're already up and running.
> 	 */
>@@ -283,9 +291,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 +324,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)
>+		nlmsvc_ref--;
> 	if (!error)
> 		nlmsvc_users++;
> 	mutex_unlock(&nlmsvc_mutex);
>@@ -344,21 +359,8 @@ lockd_down(void)
> 	}
> 	warned = 0;
> 
>+	--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..3083554 100644
>--- a/fs/lockd/svclock.c
>+++ b/fs/lockd/svclock.c
>@@ -61,6 +61,12 @@ 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)) {
>+		mutex_lock(&nlmsvc_mutex);
>+		++nlmsvc_ref;
>+		mutex_unlock(&nlmsvc_mutex);
>+	}
>+
> 	if (list_empty(&block->b_list)) {
> 		kref_get(&block->b_count);
> 	} else {
>@@ -239,6 +245,11 @@ 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)) {
>+		mutex_lock(&nlmsvc_mutex);
>+		--nlmsvc_ref;
>+		mutex_unlock(&nlmsvc_mutex);
>+	}
> 	return status;
> }
> 
>diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
>index e2d1ce3..6b67187 100644
>--- a/include/linux/lockd/lockd.h
>+++ b/include/linux/lockd/lockd.h
>@@ -151,6 +151,8 @@ extern struct svc_procedure	nlmsvc_procedures[];
> #ifdef CONFIG_LOCKD_V4
> extern struct svc_procedure	nlmsvc_procedures4[];
> #endif
>+extern struct mutex		nlmsvc_mutex;
>+extern int			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