[CC Jeff, I found your bz432855 where you noticed the same problem]
Hello,
When a client attempting to lock a file and waiting for NLM
GRANTED callback is interrupted by a signal in nlmclnt_block(), bad
things happen.
The userspace program will get EINTR and most likely retry the lock,
which it can successfully obtain (when the GRANTED callback comes).
That sounds OK, but something causes the client to lose track of this
lock and when the program tries to release the lock, the NLM UNLOCK
message is sent with a new SVID. The NFS server rightfully thinks
the file is still locked by the original SVID. Noone can lock the file
again.
I think the bug is in do_setlk(), where the VFS lock is created when
the lock attempt was interrupted by a signal. I don't understand the
reason why it does that. nlmclnt_lock() cancels the lock request if
nlmclnt_block() is interrupted, so there should be no state on the
server to be cleaned (as the comment suggests there is). But I may be
missing something.
Attached is a test program. It locks a file and waits for Enter before
unlocking it. It shows the locks held at the interesting moments
(from /proc/locks).
To reproduce the bug, run the program on two clients at the same time:
./lockunlock /mnt/nfs_server/shared_file
The first client will get the lock. Press CTRL+C on the second one, to
interrupt its wait for NLM GRANTED. Notice the VFS lock shown. Press
Enter on the first client, to release the lock. The second client will
obtain it. Let it release it. Try running it again and notice you can't
lock the file anymore.
I have tried the following patch and it helps in my testcase. But maybe
it breaks something else?
Comments welcome.
Michal
---
fs/nfs/file.c | 12 ++----------
1 files changed, 2 insertions(+), 10 deletions(-)
diff --git a/fs/nfs/file.c b/fs/nfs/file.c
index ef57a5a..b311d9a 100644
--- a/fs/nfs/file.c
+++ b/fs/nfs/file.c
@@ -572,17 +572,9 @@ static int do_setlk(struct file *filp, int cmd,
struct file_lock *fl)
lock_kernel();
/* Use local locking if mounted with "-onolock" */
- if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM)) {
+ if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM))
status = NFS_PROTO(inode)->lock(filp, cmd, fl);
- /* If we were signalled we still need to ensure that
- * we clean up any state on the server. We therefore
- * record the lock call as having succeeded in order to
- * ensure that locks_remove_posix() cleans it out when
- * the process exits.
- */
- if (status == -EINTR || status == -ERESTARTSYS)
- do_vfs_lock(filp, fl);
- } else
+ else
status = do_vfs_lock(filp, fl);
unlock_kernel();
if (status < 0)
--
1.5.3.6
On Thu, 2008-03-13 at 19:32 +0100, Michal Schmidt wrote:
> [CC Jeff, I found your bz432855 where you noticed the same problem]
>
> Hello,
>
> When a client attempting to lock a file and waiting for NLM
> GRANTED callback is interrupted by a signal in nlmclnt_block(), bad
> things happen.
>
> The userspace program will get EINTR and most likely retry the lock,
> which it can successfully obtain (when the GRANTED callback comes).
> That sounds OK, but something causes the client to lose track of this
> lock and when the program tries to release the lock, the NLM UNLOCK
> message is sent with a new SVID. The NFS server rightfully thinks
> the file is still locked by the original SVID. Noone can lock the file
> again.
>
> I think the bug is in do_setlk(), where the VFS lock is created when
> the lock attempt was interrupted by a signal. I don't understand the
> reason why it does that. nlmclnt_lock() cancels the lock request if
> nlmclnt_block() is interrupted, so there should be no state on the
> server to be cleaned (as the comment suggests there is). But I may be
> missing something.
>
> Attached is a test program. It locks a file and waits for Enter before
> unlocking it. It shows the locks held at the interesting moments
> (from /proc/locks).
>
> To reproduce the bug, run the program on two clients at the same time:
> ./lockunlock /mnt/nfs_server/shared_file
>
> The first client will get the lock. Press CTRL+C on the second one, to
> interrupt its wait for NLM GRANTED. Notice the VFS lock shown. Press
> Enter on the first client, to release the lock. The second client will
> obtain it. Let it release it. Try running it again and notice you can't
> lock the file anymore.
>
> I have tried the following patch and it helps in my testcase. But maybe
> it breaks something else?
>
> Comments welcome.
>
> Michal
>
>
> ---
> fs/nfs/file.c | 12 ++----------
> 1 files changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/fs/nfs/file.c b/fs/nfs/file.c
> index ef57a5a..b311d9a 100644
> --- a/fs/nfs/file.c
> +++ b/fs/nfs/file.c
> @@ -572,17 +572,9 @@ static int do_setlk(struct file *filp, int cmd,
> struct file_lock *fl)
> lock_kernel();
> /* Use local locking if mounted with "-onolock" */
> - if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM)) {
> + if (!(NFS_SERVER(inode)->flags & NFS_MOUNT_NONLM))
> status = NFS_PROTO(inode)->lock(filp, cmd, fl);
> - /* If we were signalled we still need to ensure that
> - * we clean up any state on the server. We therefore
> - * record the lock call as having succeeded in order to
> - * ensure that locks_remove_posix() cleans it out when
> - * the process exits.
> - */
> - if (status == -EINTR || status == -ERESTARTSYS)
> - do_vfs_lock(filp, fl);
> - } else
> + else
> status = do_vfs_lock(filp, fl);
> unlock_kernel();
> if (status < 0)
I think the intent is basically what the comment says...
If the call is interrupted, we don't know what the state of the lock on
the server is. There's a chance that the server could have granted us
the lock. If we don't set a vfs lock then we never send an unlock
request to the server, and then nobody will get it or clean it up.
I guess the thinking is that if the process is signaled that it will
probably be exiting soon anyway. That's probably not a safe assumption,
however.
The ideal thing might be to send an async unlock request to the server
when the lock call is interrupted. This is tricky though, since the
process has been signaled and you have to deal with that as well.
--
Jeff Layton <[email protected]>