fix recursive nlm_file_mutex deadlock

The attached patch seems to fix the issue - it skips (defers) the file removal. Eventually either nlm_gc_hosts (some time later when client is unmonitored) or nlmsvc_traverse_files will finish the clean up. Note that this is a 10-minutes work - not sure its ramification at this moment. Take a look ?

------------------------------------------------------------------------

I was testing NLM failover patches this morning and found the command hangs. Look like nlm_traverse_files(), where it grabs nlm_file_mutex early in the call, will have a chance to call nlm_release_file() via nlmsvc_free_block() inside kref_put(). The nlm_release_file() wants nlm_file_mutex too - this would generate a deadlock as the following:

dhcp59-234 kernel: Call Trace:

[<c02dd749>] __mutex_lock_slowpath+0x4c/0x7e
[<c02dd78a>] .text.lock.mutex+0xf/0x14
[<f8afeacd>] nlm_release_file+0x2b/0xdf [lockd]
[<f8afda90>] nlmsvc_free_block+0x8c/0x9d [lockd]
[<f8afda04>] nlmsvc_free_block+0x0/0x9d [lockd]
[<c01be98d>] kref_put+0x4e/0x58
[<f8afd175>] nlmsvc_traverse_blocks+0xaf/0xc6 [lockd]
[<f8afe960>] nlm_traverse_files+0x108/0x1cd [lockd]

The attached patch seems to fix the issue - it skips (defers) the file removal. Eventually either nlm_gc_hosts (some time later when client is unmonitored) or nlmsvc_traverse_files will finish the clean up. Note that this is a 10-minutes work - not sure its ramification at this moment. Take a look ?

-- Wendy

plain text document attachment (gfs_nlm_deadlock.patch)

Quoted 29 lines · Wendy Cheng

I was testing NLM failover patches this morning and found the command hangs. Look like nlm_traverse_files(), where it grabs nlm_file_mutex early in the call, will have a chance to call nlm_release_file() via nlmsvc_free_block() inside kref_put(). The nlm_release_file() wants nlm_file_mutex too - this would generate a deadlock as the following:

dhcp59-234 kernel: Call Trace:

[<c02dd749>] __mutex_lock_slowpath+0x4c/0x7e
[<c02dd78a>] .text.lock.mutex+0xf/0x14
[<f8afeacd>] nlm_release_file+0x2b/0xdf [lockd]
[<f8afda90>] nlmsvc_free_block+0x8c/0x9d [lockd]
[<f8afda04>] nlmsvc_free_block+0x0/0x9d [lockd]
[<c01be98d>] kref_put+0x4e/0x58
[<f8afd175>] nlmsvc_traverse_blocks+0xaf/0xc6 [lockd]
[<f8afe960>] nlm_traverse_files+0x108/0x1cd [lockd]

The attached patch seems to fix the issue - it skips (defers) the file removal. Eventually either nlm_gc_hosts (some time later when client is unmonitored) or nlmsvc_traverse_files will finish the clean up. Note that this is a 10-minutes work - not sure its ramification at this moment. Take a look ?

-- Wendy

plain text document attachment (gfs_nlm_deadlock.patch)

fs/lockd/svclock.c static void nlmsvc_free_block(struct kre · line 272

Quoted 10 lines

fs/lockd/svclock.c +3 -1

@@ -264,7 +264,9 @@ static void nlmsvc_free_block(struct kre
 
 	nlmsvc_freegrantargs(block->b_call);
 	nlm_release_call(block->b_call);
-	nlm_release_file(block->b_file);
+	down(&file->f_sema);
+	file->f_count--;
+	up(&file->f_sema);
 	kfree(block);
 }
 

Vetoed. The block holds a reference to the file. It _must_ call nlm_release_file() in order to release that reference. It is in any case a bug to grab file->f_sema without holding a reference to the file.

I suspect, rather, that the problem is due to nlmsvc_create_block() incrementing file->f_count without holding the nlm_file_mutex. If we convert it to an atomic_t instead, then that problem should be solved.

aside: Note also that we want to get rid of all that mark and sweep braindamage in nlm_traverse_*() with all the silly counting of f_lock, f_blocks, f_shares,.... and replace those variables with proper references to the struct nlm_file by the locks, blocks (is already the case?), and shares.

Cheers,
Trond

Disagree ! :)

The whole thing is about deadlock, not about reference count. Look at the logic ... nlm_traverse_files grabs the nlm_file_mutex, then comes down to nlm_release_file where it tries to get nlm_file_mutex lock again. I'll need to run now - we can discuss this tomorrow morning. Please re-read the issue and we'll discuss later.

The whole thing is about deadlock, not about reference count. Look at the logic ... nlm_traverse_files grabs the nlm_file_mutex, then comes down to nlm_release_file where it tries to get nlm_file_mutex lock again. I'll need to run now - we can discuss this tomorrow morning. Please re-read the issue and we'll discuss later.

Here you go. Assuming this compiles, it ought to prevent the recursive call to mutex_lock(&nlm_file_mutex), _and_ speed up nlm_release_file() in the case where we know we're not going to free up the file.

Cheers,
Trond

plain text document attachment (linux-2.6.18-009-
make_nlm_file_f_count_atomic.dif)
LOCKD: Make nlm_file->f_count an atomic

From: Trond Myklebust <[email protected]>

This allows us to grab the mutex in nlm_release_file only if we suspect that the file needs to be freed.

The latter fixes a deadlock in which nlm_traverse_files() ends up calling mutex_lock(&nlm_file_mutex) recursively (the second time being by means of the call to nlm_release_file() in nlmsvc_free_block()).

Signed-off-by Trond Myklebust <[email protected]>

---

 fs/lockd/svclock.c          |    2 +-
 fs/lockd/svcsubs.c          |   26 +++++++++++++++-----------
 include/linux/lockd/lockd.h |    2 +-
 3 files changed, 17 insertions(+), 13 deletions(-)

fs/lockd/svclock.c +1 -1

index c9d4197..57dcb2d 100644
@@ -207,7 +207,7 @@ nlmsvc_create_block(struct svc_rqst *rqs
 	block->b_daemon = rqstp->rq_server;
 	block->b_host   = host;
 	block->b_file   = file;
-	file->f_count++;
+	atomic_inc(&file->f_count);
 
 	/* Add to file's list of blocks */
 	block->b_fnext  = file->f_blocks;

fs/lockd/svcsubs.c +15 -11

index 2a4df9b..dfca9e7 100644
@@ -94,8 +94,10 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	mutex_lock(&nlm_file_mutex);
 
 	for (file = nlm_files[hash]; file; file = file->f_next)
-		if (!nfs_compare_fh(&file->f_handle, f))
+		if (!nfs_compare_fh(&file->f_handle, f)) {
+			atomic_inc(&file->f_count);
 			goto found;
+		}
 
 	nlm_debug_print_fh("creating file for", f);
 

@@ -108,6 +110,7 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	memcpy(&file->f_handle, f, sizeof(struct nfs_fh));
 	file->f_hash = hash;
 	init_MUTEX(&file->f_sema);
+	atomic_set(&file->f_count, 1);
 
 	/* Open the file. Note that this must not sleep for too long, else
 	 * we would lock up lockd:-) So no NFS re-exports, folks.

@@ -124,9 +127,8 @@ nlm_lookup_file(struct svc_rqst *rqstp, 
 	nlm_files[hash] = file;
 
 found:
-	dprintk("lockd: found file %p (count %d)\n", file, file->f_count);
+	dprintk("lockd: found file %p (count %d)\n", file, atomic_read(&file->f_count));
 	*result = file;
-	file->f_count++;
 	nfserr = 0;
 
 out_unlock:

@@ -221,7 +223,7 @@ nlm_inspect_file(struct nlm_host *host, 
 {
 	if (action == NLM_ACT_CHECK) {
 		/* Fast path for mark and sweep garbage collection */
-		if (file->f_count || file->f_blocks || file->f_shares)
+		if (atomic_read(&file->f_count) || file->f_blocks || file->f_shares)
 			return 1;
 	} else {
 		nlmsvc_traverse_blocks(host, file, action);

@@ -252,7 +254,7 @@ nlm_traverse_files(struct nlm_host *host
 
 			/* No more references to this file. Let go of it. */
 			if (!file->f_blocks && !file->f_locks
-			 && !file->f_shares && !file->f_count) {
+			 && !file->f_shares && !atomic_read(&file->f_count)) {
 				*fp = file->f_next;
 				nlmsvc_ops->fclose(file->f_file);
 				kfree(file);

@@ -278,18 +280,20 @@ void
 nlm_release_file(struct nlm_file *file)
 {
 	dprintk("lockd: nlm_release_file(%p, ct = %d)\n",
-				file, file->f_count);
-
-	/* Lock file table */
-	mutex_lock(&nlm_file_mutex);
+				file, atomic_read(&file->f_count));
 
 	/* If there are no more locks etc, delete the file */
-	if(--file->f_count == 0) {
+	if (atomic_dec_and_test(&file->f_count)) {
+		/*
+		 * Note! nlm_inspect_file() will check file->f_count
+		 * to see if we raced while taking nlm_file_mutex
+		 */
+		mutex_lock(&nlm_file_mutex);
 		if(!nlm_inspect_file(NULL, file, NLM_ACT_CHECK))
 			nlm_delete_file(file);
+		mutex_unlock(&nlm_file_mutex);
 	}
 
-	mutex_unlock(&nlm_file_mutex);
 }
 
 /*

include/linux/lockd/lockd.h +1 -2

index 0d92c46..59b63a1 100644
@@ -102,7 +102,7 @@ struct nlm_file {
 	struct nlm_share *	f_shares;	/* DOS shares */
 	struct nlm_block *	f_blocks;	/* blocked locks */
 	unsigned int		f_locks;	/* guesstimate # of locks */
-	unsigned int		f_count;	/* reference count */
+	atomic_t		f_count;	/* reference count */
 	struct semaphore	f_sema;		/* avoid concurrent access */
 	int		       	f_hash;		/* hash of f_handle */
 };
-------------------------------------------------------------------------

Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________ NFS maillist - [email protected] https://lists.sourceforge.net/lists/listinfo/nfs

We simply can't call nlm_release_file() as long as it requires nlm_file_mutex (no matter where you move the locking line) within nlm_traverse_files() code path. I'm ok with atomic variable though.

Quoted 3 lines · Wendy Cheng

We simply can't call nlm_release_file() as long as it requires nlm_file_mutex (no matter where you move the locking line) within nlm_traverse_files() code path. I'm ok with atomic variable though.

As long as we can guarantee that file->f_count doesn't drop to zero (and we can make that guarantee in nlm_traverse_file()), then the code is correct.