[PATCH] fix recursive nlm_file_mutex deadlock

[PATCH] fix recursive nlm_file_mutex deadlock

[Wendy Cheng]

[-- Attachment #1: Type: text/plain, Size: 1068 bytes --]

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


[-- Attachment #2: gfs_nlm_deadlock.patch --]
[-- Type: text/plain, Size: 388 bytes --]

--- linux-2/fs/lockd/svclock.c	2006-08-08 10:20:16.000000000 -0400
+++ linux/fs/lockd/svclock.c	2006-08-09 10:28:35.000000000 -0400
@@ -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);
 }
 

[-- Attachment #3: Type: text/plain, Size: 373 bytes --]

-------------------------------------------------------------------------
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

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Wendy Cheng]

Wendy Cheng wrote:

>
> 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 ?
>
Well, I never really understand what this "signed-off" line is all 
about. But if it is required, here it is:

Signed-off-by: S. Wendy Cheng (wcheng@redhat.com)


>
>------------------------------------------------------------------------
>
>--- linux-2/fs/lockd/svclock.c	2006-08-08 10:20:16.000000000 -0400
>+++ linux/fs/lockd/svclock.c	2006-08-09 10:28:35.000000000 -0400
>@@ -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);
> }
> 
>  
>

-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust]

On Wed, 2006-08-09 at 14:13 -0400, Wendy Cheng wrote:
> 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)
> --- linux-2/fs/lockd/svclock.c	2006-08-08 10:20:16.000000000 -0400
> +++ linux/fs/lockd/svclock.c	2006-08-09 10:28:35.000000000 -0400
> @@ -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


-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Wendy Cheng]

Trond Myklebust wrote:

>On Wed, 2006-08-09 at 14:13 -0400, Wendy Cheng wrote:
>  
>
>>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)
>>--- linux-2/fs/lockd/svclock.c	2006-08-08 10:20:16.000000000 -0400
>>+++ linux/fs/lockd/svclock.c	2006-08-09 10:28:35.000000000 -0400
>>@@ -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.
>  
>

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.

-- Wendy

>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
>
>  
>

-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust]

On Wed, 2006-08-09 at 18:07 -0400, Wendy Cheng wrote:

> 
> 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.

OK. The we need to fix the borken mark/sweep crap. Trying to circumvent
refcounting rules is unacceptable.

Trond


-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 617 bytes --]

On Wed, 2006-08-09 at 18:07 -0400, Wendy Cheng wrote:
> 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


[-- Attachment #2: linux-2.6.18-009-make_nlm_file_f_count_atomic.dif --]
[-- Type: text/plain, Size: 4152 bytes --]

LOCKD: Make nlm_file->f_count an atomic

From: Trond Myklebust <Trond.Myklebust@netapp.com>

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 <Trond.Myklebust@netapp.com>
---

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

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index c9d4197..57dcb2d 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -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;
diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 2a4df9b..dfca9e7 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -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);
 }
 
 /*
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 0d92c46..59b63a1 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -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 */
 };

[-- Attachment #3: Type: text/plain, Size: 373 bytes --]

-------------------------------------------------------------------------
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

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Wendy Cheng]

On Wed, 2006-08-09 at 19:57 -0400, Trond Myklebust wrote:

> 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.

Look like this issue can't be patched by any 10-minutes code (that
applies to both of us). 

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.

I always think nlm_file_mutex is used to protect nlm_files *list* (which
is a list of individual nlm_file) and file->f_sema is used to protect
the *individual* nlm_file. But I can be wrong (and will go to read the
code more).

In short, you patch will deadlock again.

-- Wendy

> 
> 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 <Trond.Myklebust@netapp.com>
> 
> 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 <Trond.Myklebust@netapp.com>
> ---
> 
>  fs/lockd/svclock.c          |    2 +-
>  fs/lockd/svcsubs.c          |   26 +++++++++++++++-----------
>  include/linux/lockd/lockd.h |    2 +-
>  3 files changed, 17 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index c9d4197..57dcb2d 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -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;
> diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
> index 2a4df9b..dfca9e7 100644
> --- a/fs/lockd/svcsubs.c
> +++ b/fs/lockd/svcsubs.c
> @@ -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);
>  }
>  
>  /*
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 0d92c46..59b63a1 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -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 - NFS@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/nfs


-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust]

On Thu, 2006-08-10 at 11:24 -0400, Wendy Cheng wrote:

> 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.

  Trond


-------------------------------------------------------------------------
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  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs

Re: [PATCH] fix recursive nlm_file_mutex deadlock

[Trond Myklebust]

[-- Attachment #1: Type: text/plain, Size: 754 bytes --]

On Thu, 2006-08-10 at 11:40 -0400, Trond Myklebust wrote:
> On Thu, 2006-08-10 at 11:24 -0400, Wendy Cheng wrote:
> 
> > 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.

Doh! There is no reason whatsoever to be holding nlm_file_mutex across
the call to nlm_inspect_file. As you said, the only thing it does is
protect the list, and we don't care about that.

The simplest fix is just to do something like the following.

Cheers,
  Trond

[-- Attachment #2: linux-2.6.18-009-fix_nlm_traverse_files_deadlock.dif --]
[-- Type: message/rfc822, Size: 1653 bytes --]

From: Trond Myklebust <Trond.Myklebust@netapp.com>
Subject: No Subject
Date: Thu, 10 Aug 2006 12:01:51 -0400
Message-ID: <1155225711.10547.49.camel@localhost>

nlm_traverse_files() is not allowed to hold the nlm_file_mutex while calling
nlm_inspect file, since it may end up calling nlm_release_file() when
releaseing the blocks.

Signed-off-by: Trond Myklebust <Trond.Myklebust@netapp.com>
---

 fs/lockd/svcsubs.c |   15 +++++++++------
 1 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/svcsubs.c b/fs/lockd/svcsubs.c
index 2a4df9b..01b4db9 100644
--- a/fs/lockd/svcsubs.c
+++ b/fs/lockd/svcsubs.c
@@ -237,19 +237,22 @@ static int
 nlm_traverse_files(struct nlm_host *host, int action)
 {
 	struct nlm_file	*file, **fp;
-	int		i;
+	int i, ret = 0;
 
 	mutex_lock(&nlm_file_mutex);
 	for (i = 0; i < FILE_NRHASH; i++) {
 		fp = nlm_files + i;
 		while ((file = *fp) != NULL) {
+			file->f_count++;
+			mutex_unlock(&nlm_file_mutex);
+
 			/* Traverse locks, blocks and shares of this file
 			 * and update file->f_locks count */
-			if (nlm_inspect_file(host, file, action)) {
-				mutex_unlock(&nlm_file_mutex);
-				return 1;
-			}
+			if (nlm_inspect_file(host, file, action))
+				ret = 1;
 
+			mutex_lock(&nlm_file_mutex);
+			file->f_count--;
 			/* No more references to this file. Let go of it. */
 			if (!file->f_blocks && !file->f_locks
 			 && !file->f_shares && !file->f_count) {
@@ -262,7 +265,7 @@ nlm_traverse_files(struct nlm_host *host
 		}
 	}
 	mutex_unlock(&nlm_file_mutex);
-	return 0;
+	return ret;
 }
 
 /*

[-- Attachment #3: Type: text/plain, Size: 373 bytes --]

-------------------------------------------------------------------------
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

[-- Attachment #4: Type: text/plain, Size: 140 bytes --]

_______________________________________________
NFS maillist  -  NFS@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/nfs