Skip to contentSkip to conversation

lockd

PATCH In review Steve Dickson 3 participants 6 messages Oct 4, 2004

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 lockd

Hey Neil,

Attached is a patch that fixes some potential SMP races in the lockd code that were identified by the SLEEP_ON_BKLCHECK that was (at one time) in the -mm tree...

fs/lockd/clntlock.c renamed from fs/lockd/clntlock.c.old +8 -2
@@ -50,14 +50,19 @@ nlmclnt_block(struct nlm_host *host, str
 	struct nlm_wait	block, **head;
 	int		err;
 	u32		pstate;
+	wait_queue_t __wait;
 
 	block.b_host   = host;
 	block.b_lock   = fl;
-	init_waitqueue_head(&block.b_wait);
 	block.b_status = NLM_LCK_BLOCKED;
+	init_waitqueue_entry(&__wait, current);
+	init_waitqueue_head(&block.b_wait);
+	add_wait_queue(&block.b_wait, &__wait);
+
 	block.b_next   = nlm_blocked;
 	nlm_blocked    = █
 
+
 	/* Remember pseudo nsm state */
 	pstate = host->h_state;
 
@@ -69,7 +74,7 @@ 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);
+	schedule_timeout(30*HZ);
 
 	for (head = &nlm_blocked; *head; head = &(*head)->b_next) {
 		if (*head == &block) {
@@ -77,6 +82,7 @@ nlmclnt_block(struct nlm_host *host, str
 			break;
 		}
 	}
+	remove_wait_queue(&block.b_wait, &__wait);
 
 	if (!signalled()) {
 		*statp = block.b_status;
fs/lockd/svc.c renamed from fs/lockd/svc.c.old +17 -2
@@ -278,6 +278,8 @@ void
 lockd_down(void)
 {
 	static int warned;
+	wait_queue_t __wait;
+	int retries=0;
 
 	down(&nlmsvc_sema);
 	if (nlmsvc_users) {
@@ -294,20 +296,33 @@ lockd_down(void)
 	warned = 0;
 
 	kill_proc(nlmsvc_pid, SIGKILL, 1);
+
+	init_waitqueue_entry(&__wait, current);
+	add_wait_queue(&lockd_exit,  &__wait);
+
 	/*
 	 * 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) {
+	set_current_state(TASK_UNINTERRUPTIBLE);
+	while (nlmsvc_pid) {
+
+		schedule_timeout(HZ);
+		if (retries++ < 3)
+			continue;
+
 		printk(KERN_WARNING 
 			"lockd_down: lockd failed to exit, clearing pid\n");
 		nlmsvc_pid = 0;
 	}
+	set_current_state(TASK_RUNNING);
+	remove_wait_queue(&lockd_exit,  &__wait);
+
 	spin_lock_irq(&current->sighand->siglock);
 	recalc_sigpending();
 	spin_unlock_irq(&current->sighand->siglock);
+
 out:
 	up(&nlmsvc_sema);
 }
#1 lore raw

P=E5 m=E5 , 04/10/2004 klokka 18:24, skreiv Steve Dickson:

Quoted 1 line

Hey Neil,

Hey! This is the client side NLM code... 8-)

fs/lockd/svc.c lockd_down(void) · line 308

Quoted 4 lines
fs/lockd/svc.c renamed from fs/lockd/svc.c.old +1 -2
 	clear_thread_flag(TIF_SIGPENDING);
-	interruptible_sleep_on_timeout(&lockd_exit, HZ);
-	if (nlmsvc_pid) {
+	set_current_state(TASK_UNINTERRUPTIBLE);

Nope. Those clearly are not the same.

Note that you probably also want to move the call to set_current_state(TASK_INTERRUPTIBLE) inside the loop. In that case you can also remove the call to set_current_state(TASK_RUNNING) ('cos schedule_timeout() will do that for you).

Also, why aren't you using the more standard DECLARE_WAITQUEUE(__wait)?

Cheers,
Trond

#2 lore raw
Quoted 11 lines · Trond Myklebust

P=E5 m=E5 , 04/10/2004 klokka 18:24, skreiv Steve Dickson:

=20

Quoted 3 lines

Hey Neil,
=20

Hey! This is the client side NLM code... 8-)
=20

Sorry buddy.... I'm having one of those days!!!! :-\

Quoted 2 lines

Note that you probably also want to move the call to set_current_state(TASK_INTERRUPTIBLE) inside the loop. In that case yo=

u

Quoted 5 lines

can also remove the call to set_current_state(TASK_RUNNING) ('cos schedule_timeout() will do that for you).

=20

Ok...

Quoted 1 line

Also, why aren't you using the more standard DECLARE_WAITQUEUE(__wait)=

?

Quoted 2 lines

=20

I guess I didn't realize that would be a better way to do it... I'll=20 look into to...

thanks,

SteveD.

#3 lore raw
In reply to Steve Dickson

P=E5 m=E5 , 04/10/2004 klokka 18:24, skreiv Steve Dickson:

Quoted 5 lines

Hey Neil,
=20
Attached is a patch that fixes some potential SMP races in the lockd code that were identified by the SLEEP_ON_BKLCHECK that was (at one time) in the -mm tree...

Just for the record: the "SMP race condition" argument given here is completely bogus. sleep_on_* is quite safe to use when the SMP races ar= e
being handled using the BKL, as is the case here.

That said, I agree that the patch is of interest given the long term goal of removing the BKL completely. Perhaps you could therefore also amend your changelog entry text to reflect this motive?

Cheers,
Trond

#4 lore raw
Quoted 10 lines · Trond Myklebust

På må , 04/10/2004 klokka 18:24, skreiv Steve Dickson:

Quoted 5 lines

Hey Neil,

Attached is a patch that fixes some potential SMP races in the lockd code that were identified by the SLEEP_ON_BKLCHECK that was (at one time) in the -mm tree...

Just for the record: the "SMP race condition" argument given here is completely bogus. sleep_on_* is quite safe to use when the SMP races are being handled using the BKL, as is the case here.

actually this triggered because there was NO bkl... if you hold the bkl the warning doesn't trigger.....

#5 lore raw

P=E5 m=E5 , 04/10/2004 klokka 20:13, skreiv Arjan van de Ven:

Quoted 1 line

actually this triggered because there was NO bkl... if you hold the b=

kl

Quoted 1 line

the warning doesn't trigger.....

Then the fix is downright wrong.

We *must* be holding the BKL upon entry to nlmclnt_lock(). All sorts of other things depend on it.

Cheers,
Trond