Re: [PATCH 2/2] fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks

From: Benjamin Coddington
Date: Tue Jun 20 2017 - 10:03:51 EST

On 19 Jun 2017, at 13:32, Jeff Layton wrote:

On Mon, 2017-06-19 at 09:24 -0400, Benjamin Coddington wrote:
@@ -2041,16 +2034,46 @@ SYSCALL_DEFINE2(flock, unsigned int, fd, unsigned int, cmd)
int vfs_test_lock(struct file *filp, struct file_lock *fl)
- if (filp->f_op->lock && is_remote_lock(filp))
+ if (filp->f_op->lock && is_remote_lock(filp)) {
+ fl->fl_flags |= FL_PID_PRIV;
return filp->f_op->lock(filp, F_GETLK, fl);
+ }
posix_test_lock(filp, fl);
return 0;

I think this looks wrong for NFS.

Oh yes, this is completely wrong.. It should be looking for fl_ops, which
would set the flag for lock managers.

There are really two cases we're concerned with here:

1) the lock is held by a task on the client itself, in which case we
probably want to report the pid as we would on a local fs.


2) the lock is held by another host entirely in which case the pid
doesn't have any meaning. We probably ought to return something like '-
1' as the pid (like we would for OFD locks).

I don't think we have f_op->lock() users that only set remote locks. For
NFS, the remote lock is always matched by a local lock.

The problem for NFS is that you're setting the flag unconditionally
there. It may very well be the case that we _want_ to translate the
fl_pid according to the local namespace (i.e. if the lock is held by a
task on the same host).

I think what you want to do here is have the fs ->lock operation set
that flag if the fl_pid should be used "as-is" instead of being

Most of the current lock operations can just set it early (to preserve
the existing behavior), but NFS could be set up to set that flag if the
lock request goes to the server.

I think this is just a mistake.. I think we want to always translate all
local locks, unless the lock is placed by a lock manager.

I'll send a corrected version.