lkml.org 
[lkml]   [2017]   [Jun]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH 2/2] fs/locks: Remove fl_nspid and use fs-specific l_pid for remote locks
Date
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

\
 
 \ /
  Last update: 2017-06-20 18:11    [W:0.086 / U:1.212 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site