use completions instead of sleep_on for rpciod

[let's hope some sunrpc folks are on this list, too, the nfs list
mentioned in MAINTAINERS refuses postings from non-subsribers..]

The rpciod shutdown code gives ugly sleep_on without BKL warnings in -mm. And it looks indeed somewhat racy.

The easy fix would be to simply use a completion as in the patch below, but that removes all the signal fuzzing semantics the current code has. I don't really understand why we want to cancel the operation by signals, but I think it'd be better to leave that to people familar with the code anyway..

@@ -71,7 +71,7 @@
  * rpciod-related stuff
  */
 static DECLARE_WAIT_QUEUE_HEAD(rpciod_idle);
-static DECLARE_WAIT_QUEUE_HEAD(rpciod_killer);
+static DECLARE_COMPLETION(rpciod_killer);
 static DECLARE_MUTEX(rpciod_sema);
 static unsigned int		rpciod_users;
 static pid_t			rpciod_pid;

@@ -950,7 +950,6 @@
 static int
 rpciod(void *ptr)
 {
-	wait_queue_head_t *assassin = (wait_queue_head_t*) ptr;
 	int		rounds = 0;
 
 	lock_kernel();

@@ -992,11 +991,11 @@
 		rpciod_killall();
 	}
 
-	rpciod_pid = 0;
-	wake_up(assassin);
-
 	dprintk("RPC: rpciod exiting\n");
 	unlock_kernel();
+
+	rpciod_pid = 0;
+	complete_and_exit(&rpciod_killer, 0);
 	return 0;
 }
 

@@ -1041,7 +1040,7 @@
 	/*
 	 * Create the rpciod thread and wait for it to start.
 	 */
-	error = kernel_thread(rpciod, &rpciod_killer, 0);
+	error = kernel_thread(rpciod, NULL, 0);
 	if (error < 0) {
 		printk(KERN_WARNING "rpciod_up: create thread failed, error=%d\n", error);
 		rpciod_users--;

@@ -1057,8 +1056,6 @@
 void
 rpciod_down(void)
 {
-	unsigned long flags;
-
 	down(&rpciod_sema);
 	dprintk("rpciod_down pid %d sema %d\n", rpciod_pid, rpciod_users);
 	if (rpciod_users) {

@@ -1073,27 +1070,8 @@
 	}
 
 	kill_proc(rpciod_pid, SIGKILL, 1);
-	/*
-	 * Usually rpciod will exit very quickly, so we
-	 * wait briefly before checking the process id.
-	 */
-	clear_thread_flag(TIF_SIGPENDING);
-	yield();
-	/*
-	 * Display a message if we're going to wait longer.
-	 */
-	while (rpciod_pid) {
-		dprintk("rpciod_down: waiting for pid %d to exit\n", rpciod_pid);
-		if (signalled()) {
-			dprintk("rpciod_down: caught signal\n");
-			break;
-		}
-		interruptible_sleep_on(&rpciod_killer);
-	}
-	spin_lock_irqsave(&current->sighand->siglock, flags);
-	recalc_sigpending();
-	spin_unlock_irqrestore(&current->sighand->siglock, flags);
-out:
+	wait_for_completion(&rpciod_killer);
+ out:
 	up(&rpciod_sema);
 }
 

I think your patch is fine, and there are no signal issues (such code looks merely to be some abberation rather than anything else).

So I've added it to my tree.

Yes, this code is indeed one of the areas that breaks when you use the sunrpc code without the BKL. Another one was the missing spinlock in xprt_alloc_xid. The code in fs/nfs/unlink.c is probably also racy without BKL.

So if you remove the sunrpc BKL be prepared for lots and lots of bug reports :)

Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+ 


-------------------------------------------------------
The SF.Net email is sponsored by EclipseCon 2004
Premiere Conference on Open Tools Development and Integration
See the breadth of Eclipse activity. February 3-5 in Anaheim, CA.
http://www.eclipsecon.org/osdn
_______________________________________________
NFS maillist  -  [email protected]
https://lists.sourceforge.net/lists/listinfo/nfs

Here's another interesting one for you, this time in lockd:

nlmclnt_grace_wait() is completetly unused, removed.

nlmclnt_call() uses sleep_on_interruptible, but doesn't actually check for signals, I've fixed it to use wait_event_interruptible and check for signals, but again I'd prefer someone knowledgeable to take look at what this actually was intended to do..

@@ -195,19 +195,6 @@
 }
 
 /*
- * Wait while server is in grace period
- */
-static inline int
-nlmclnt_grace_wait(struct nlm_host *host)
-{
-	if (!host->h_reclaiming)
-		interruptible_sleep_on_timeout(&host->h_gracewait, 10*HZ);
-	else
-		interruptible_sleep_on(&host->h_gracewait);
-	return signalled()? -ERESTARTSYS : 0;
-}
-
-/*
  * Allocate an NLM RPC call struct
  */
 struct nlm_rqst *

@@ -254,8 +241,11 @@
 		msg.rpc_cred = nfs_file_cred(filp);
 
 	do {
-		if (host->h_reclaiming && !argp->reclaim) {
-			interruptible_sleep_on(&host->h_gracewait);
+		status = wait_event_interruptible(host->h_gracewait,
+				(!host->h_reclaiming || argp->reclaim));
+		if (status) {
+			if (signalled())
+				return -EINTR;
 			continue;
 		}
 

P=E5 m=E5 , 09/02/2004 klokka 11:28, skreiv Olaf Kirch:

-mm. And it looks indeed somewhat racy.

sunrpc code without the BKL. Another one was the missing spinlock in xprt_alloc_xid. The code in fs/nfs/unlink.c is probably also racy without BKL.

Ahem....

xprt_alloc_xid is quite safe. It's always called under xprt->xprt_lock.

The code in unlink.c is only called from dentry_iput() or from sillyrename(). In either case, there is no possibility for SMP races.

I'll admit that the rpciod killer code is racy, but I'm not sure that completions are the best answer, as they have at least one nasty feature:
they are not interruptible.

Cheers,
Trond

Would you like to back out the change then? I put it into Linus's tree already.

I had cause to poke around there recently, and it's *definitely* racy without BKL. That file would benefit from several modernisations:

1. move the list of nfs_unlinkdata from a global to a field in nfs_server

2. add a lock in nfs_server for it instead of implicit BKL in nfs_complete_unlink, nfs_async_unlink, nfs_put_unlinkdata

3. use proper list.h lists

Yes but it's worth doing, especially serverside.

Greg.

Greg Banks, R&D Software Engineer, SGI Australian Software Group.
I don't speak for SGI.

It wasn't in 2.4.19 or whenever I ran into this. And a per-transport lock isn't enough to protect a global variable shared by all transports. Trust me: I spent several weeks debugging this particular problem :) (See the PS for a somewhat lengthy description of what I remember)

I think that BKL removal at this point is probably not a good idea. Suse had a BKL removal patch in their SLES8 kernel, and I spent a total of 4-5 weeks hunting down all sorts of bugs related to SMP races. And I don't think we've seen all of these bugs yet. It might be better to postpone this for the 2.7 branch.

Are you sure? nfs_dentry_iput explicitly does a lock_kernel before calling nfs_complete_unlink. I think that's because dentry_iput explicitly drops the dcache_lock before calling dentry->d_op->d_iput. I wouldn't remove that BKL :)

And even if you leave that BKL and just remove the one in rpciod, you may still have a race between rpciod and dput. One CPU may be adding entries inside nfs_async_unlink (called from dentry_iput()), and at the same time rpciod can run on another CPU, calling nfs_async_unlink_release, removing stuff from the nfs_deletes list. The reference counting on nfs_unlinkdata isn't atomic either.

Olaf

PS: Let me try to put the pieces back together... this was on a PPC SMP system (so it can take a while for modified values to be be visible on other CPUs). There were several mounts being stressed in parallel.

xprt_request_init did a simple "req->rq_xid = xid++;" without any additional spin locks protecting the xid variable (we have that spin lock now).

The bug required at least 3 CPUs, and 3 different mounts.

CPU0 CPU1 CPU2
--------------------------------------------------------

   |    lock xprt1      lock xprt2
   |                                    lock xprt3
   |    read xid=0
   |                    read xid=0
   |    write xid=1
                                        read xid=1
 time++                                 write xid=2
                        write xid=1
   |                                    unlock xprt3
   |                                    lock xprt3
   |                                    read xid=1
   |                                    write xid=2
   v                                    unlock xprt3

Note that on PPC, modified data can take a long time to become visible on other CPUs. Cache sync is only guaranteed by operations such as spin_unlock etc.

Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+ 

I won't argue that point. But I've had to deal with the fall-out from "oh-well-they-seem-to-work-here" type of NFS BKL removal patches, so I'm quite wary of adding anything at this point in the game :)

Olaf

Olaf Kirch     |  Stop wasting entropy - start using predictable
[email protected]   |  tempfile names today!
---------------+ 

P=E5 ty , 10/02/2004 klokka 10:15, skreiv Olaf Kirch:

lock isn't enough to protect a global variable shared by all transports. Trust me: I spent several weeks debugging this particular problem :) (See the PS for a somewhat lengthy description of what I remember)

Duh... I forgot that the XID was still global...

Does anybody see any problems with moving the XID counter into the struct xprt? Please bear in mind that the replay caches on modern NFS servers look at XID+client IP address+port number, so 2 sockets that play the sam= e
XID to the server should not normally conflict.

e
had a BKL removal patch in their SLES8 kernel, and I spent a total of 4-5

think we've seen all of these bugs yet. It might be better to postpone this for the 2.7 branch.

I totally agree with this...

No extra work has been done on BKL removal in the 2.6 branch, and I know that the old patch that was up for testing in the 2.4 tree was buggy in several ways...

sillyrename(). In either case, there is no possibility for SMP races.

calling

s
the dcache_lock before calling dentry->d_op->d_iput. I wouldn't remove that BKL :)

Yep, but there is only one inode per dentry, and the VFS rules forbids yo= u
to convert a negative dentry into a positive one (the contrary is allowed= ,
but only under certain restricted situations).

may

s
inside nfs_async_unlink (called from dentry_iput()), and at the same time rpciod can run on another CPU, calling nfs_async_unlink_release, removing stuff from the nfs_deletes list. The reference counting on nfs_unlinkdata isn't atomic either.

Sure... The unlink.c global lists definitely depend on the BKL for protection. The latter would need to be replaced by a new global spinlock.

Cheers,
Trond