Fix NLM reference count panic

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

This fixes the incorrect fclose call inside nlm_traverse_files() where a posix lock could still be held by NFS client. Problem was found in a kernel panic inside locks_remove_flock() (fs/locks.c:2034) as part of the fclose call due to NFS-NLM locks still hanging on inode->i_flock list.

Also see: http://people.redhat.com/wcheng/Patches/NFS/NLM/001.txt

 svcsubs.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

fs/lockd/svcsubs.c +1 -2

@@ -250,8 +250,7 @@ nlm_traverse_files(struct nlm_host *host
 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
-			 && !file->f_shares && !file->f_count) {
+			if (!nlm_file_inuse(file)) {
 				hlist_del(&file->f_list);
 				nlmsvc_ops->fclose(file->f_file);
 				kfree(file);

This fixes the incorrect fclose call inside nlm_traverse_files() where a posix lock could still be held by NFS client. Problem was found in a kernel panic inside locks_remove_flock() (fs/locks.c:2034) as part of the fclose call due to NFS-NLM locks still hanging on inode->i_flock list.

Also see: http://people.redhat.com/wcheng/Patches/NFS/NLM/001.txt

 svcsubs.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

Quoted 3 lines · Wendy Cheng

This fixes the incorrect fclose call inside nlm_traverse_files() where a posix lock could still be held by NFS client. Problem was found in a kernel panic inside locks_remove_flock() (fs/locks.c:2034) as part of the fclose call due to NFS-NLM locks still hanging on inode->i_flock list.

Quoted 3 lines

Also see: http://people.redhat.com/wcheng/Patches/NFS/NLM/001.txt

Next time it'd be best just to include that into the referred-to text into the message.

Quoted 10 lines

svcsubs.c | 3 +--

1 files changed, 1 insertion(+), 2 deletions(-)

fs/lockd/svcsubs.c nlm_traverse_files(struct nlm_host *host · line 256

Quoted 9 lines

fs/lockd/svcsubs.c +1 -2

@@ -250,8 +250,7 @@ nlm_traverse_files(struct nlm_host *host
 			mutex_lock(&nlm_file_mutex);
 			file->f_count--;
 			/* No more references to this file. Let go of it. */
-			if (list_empty(&file->f_blocks) && !file->f_locks
-			 && !file->f_shares && !file->f_count) {
+			if (!nlm_file_inuse(file)) {
 				hlist_del(&file->f_list);
 				nlmsvc_ops->fclose(file->f_file);
 				kfree(file);

This just replaces the file->f_locks check by a search of the inode's lock list.

What confuses me here is that the nlm_inspect_file() call just above already did that search, and set file->f_locks accordingly. The only difference is that now we've acquired the nlm_file_mutex. I don't understand yet how that makes a difference.

Quoted 10 lines · J. Bruce Fields

On Fri, Jan 04, 2008 at 05:48:49PM -0500, Wendy Cheng wrote:
This just replaces the file->f_locks check by a search of the inode's lock list.

What confuses me here is that the nlm_inspect_file() call just above already did that search, and set file->f_locks accordingly. The only difference is that now we've acquired the nlm_file_mutex. I don't understand yet how that makes a difference.

You're right. I got the patch sequence wrong. It will cause panic only when we selectively unlock nlm locks under my "unlock" patch as the following. See how it returns without doing nlm_traverse_locks() below ... Let's combine this patch into the big unlock patch so we won't have any confusion. The unlock patch will submitted on Monday after this weekend's sanity check test run. In short, I withdraw this patch... Wendy

nlm_inspect_file(struct nlm_host *host, struct nlm_file *file, nlm_host_match_fn_t match)
{
+ /* Cluster failover has timing constraints. There is a slight
+ * performance hit if nlm_fo_unlock_match() is implemented as
+ * a match fn (since it will be invoked for each block, share,
+ * and lock later when the lists are traversed). Instead, we
+ * add path-matching logic into the following unlikely clause.
+ * If matches, the dummy nlmsvc_fo_match will always return
+ * true.
+ */
+ dprintk("nlm_inspect_files: file=%p\n", file);
+ if (unlikely(match == nlmsvc_fo_match)) {
+ if (!nlmsvc_fo_unlock_match((void *)host, file))
+ return 0;
+ fo_printk("nlm_fo find lock file entry (0x%p)\n", file);
+ }
+
nlmsvc_traverse_blocks(host, file, match); nlmsvc_traverse_shares(host, file, match);
return nlm_traverse_locks(host, file, match);

(quoted diff) +0 -0

@@ -369,3 +451,35 @@ nlmsvc_invalidate_all(void)
 */
 nlm_traverse_files(NULL, nlmsvc_is_client);
 }

Quoted 16 lines · Wendy Cheng

Quoted 12 lines · J. Bruce Fields

On Fri, Jan 04, 2008 at 05:48:49PM -0500, Wendy Cheng wrote:
This just replaces the file->f_locks check by a search of the inode's lock list.

What confuses me here is that the nlm_inspect_file() call just above already did that search, and set file->f_locks accordingly. The only difference is that now we've acquired the nlm_file_mutex. I don't understand yet how that makes a difference.

Quoted 3 lines

You're right. I got the patch sequence wrong. It will cause panic only when we selectively unlock nlm locks under my "unlock" patch as the following. See how it returns without doing nlm_traverse_locks() below ... Let's combine this patch into the big unlock patch so we won't have any confusion. The unlock patch will submitted on Monday after this weekend's sanity check test run. In short, I withdraw this patch... Wendy

OK. I'll admit to still being a little confused--but with luck it'll all make sense when I see the whole thing. Have a good weekend!

Actually the *existing* logic (implying without my changes) is awkward. It uses file->f_locks to carry the result of unlock operation. Leave the non-zero f_locks value hanging there if unlock fails, It only resets it back to zero when next time nlm_inspect_file is called. Code like this is a good place to generate subtle race conditions that could result with file close failure. Just a complaint - I don't have a solid proof that it has caused troubles though.