Skipped unlocks

We only need to check lock exclusive/shared types against open mode when flock() is used on NFS, so move it into the flock-specific path instead of checking it for all locks.

Signed-off-by Benjamin Coddington <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

---
 fs/nfs/file.c     | 18 ++++++++++++++++--
 fs/nfs/nfs4proc.c | 14 --------------
 2 files changed, 16 insertions(+), 16 deletions(-)

fs/nfs/file.c +16 -2

index 26dbe8b0c10d..a490f45df4db 100644
@@ -820,9 +820,23 @@ int nfs_flock(struct file *filp, int cmd, struct file_lock *fl)
 	if (NFS_SERVER(inode)->flags & NFS_MOUNT_LOCAL_FLOCK)
 		is_local = 1;
 
-	/* We're simulating flock() locks using posix locks on the server */
-	if (fl->fl_type == F_UNLCK)
+	/*
+	 * VFS doesn't require the open mode to match a flock() lock's type.
+	 * NFS, however, may simulate flock() locking with posix locking which
+	 * requires the open mode to match the lock type.
+	 */
+	switch (fl->fl_type) {
+	case F_UNLCK:
 		return do_unlk(filp, cmd, fl, is_local);
+	case F_RDLCK:
+		if (!(filp->f_mode & FMODE_READ))
+			return -EBADF;
+		break;
+	case F_WRLCK:
+		if (!(filp->f_mode & FMODE_WRITE))
+			return -EBADF;
+	}
+
 	return do_setlk(filp, cmd, fl, is_local);
 }
 EXPORT_SYMBOL_GPL(nfs_flock);

fs/nfs/nfs4proc.c +0 -7

index 9388899e4050..91f88bfbbe79 100644
@@ -6592,20 +6592,6 @@ nfs4_proc_lock(struct file *filp, int cmd, struct file_lock *request)
 	    !test_bit(NFS_STATE_POSIX_LOCKS, &state->flags))
 		return -ENOLCK;
 
-	/*
-	 * Don't rely on the VFS having checked the file open mode,
-	 * since it won't do this for flock() locks.
-	 */
-	switch (request->fl_type) {
-	case F_RDLCK:
-		if (!(filp->f_mode & FMODE_READ))

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks. NFS will check for this flag to ensure an unlock is sent in a following patch.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/locks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/locks.c +0 -0

index 26811321d39b..af2031a1fcff 100644

Quoted 14 lines · Benjamin Coddington

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks. NFS will check for this flag to ensure an unlock is sent in a following patch.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/locks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/locks.c +0 -0

index 26811321d39b..af2031a1fcff 100644

fs/locks.c locks_remove_flock(struct file *filp, struct file_lock_context *flctx) · line 2510

Quoted 8 lines

fs/locks.c +1 -1

@@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
 		.fl_owner = filp,
 		.fl_pid = current->tgid,
 		.fl_file = filp,
-		.fl_flags = FL_FLOCK,
+		.fl_flags = FL_FLOCK | FL_CLOSE,
 		.fl_type = F_UNLCK,
 		.fl_end = OFFSET_MAX,
 	};

Looks good. I'm fine with merging this one via the NFS tree, btw...

Reviewed-by Jeff Layton <[email protected]>

NFS attempts to wait for read and write completion before unlocking in order to ensure that the data returned was protected by the lock. When this waiting is interrupted by a signal, the unlock may be skipped, and messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting locks indefinitely. For NFSv4, the leftover lock will be removed by the server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/file.c +0 -0

index a490f45df4db..df695f52bb9d 100644

fs/nfs/file.c +0 -4

 	}
 
-	/* NOTE: special case
-	 * 	If we're signalled while cleaning up locks on process exit, we
-	 * 	still need to complete the unlock.
-	 */
 	/*
 	 * Use local locking if mounted with "-onolock" or with appropriate
 	 * "-olocal_lock="

Quoted 26 lines · Jeff Layton

Quoted 14 lines · Benjamin Coddington

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks. NFS will check for this flag to ensure an unlock is sent in a following patch.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/locks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/locks.c +0 -0

index 26811321d39b..af2031a1fcff 100644

fs/locks.c locks_remove_flock(struct file *filp, struct file_lock_context *flctx) · line 2510

Quoted 8 lines

fs/locks.c +1 -1

@@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct file_lock_context *flctx)
            .fl_owner = filp,
            .fl_pid = current->tgid,
            .fl_file = filp,
-           .fl_flags = FL_FLOCK,
+           .fl_flags = FL_FLOCK | FL_CLOSE,
            .fl_type = F_UNLCK,
            .fl_end = OFFSET_MAX,
    };

Looks good. I'm fine with merging this one via the NFS tree, btw...

Quoted 2 lines

Reviewed-by Jeff Layton <[email protected]>

(cc'ing linux-fsdevel and Miklos)

Hmm, that said, this potentially may affect other filesystems. If you do any more postings of this set, please cc linux-fsdevel.

In particular, I'll note that fuse has this:

        /* Unlock on close is handled by the flush method */
        if (fl->fl_flags & FL_CLOSE)
                return 0;

I don't see any lock handling in fuse_flush (though it does pass down a lockowner). I guess the expectation is that the userland driver should close down POSIX locks when the flush method is called.

Miklos, does this also apply to flock locks? Would Ben's patch cause any grief here?

Quoted 38 lines · Jeff Layton

Quoted 27 lines · Jeff Layton

Quoted 22 lines · Benjamin Coddington

Set FL_CLOSE in fl_flags as in locks_remove_posix() when clearing locks. NFS will check for this flag to ensure an unlock is sent in a following patch.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/locks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/locks.c +1 -1

index 26811321d39b..af2031a1fcff 100644
@@ -2504,7 +2504,7 @@ locks_remove_flock(struct file *filp, struct 
 file_lock_context *flctx)
            .fl_owner = filp,
            .fl_pid = current->tgid,
            .fl_file = filp,
-           .fl_flags = FL_FLOCK,
+           .fl_flags = FL_FLOCK | FL_CLOSE,
            .fl_type = F_UNLCK,
            .fl_end = OFFSET_MAX,

Quoted 1 line

    };

Looks good. I'm fine with merging this one via the NFS tree, btw...

Quoted 2 lines

Reviewed-by Jeff Layton <[email protected]>

(cc'ing linux-fsdevel and Miklos)

Hmm, that said, this potentially may affect other filesystems. If you do any more postings of this set, please cc linux-fsdevel.

In particular, I'll note that fuse has this:

Quoted 7 lines

        /* Unlock on close is handled by the flush method */
        if (fl->fl_flags & FL_CLOSE)
                return 0;

I don't see any lock handling in fuse_flush (though it does pass down a lockowner). I guess the expectation is that the userland driver should close down POSIX locks when the flush method is called.

Right.

Quoted 3 lines

Miklos, does this also apply to flock locks? Would Ben's patch cause any grief here?

Yes, it would make the final close of file not unlock flocks on that open file.

Simple fix is to change that condition to

#define FL_CLOSE_POSIX (FL_POSIX | FL_CLOSE)

        if (fl->fl_flags & FL_CLOSE_POSIX == FL_CLOSE_POSIX)
                return 0;

Quoted 27 lines · Benjamin Coddington

NFS attempts to wait for read and write completion before unlocking in order to ensure that the data returned was protected by the lock. When this waiting is interrupted by a signal, the unlock may be skipped, and messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting locks indefinitely. For NFSv4, the leftover lock will be removed by the server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/file.c +0 -0

index a490f45df4db..df695f52bb9d 100644
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct 
 file_lock *fl, int is_local)

fs/nfs/file.c do_unlk(struct file *filp, int cmd, struct · line 706

Quoted 10 lines

fs/nfs/file.c +5 -1

 	if (!IS_ERR(l_ctx)) {
 		status = nfs_iocounter_wait(l_ctx);
 		nfs_put_lock_context(l_ctx);
-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning up locks on process exit, we
+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
 			return status;

Hmm, I don't know if this is safe...

Suppose we have a bunch of buffered writeback going on, and we're sitting here waiting for it so we can do the unlock. The task catches a signal, and then issues the unlock while writeback is still going on. Another client then grabs the lock, and starts doing reads and writes while this one is still writing back.

I think the unlock really needs to wait until writeback is done, regardless of whether you catch a signal or not.

Quoted 57 lines · Jeff Layton

Quoted 35 lines · Benjamin Coddington

NFS attempts to wait for read and write completion before unlocking in
order to ensure that the data returned was protected by the lock. When
this waiting is interrupted by a signal, the unlock may be skipped, and
messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1 fl_type=0x0 
fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1 fl_type=0x0 

fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting
locks indefinitely. For NFSv4, the leftover lock will be removed by the
server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send
the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/file.c +0 -0

index a490f45df4db..df695f52bb9d 100644
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd, struct 
 file_lock *fl, int is_local)

fs/nfs/file.c do_unlk(struct file *filp, int cmd, struct · line 706

Quoted 10 lines

fs/nfs/file.c +5 -1

 	if (!IS_ERR(l_ctx)) {
 		status = nfs_iocounter_wait(l_ctx);
 		nfs_put_lock_context(l_ctx);
-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning up locks on process exit, we
+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))
 			return status;

Hmm, I don't know if this is safe...

Suppose we have a bunch of buffered writeback going on, and we're sitting here waiting for it so we can do the unlock. The task catches a
signal, and then issues the unlock while writeback is still going on. Another client then grabs the lock, and starts doing reads and writes while this one is still writing back.

Quoted 2 lines

I think the unlock really needs to wait until writeback is done, regardless of whether you catch a signal or not.

The only other way to do this is as you've sugggested many times -- by putting the LOCKU on a rpc_wait_queue. But that won't work for NFSv3.

For NFSv4, the other thing to do might be to look up the lock state and set
NFS_LOCK_LOST, then unlock. That will cause subsequent writes to not attempt recovery.

How can this be fixed for NFSv3 without voilating the NLM layers?

Quoted 68 lines · Jeff Layton

Quoted 67 lines · Benjamin Coddington

Quoted 65 lines · Jeff Layton

Quoted 53 lines · Benjamin Coddington

NFS attempts to wait for read and write completion before unlocking
in
order to ensure that the data returned was protected by the lock.
When
this waiting is interrupted by a signal, the unlock may be skipped,
and
messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1
fl_type=0x0 
fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1

fl_type=0x0
fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting
locks indefinitely.  For NFSv4, the leftover lock will be removed by
the
server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send
the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c | 10 +++++-----

 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/file.c +0 -0

index a490f45df4db..df695f52bb9d 100644
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd,

struct
file_lock *fl, int is_local)
  if (!IS_ERR(l_ctx)) {
  status = nfs_iocounter_wait(l_ctx); nfs_put_lock_context(l_ctx);

fs/nfs/file.c +2 -1

-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning

up locks on process exit, we

fs/nfs/file.c +3 -0

+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))

Quoted 1 line

  return status;

Hmm, I don't know if this is safe...

Suppose we have a bunch of buffered writeback going on, and we're sitting here waiting for it so we can do the unlock. The task catches a signal, and then issues the unlock while writeback is still going on. Another client then grabs the lock, and starts doing reads and writes while this one is still writing back.

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

I think the unlock really needs to wait until writeback is done, regardless of whether you catch a signal or not.

The only other way to do this is as you've sugggested many times -- by putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.

IDGI -- why doesn't that (or something similar) not work for v3?

Quoted 8 lines

Quoted 4 lines

For NFSv4, the other thing to do might be to look up the lock state and set NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to not attempt recovery.

No! LOCK_LOST is really for the case where we've lost contact with the server. We don't want to get kicked into recovery just because of a SIGKILL.

I suggest we add a new lock state: NFS_LOCK_UNLOCKED to allow the client to recognise that the failed operation should not be retried.

In fact, we might want to have nfs4_set_rw_stateid() scan for that, and return an error that could be made to fail the operation. Furthermore, maybe __nfs4_find_lock_state() should also ignore lock structures in this state so that once the lock is in this state, we always ignore new attempts. This could also be made to work with pNFS; although for most layout types, the locking will not be enforced by the server, we can at least prevent new I/O requests from being generated after the NFS_LOCK_UNLOCKED state has been declared.

You can't do any of the above with NFSv3, since the locking is decoupled from the actual I/O. Is anyone actually using NLM for mission critical applications that need this kind of protection? (followup question: is there a good reason why they are doing so instead of using NFSv4?)

Quoted 69 lines · Trond Myklebust

Quoted 68 lines · Jeff Layton

Quoted 67 lines · Benjamin Coddington

Quoted 65 lines · Jeff Layton

Quoted 53 lines · Benjamin Coddington

NFS attempts to wait for read and write completion before unlocking
in
order to ensure that the data returned was protected by the lock.
When
this waiting is interrupted by a signal, the unlock may be skipped,
and
messages similar to the following are seen in the kernel ring buffer:

[20.167876] Leaked locks on dev=0x0:0x2b ino=0x8dd4c3:
[20.168286] POSIX: fl_owner=ffff880078b06940 fl_flags=0x1
fl_type=0x0 
fl_pid=20183
[20.168727] POSIX: fl_owner=ffff880078b06680 fl_flags=0x1

fl_type=0x0
fl_pid=20185

For NFSv3, the missing unlock will cause the server to refuse conflicting
locks indefinitely.  For NFSv4, the leftover lock will be removed by
the
server after the lease timeout.

This patch fixes this issue by skipping the wait in order to immediately send
the unlock if the FL_CLOSE flag is set when signaled.

Signed-off-by Benjamin Coddington <[email protected]>

---
 fs/nfs/file.c | 10 +++++-----

 1 file changed, 5 insertions(+), 5 deletions(-)

fs/nfs/file.c +0 -0

index a490f45df4db..df695f52bb9d 100644
@@ -697,14 +697,14 @@ do_unlk(struct file *filp, int cmd,

struct
file_lock *fl, int is_local)
  if (!IS_ERR(l_ctx)) {
  status = nfs_iocounter_wait(l_ctx); nfs_put_lock_context(l_ctx);

fs/nfs/file.c +2 -1

-		if (status < 0)
+		/*  NOTE: special case
+		 * 	If we're signalled while cleaning

up locks on process exit, we

fs/nfs/file.c +3 -0

+		 * 	still need to complete the unlock.
+		 */
+		if (status < 0 && !(fl->fl_flags & FL_CLOSE))

Quoted 1 line

  return status;

Hmm, I don't know if this is safe...

Suppose we have a bunch of buffered writeback going on, and we're sitting here waiting for it so we can do the unlock. The task catches a signal, and then issues the unlock while writeback is still going on. Another client then grabs the lock, and starts doing reads and writes while this one is still writing back.

Quoted 8 lines

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

I think the unlock really needs to wait until writeback is done, regardless of whether you catch a signal or not.

The only other way to do this is as you've sugggested many times -- by putting the LOCKU on a rpc_wait_queue.  But that won't work for NFSv3.

IDGI -- why doesn't that (or something similar) not work for v3?

Because of the layering, as discussed further on.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

For NFSv4, the other thing to do might be to look up the lock state and set NFS_LOCK_LOST, then unlock.  That will cause subsequent writes to not attempt recovery.

No! LOCK_LOST is really for the case where we've lost contact with the server. We don't want to get kicked into recovery just because of a SIGKILL.

True, it was added for the case where contact is lost, but it does exactly what we'd want.

If we're coming through do_unlk() with FL_CLOSE, then we've just done a vfs_fsync() and I believe that all dirty data are now in requests queued up and being transmitted. The signal has had us bail out of the wait for those pages to writeback, and I don't think new pages can be dirtied under this lock. So, an unlock at this point will at worst cause some of that I/O to fail with BAD_STATEID (or OLD_STATEID?). What we want to prevent then is the client retrying those writes under another stateid, which is what NFS_LOCK_LOST does.

Quoted 16 lines

I suggest we add a new lock state: NFS_LOCK_UNLOCKED to allow the client to recognise that the failed operation should not be retried.

In fact, we might want to have nfs4_set_rw_stateid() scan for that, and return an error that could be made to fail the operation. Furthermore, maybe __nfs4_find_lock_state() should also ignore lock structures in this state so that once the lock is in this state, we always ignore new attempts. This could also be made to work with pNFS; although for most layout types, the locking will not be enforced by the server, we can at least prevent new I/O requests from being generated after the NFS_LOCK_UNLOCKED state has been declared.

You can't do any of the above with NFSv3, since the locking is decoupled from the actual I/O. Is anyone actually using NLM for mission critical applications that need this kind of protection? (followup question: is there a good reason why they are doing so instead of using NFSv4?)

Yes, people are using it. And I don't think those people care much about whether or not their I/O completes under the lock after a signal. They care that when their job on a cluster gets hung up and their scheduler kills it, they can't submit the job again anywhere else on the cluster because the server thinks the job's file is locked. The only fix is to restart the server, which is very disruptive.

So, I'm happy to work on a NFS_LOCK_UNLOCKED approach for v4 and I'd also be happy to just leave v3 to always unlock.

Well, that's where their immediate pain point is. If they have applications that are relying on NLM locking to serialize access to the files, then this could potentially corrupt their file data.

It may be that no one would ever notice, but those sorts of bug reports can be very difficult to track down. If we can fix this in a way that works across versions, then that's what I'd suggest.