[PATCH] lockd: replace semaphore, sleep_on

[PATCH] lockd: replace semaphore, sleep_on

[Thomas Gleixner]

Use wait_event, completion instead of the obsolete sleep_on functions
and the abused semaphore

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Ingo Molnar <mingo@elte.hu>
---

 2.6.9-bk-041020-thomas/fs/lockd/clntlock.c |   12 +++++++-----
 2.6.9-bk-041020-thomas/fs/lockd/svc.c      |   23
+++++++++++++----------
 2 files changed, 20 insertions(+), 15 deletions(-)

diff -puN fs/lockd/svc.c~lockd fs/lockd/svc.c
--- 2.6.9-bk-041020/fs/lockd/svc.c~lockd	2004-10-20 15:56:52.000000000
+0200
+++ 2.6.9-bk-041020-thomas/fs/lockd/svc.c	2004-10-20 15:56:52.000000000
+0200
@@ -46,7 +46,7 @@ static pid_t			nlmsvc_pid;
 int				nlmsvc_grace_period;
 unsigned long			nlmsvc_timeout;
 
-static DECLARE_MUTEX_LOCKED(lockd_start);
+static DECLARE_WAIT_QUEUE_HEAD(lockd_start);
 static DECLARE_WAIT_QUEUE_HEAD(lockd_exit);
 
 /*
@@ -109,7 +109,7 @@ lockd(struct svc_rqst *rqstp)
 	 * Let our maker know we're running.
 	 */
 	nlmsvc_pid = current->pid;
-	up(&lockd_start);
+	wake_up(&lockd_start);
 
 	daemonize("lockd");
 
@@ -258,8 +258,15 @@ lockd_up(void)
 			"lockd_up: create thread failed, error=%d\n", error);
 		goto destroy_and_out;
 	}
-	down(&lockd_start);
-
+	/*
+	 * Wait for the lockd process to start, but since we're holding
+	 * the lockd semaphore, we can't wait around forever ...
+	 */
+	if (wait_event_interruptible_timeout(lockd_start,
+					nlmsvc_pid != 0, HZ) <= 0) {
+		printk(KERN_WARNING
+			"lockd_down: lockd failed to start\n");
+	}
 	/*
 	 * Note: svc_serv structures have an initial use count of 1,
 	 * so we exit through here on both success and failure.
@@ -298,16 +305,12 @@ lockd_down(void)
 	 * 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) {
+	if (wait_event_interruptible_timeout(lockd_exit,
+					nlmsvc_pid == 0, HZ) <= 0) {
 		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:
 	up(&nlmsvc_sema);
 }
diff -puN fs/lockd/clntlock.c~lockd fs/lockd/clntlock.c
--- 2.6.9-bk-041020/fs/lockd/clntlock.c~lockd	2004-10-20
15:56:52.000000000 +0200
+++ 2.6.9-bk-041020-thomas/fs/lockd/clntlock.c	2004-10-20
15:56:52.000000000 +0200
@@ -6,6 +6,7 @@
  * Copyright (C) 1996, Olaf Kirch <okir@monad.swb.de>
  */
 
+#include <linux/completion.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/time.h>
@@ -32,7 +33,7 @@ static int			reclaimer(void *ptr);
  */
 struct nlm_wait {
 	struct nlm_wait *	b_next;		/* linked list */
-	wait_queue_head_t	b_wait;		/* where to wait on */
+	struct completion	b_wait;		/* where to wait on */
 	struct nlm_host *	b_host;
 	struct file_lock *	b_lock;		/* local file lock */
 	unsigned short		b_reclaim;	/* got to reclaim lock */
@@ -53,7 +54,7 @@ nlmclnt_block(struct nlm_host *host, str
 
 	block.b_host   = host;
 	block.b_lock   = fl;
-	init_waitqueue_head(&block.b_wait);
+	init_completion(&block.b_wait);
 	block.b_status = NLM_LCK_BLOCKED;
 	block.b_next   = nlm_blocked;
 	nlm_blocked    = &block;
@@ -69,7 +70,8 @@ nlmclnt_block(struct nlm_host *host, str
 	 * a 1 minute timeout would do. See the comment before
 	 * nlmclnt_lock for an explanation.
 	 */
-	sleep_on_timeout(&block.b_wait, 30*HZ);
+
+	wait_for_completion_timeout(&block.b_wait, 30*HZ);
 
 	for (head = &nlm_blocked; *head; head = &(*head)->b_next) {
 		if (*head == &block) {
@@ -118,7 +120,7 @@ nlmclnt_grant(struct nlm_lock *lock)
 	 * wake up the caller.
 	 */
 	block->b_status = NLM_LCK_GRANTED;
-	wake_up(&block->b_wait);
+	complete(&block->b_wait);
 
 	return nlm_granted;
 }
@@ -233,7 +235,7 @@ restart:
 	for (block = nlm_blocked; block; block = block->b_next) {
 		if (block->b_host == host) {
 			block->b_status = NLM_LCK_DENIED_GRACE_PERIOD;
-			wake_up(&block->b_wait);
+			complete(&block->b_wait);
 		}
 	}
 
_

Re: [PATCH] lockd: replace semaphore, sleep_on

[Matthew Wilcox]

Not really my area, you want to ask Trond about this.

On Wed, Oct 20, 2004 at 09:15:43PM +0200, Thomas Gleixner wrote:
> 
> Use wait_event, completion instead of the obsolete sleep_on functions
> and the abused semaphore
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Acked-by: Ingo Molnar <mingo@elte.hu>
> ---
> 
>  2.6.9-bk-041020-thomas/fs/lockd/clntlock.c |   12 +++++++-----
>  2.6.9-bk-041020-thomas/fs/lockd/svc.c      |   23
> +++++++++++++----------
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff -puN fs/lockd/svc.c~lockd fs/lockd/svc.c
> --- 2.6.9-bk-041020/fs/lockd/svc.c~lockd	2004-10-20 15:56:52.000000000
> +0200
> +++ 2.6.9-bk-041020-thomas/fs/lockd/svc.c	2004-10-20 15:56:52.000000000
> +0200
> @@ -46,7 +46,7 @@ static pid_t			nlmsvc_pid;
>  int				nlmsvc_grace_period;
>  unsigned long			nlmsvc_timeout;
>  
> -static DECLARE_MUTEX_LOCKED(lockd_start);
> +static DECLARE_WAIT_QUEUE_HEAD(lockd_start);
>  static DECLARE_WAIT_QUEUE_HEAD(lockd_exit);
>  
>  /*
> @@ -109,7 +109,7 @@ lockd(struct svc_rqst *rqstp)
>  	 * Let our maker know we're running.
>  	 */
>  	nlmsvc_pid = current->pid;
> -	up(&lockd_start);
> +	wake_up(&lockd_start);
>  
>  	daemonize("lockd");
>  
> @@ -258,8 +258,15 @@ lockd_up(void)
>  			"lockd_up: create thread failed, error=%d\n", error);
>  		goto destroy_and_out;
>  	}
> -	down(&lockd_start);
> -
> +	/*
> +	 * Wait for the lockd process to start, but since we're holding
> +	 * the lockd semaphore, we can't wait around forever ...
> +	 */
> +	if (wait_event_interruptible_timeout(lockd_start,
> +					nlmsvc_pid != 0, HZ) <= 0) {
> +		printk(KERN_WARNING
> +			"lockd_down: lockd failed to start\n");
> +	}
>  	/*
>  	 * Note: svc_serv structures have an initial use count of 1,
>  	 * so we exit through here on both success and failure.
> @@ -298,16 +305,12 @@ lockd_down(void)
>  	 * 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) {
> +	if (wait_event_interruptible_timeout(lockd_exit,
> +					nlmsvc_pid == 0, HZ) <= 0) {
>  		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:
>  	up(&nlmsvc_sema);
>  }
> diff -puN fs/lockd/clntlock.c~lockd fs/lockd/clntlock.c
> --- 2.6.9-bk-041020/fs/lockd/clntlock.c~lockd	2004-10-20
> 15:56:52.000000000 +0200
> +++ 2.6.9-bk-041020-thomas/fs/lockd/clntlock.c	2004-10-20
> 15:56:52.000000000 +0200
> @@ -6,6 +6,7 @@
>   * Copyright (C) 1996, Olaf Kirch <okir@monad.swb.de>
>   */
>  
> +#include <linux/completion.h>
>  #include <linux/module.h>
>  #include <linux/types.h>
>  #include <linux/time.h>
> @@ -32,7 +33,7 @@ static int			reclaimer(void *ptr);
>   */
>  struct nlm_wait {
>  	struct nlm_wait *	b_next;		/* linked list */
> -	wait_queue_head_t	b_wait;		/* where to wait on */
> +	struct completion	b_wait;		/* where to wait on */
>  	struct nlm_host *	b_host;
>  	struct file_lock *	b_lock;		/* local file lock */
>  	unsigned short		b_reclaim;	/* got to reclaim lock */
> @@ -53,7 +54,7 @@ nlmclnt_block(struct nlm_host *host, str
>  
>  	block.b_host   = host;
>  	block.b_lock   = fl;
> -	init_waitqueue_head(&block.b_wait);
> +	init_completion(&block.b_wait);
>  	block.b_status = NLM_LCK_BLOCKED;
>  	block.b_next   = nlm_blocked;
>  	nlm_blocked    = &block;
> @@ -69,7 +70,8 @@ nlmclnt_block(struct nlm_host *host, str
>  	 * a 1 minute timeout would do. See the comment before
>  	 * nlmclnt_lock for an explanation.
>  	 */
> -	sleep_on_timeout(&block.b_wait, 30*HZ);
> +
> +	wait_for_completion_timeout(&block.b_wait, 30*HZ);
>  
>  	for (head = &nlm_blocked; *head; head = &(*head)->b_next) {
>  		if (*head == &block) {
> @@ -118,7 +120,7 @@ nlmclnt_grant(struct nlm_lock *lock)
>  	 * wake up the caller.
>  	 */
>  	block->b_status = NLM_LCK_GRANTED;
> -	wake_up(&block->b_wait);
> +	complete(&block->b_wait);
>  
>  	return nlm_granted;
>  }
> @@ -233,7 +235,7 @@ restart:
>  	for (block = nlm_blocked; block; block = block->b_next) {
>  		if (block->b_host == host) {
>  			block->b_status = NLM_LCK_DENIED_GRACE_PERIOD;
> -			wake_up(&block->b_wait);
> +			complete(&block->b_wait);
>  		}
>  	}
>  
> _
> 
> 

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH] lockd: replace semaphore, sleep_on

[Trond Myklebust]

> On Wed, Oct 20, 2004 at 09:15:43PM +0200, Thomas Gleixner wrote:
> > 
> > Use wait_event, completion instead of the obsolete sleep_on functions
> > and the abused semaphore

What is the point of all this timeout business when starting/stopping
lockd? We sure as hell don't want lockd to survive us if we are the
process that is responsible for killing it, and neither do we want to
start locking if there is no lockd to handle callbacks.
Please see the example of rpciod in net/sunrpc/sched.c for how to do
this correctly.

As for the client changes to the GRANTED mechanism, where is
wait_for_completion_timeout() defined? It is not part of Linus' kernel.

Cheers,
  Trond

-- 
Trond Myklebust <trond.myklebust@fys.uio.no>