On Thu, 2017-05-18 at 13:36 -0400, Benjamin Coddington wrote: > > On Thu, 2017-05-18 at 12:02 -0400, Benjamin Coddington wrote: > > > diff --git a/fs/locks.c b/fs/locks.c > > > index af2031a1fcff..959b3f93f4bd 100644 > > > --- a/fs/locks.c > > > +++ b/fs/locks.c > > > @@ -249,7 +249,7 @@ locks_dump_ctx_list(struct list_head *list, char > > > *list_type) > > > struct file_lock *fl; > > > > > > list_for_each_entry(fl, list, fl_list) { > > > - pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n", > > > list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, fl->fl_pid); > > > + pr_warn("%s: fl_owner=%p fl_flags=0x%x fl_type=0x%x fl_pid=%u\n", > > > list_type, fl->fl_owner, fl->fl_flags, fl->fl_type, > > > pid_vnr(fl->fl_nspid)); > > > } > > > } > > > > > > > Probably should change the format to say fl_nspid=%u here, just to be > > clear. Might also want to keep fl_pid in there since the lock manager > > could set it. > > Yes, I thought about just spitting out both. Let's do that. > > > > @@ -2074,7 +2075,7 @@ static int posix_lock_to_flock(struct flock > > > *flock, struct file_lock *fl) > > > #if BITS_PER_LONG == 32 > > > static void posix_lock_to_flock64(struct flock64 *flock, struct > > > file_lock *fl) > > > { > > > - flock->l_pid = IS_OFDLCK(fl) ? -1 : fl->fl_pid; > > > + flock->l_pid = IS_OFDLCK(fl) ? -1 : pid_vnr(fl->fl_nspid); > > > > What about the lock managers that _do_ set fl->fl_pid. With nfsv2/3, > > this is always going to give you back the pid of lockd, AFAICT. > > But isn't this really what you want? If a local process wants to know > who > holds a conflicting lock, the fl_pid of a remote system is really pretty > useless. Not only that, but there's no way for the local process to > know > when the pid is local or remote. Better to be consistent and always > return > something that's useful. > The l_pid field in struct flock (and by extension fl_pid) is pretty poorly defined in the spec(s), especially when there is a remote host involved. Programs that rely on it are insane, of course...but Linux has always behaved this way. In the absence of a compelling reason to change it, I think we should keep the behavior in this respect as close as possible to what we have now. > > Do we want to present the pid value that the client sent here instead > > in > > that case? Maybe the lm could set a fl_flag to indicate that the pid > > should be taken directly from fl_pid here? Then you could move the > > above > > logic to a static inline or something. > > > > Alternately, you could add a new lm_present_pid operation to lock > > managers to format the pid as they see fit. > > Either works to solve the problem, but I still think that F_GETLK and > /proc/locks should only return local pids. > > > > @@ -2322,6 +2325,7 @@ int fcntl_getlk64(struct file *filp, unsigned > > > int cmd, struct flock64 __user *l) > > > int error; > > > > > > error = -EFAULT; > > > + file_lock.fl_nspid = get_pid(task_tgid(current)); > > > > Might it be cleaner to create a FILE_LOCK(name) macro that does this > > on > > the stack (like LIST_HEAD()) ? > > Yes, it would. I'll do it. > In some of those places it might not hurt to consider allocating and freeing a file_lock instead. file_lock is not exactly small (208 bytes on my latest build)... > > > @@ -2520,6 +2527,7 @@ locks_remove_flock(struct file *filp, struct > > > file_lock_context *flctx) > > > > > > if (fl.fl_ops && fl.fl_ops->fl_release_private) > > > fl.fl_ops->fl_release_private(&fl); > > > + put_pid(fl.fl_nspid); > > > > I think we only need to take a reference for when the file_lock can > > outlive the current task, so the get/put may not be necessary in these > > functions. > > Right.. of course. I can clean those up. > -- Jeff Layton <jlayton@xxxxxxxxxxxxxxx> -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html