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 - 12:10:13 EST
On 20 Jun 2017, at 10:03, Benjamin Coddington wrote:
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;
}
EXPORT_SYMBOL_GPL(vfs_test_lock);
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.
OK, please disregard this response completely. You're absolutely
correct.
I spent too much time away from this problem and was confused.
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.
...or...
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).
Right, exactly.
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.
But we can do F_GETLK for a remote file with a remote 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
translated.
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.
Yes, I think we ought to add the flag in this patch, but as you suggest
push
the responsibility for setting it out to the filesystems. I'll send one
more version that adds the flag, but doesn't set it in vfs_test_lock(),
and
follow that with a patch for where the flag ought to be set.
Ben