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

[Benjamin Coddington]

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.

> 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).

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
> 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.

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.

Ben