NLM: make sure that only one lockd is running at a time
Subsystems (2)
RFC PATCH 1/1 NLM: make sure that only one lockd is running at a time
I've been looking at a rather nasty oops in NLM code, and while it's hard to conclusively prove it, my current thinking is that there were 2 competing lockd daemons running at the same time -- one in the process of coming down and one up and working. The current code allows this, but the data that's touched by lockd doesn't seem to be adequately protected against this possibility.
The only protection for some of the data there appears to be the BKL, and there are functions that definitely sleep. Since it doesn't appear to be safe to have more than one lockd running at a time, we should probably prevent that from happening. It also seems like the current scheme of waiting for a bit and then forgetting about the existing lockd thread could be papering over bugs that cause lockd to take a long time coming down.
The following patch should prevent the kernel from spawning more than 1 lockd at a time, and causes the last caller of lockd_down to block until lockd actually comes down.
This patch isn't well tested, so this is really more an RFC and a starting point for discussion. The current code allows multiple lockd's to run, but that doesn't seem to be safe. Do we really need to allow that to occur, or should it be prevented? Is it acceptable to allow callers of lockd_down() to block this way?
Thoughts?
fs/lockd/svc.c +10 -18
@@ -151,7 +151,7 @@ lockd(struct svc_rqst *rqstp)* NFS mount or NFS daemon has gone away, and we've been sent a * signal, or else another process has taken over our job. */ -while ((nlmsvc_users || !signalled ()) && nlmsvc_pid == current->pid) { +while (nlmsvc_users) {long timeout =MAX_SCHEDULE_TIMEOUT ;char buf[RPC_MAX_ADDRBUFLEN ];
@@ -200,15 +200,9 @@ lockd(struct svc_rqst *rqstp)* Check whether there's a new lockd process before * shutting down the hosts and clearing the slot. */ -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" ); +if (nlmsvc_ops) +nlmsvc_invalidate_all (); +nlm_shutdown_hosts ();wake_up (&lockd_exit);/* Exit the RPC thread */
@@ -346,16 +340,14 @@ lockd_down(void)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 ... +* Wait indefinitely for the lockd process to exit. Allowing a new +* lockd to start before the old one has come down is dangerous and +* may be papering over lockd bugs. */ 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 ; - } +sleep_on (&lockd_exit); + nlmsvc_pid =0 ; + nlmsvc_serv =NULL ;spin_lock_irq (¤t->sighand->siglock);recalc_sigpending ();spin_unlock_irq (¤t->sighand->siglock);
-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems? Stop. Now Search log events and configuration files using AJAX and a browser. Download your FREE copy of Splunk now >> http://get.splunk.com/
_______________________________________________
NFS maillist - [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs