On 19 May 2017, at 8:35, Benjamin Coddington wrote:
> On 18 May 2017, at 16:41, Jeff Layton wrote:
>
>> 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.
>
> But if it is completely useless today, then we can change it without
> worrying about breaking it because it is already broken. There's no
> documentation anywhere that informs users of F_GETLK or /proc/locks that
> l_pid is completely unreliable.
>
> Do you know why linux hasn't picked up l_sysid? I can't seem to find any
> previous discussion about it.
>
>> 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.
>
> I think the reason would be l_pid should at least have some consistent
> meaning. I think now that this patch probably shouldn't change it, but it
> should be changed.
>
>>>> 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.
It turns out that the lm_present_pid approach is not sufficient and we
should instead use a flag, since the non-lock-manager fs/lockd/clntproc.c
wants to use fl->fl_pid = 0 in the case where the client is testing and
finds a conflicting lock.
So, the client considers remote pids to be bogus, which makes a lot of sense
to me.
Additionally, after testing, today's kernel returns lockd's pid on a local
F_GETLCK for a remotely-held NFS lock. So, I think our understanding of the
situation needs to be reversed. Lock manager's locks are locally reporting
the local lock pid, but sometimes a remote lock needs to override the local
pid to set fl_pid.
Ben
