FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Cluster Development

 
 
LinkBack Thread Tools
 
Old 01-07-2008, 04:53 AM
Wendy Cheng
 
Default Fix lockd panic

This small patch has not been changed since our last discussion:
http://www.opensubscriber.com/message/nfs@lists.sourceforge.net/6348912.html

To recap the issue, a client could ask for a posix lock that invokes:

>>> server calls nlm4svc_proc_lock() ->
>>> * server lookup file (f_count++)
>>> * server lock the file
>>> * server calls nlm_release_host
>>> * server calls nlm_release_file (f_count--)
>>> * server return to client with status 0
>>>

As part of the lookup file, the lock stays on vfs inode->i_flock list
with zero f_count. Any call into nlm_traverse_files() will BUG() in
locks_remove_flock() (fs/locks.c:2034) during fclose(), if that file
happens to be of no interest to that particular search. Since after
nlm_inspect_file(), the logic unconditionally checks for possible
removing of the file. As the file is not blocked, nothing to do with
shares, and f_count is zero, it will get removed from hash and fclose()
invoked with the posix lock hanging on i_flock list.


-- Wendy
 
Old 01-08-2008, 04:31 AM
Neil Brown
 
Default Fix lockd panic

On Monday January 7, wcheng@redhat.com wrote:
> This small patch has not been changed since our last discussion:
> http://www.opensubscriber.com/message/nfs@lists.sourceforge.net/6348912.html
>
> To recap the issue, a client could ask for a posix lock that invokes:
>
> >>> server calls nlm4svc_proc_lock() ->
> >>> * server lookup file (f_count++)
> >>> * server lock the file
> >>> * server calls nlm_release_host
> >>> * server calls nlm_release_file (f_count--)
> >>> * server return to client with status 0
> >>>
>
> As part of the lookup file, the lock stays on vfs inode->i_flock list
> with zero f_count. Any call into nlm_traverse_files() will BUG() in
> locks_remove_flock() (fs/locks.c:2034) during fclose(), if that file
> happens to be of no interest to that particular search. Since after
> nlm_inspect_file(), the logic unconditionally checks for possible
> removing of the file. As the file is not blocked, nothing to do with
> shares, and f_count is zero, it will get removed from hash and fclose()
> invoked with the posix lock hanging on i_flock list.

If I'm reading this correctly, this bug is introduced by your previous
patch.

The important difference between the old code and the new code here is
that the old code tests "file->f_locks" while the new code iterates
through i_flock to see if there are any lockd locks.

f_locks is set to a count of lockd locks in nlm_traverse_locks which
*was* always called by nlm_inspect_file which is called immediately
before the code you are changing.
But since your patch, nlm_inspect_file does not always call
nlm_traverse_locks, so there is a chance that f_locks is wrong.

With this patch, f_locks it not used at all any more.

Introducing a bug in one patch and fixing in the next is bad style.

Some options:

Have an initial patch which removes all references to f_locks and
includes the change in this patch. With that in place your main
patch won't introduce a bug. If you do this, you should attempt to
understand and justify the performance impact (does nlm_traverse_files
become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the
call to nlm_inspect_file.

something else???

So NAK for this one in it's current form... unless I've misunderstood
something.

NeilBrown

>
> -- Wendy
>
> 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.
>
> Signed-off-by: S. Wendy Cheng <wcheng@redhat.com>
>
> svcsubs.c | 3 +--
> 1 files changed, 1 insertion(+), 2 deletions(-)
>
> --- linux-nlm-1/fs/lockd/svcsubs.c 2008-01-06 18:23:20.000000000 -0500
> +++ linux/fs/lockd/svcsubs.c 2008-01-06 18:24:12.000000000 -0500
> @@ -332,8 +332,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);
 
Old 01-09-2008, 02:02 AM
Wendy Cheng
 
Default Fix lockd panic

Neil Brown wrote:



If I'm reading this correctly, this bug is introduced by your previous
patch.


Depending on how you see the issue. From my end, I view this as the
existing code has a "trap" and I fell into it. This is probably a chance
to clean up this logic.



The important difference between the old code and the new code here is
that the old code tests "file->f_locks" while the new code iterates
through i_flock to see if there are any lockd locks.

f_locks is set to a count of lockd locks in nlm_traverse_locks which
*was* always called by nlm_inspect_file which is called immediately
before the code you are changing.
But since your patch, nlm_inspect_file does not always call
nlm_traverse_locks, so there is a chance that f_locks is wrong.

With this patch, f_locks it not used at all any more.



Yes, a fair description of the issue !


Introducing a bug in one patch and fixing in the next is bad style.



ok .....


Some options:

Have an initial patch which removes all references to f_locks and
includes the change in this patch. With that in place your main
patch won't introduce a bug. If you do this, you should attempt to
understand and justify the performance impact (does nlm_traverse_files
become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the
call to nlm_inspect_file.

something else???




Let's see what hch says in another email... will come back to this soon.


So NAK for this one in it's current form... unless I've misunderstood
something.




I expect this

-- Wendy
 
Old 01-09-2008, 03:43 AM
Wendy Cheng
 
Default Fix lockd panic

Wendy Cheng wrote:


Neil Brown wrote:


Some options:

Have an initial patch which removes all references to f_locks and
includes the change in this patch. With that in place your main
patch won't introduce a bug. If you do this, you should attempt to
understand and justify the performance impact (does nlm_traverse_files
become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the
call to nlm_inspect_file.

something else???




Let's see what hch says in another email... will come back to this soon.



Will do a quick and dirty performance measure tomorrow when I get to the
office. Then we'll go from there.


-- Wendy
 
Old 01-09-2008, 10:33 PM
Wendy Cheng
 
Default Fix lockd panic

Wendy Cheng wrote:

Wendy Cheng wrote:


Neil Brown wrote:


Some options:

Have an initial patch which removes all references to f_locks and
includes the change in this patch. With that in place your main
patch won't introduce a bug. If you do this, you should attempt to
understand and justify the performance impact (does nlm_traverse_files
become quadratic in number of locks. Is that acceptable?).

Change the first patch to explicitly update f_count if you bypass the
call to nlm_inspect_file.

something else???




Let's see what hch says in another email... will come back to this soon.



Will do a quick and dirty performance measure tomorrow when I get to
the office. Then we'll go from there.


The test didn't go well and I'm still debugging .. However, let's
revisit the issue:


[quot from Neil's comment]
The important difference between the old code and the new code here is
that the old code tests "file->f_locks" while the new code iterates
through i_flock to see if there are any lockd locks.

f_locks is set to a count of lockd locks in nlm_traverse_locks which
*was* always called by nlm_inspect_file which is called immediately
before the code you are changing.
But since your patch, nlm_inspect_file does not always call
nlm_traverse_locks, so there is a chance that f_locks is wrong.

With this patch, f_locks it not used at all any more.
[end quot]

The point here is "with this patch, f_locks it not used at all any
more." Note that we have a nice inline function "nlm_file_inuse", why
should we use f_locks (that I assume people agree that it is awkward) ?
Could we simply drop f_locks all together in this section of code?


-- Wendy
 
Old 01-12-2008, 05:51 AM
Wendy Cheng
 
Default Fix lockd panic

Wendy Cheng wrote:

The point here is "with this patch, f_locks it not used at all any
more." Note that we have a nice inline function "nlm_file_inuse", why
should we use f_locks (that I assume people agree that it is awkward)
? Could we simply drop f_locks all together in this section of code?



Start to have a second thought about this ....

Removing f_locks does make the code much more readable. However, if
inode->i_flock list is long, e.g. large amount of (clients) hosts and/or
processes from other hosts competing for the same lock, we don't want to
do the list walk twice within nlm_traverse_files(). Intuitively this is
unlikely but I prefer not changing the current behavior. As the result,
I'm withdrawing the patch all together.


The first patch will handle the issue correctly. Will submit it after
this post.


-- Wendy
 

Thread Tools




All times are GMT. The time now is 03:24 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org