Re: [PATCH v3 31/38] nfsd: Move the open owner hash table into struct nfs4_client

[Trond Myklebust <trond.myklebust@primarydata.com>]

On Sat, Aug 2, 2014 at 9:43 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
> On 8/2/2014 21:11, Trond Myklebust wrote:
>> On Sat, Aug 2, 2014 at 6:39 AM, Kinglong Mee <kinglongmee@gmail.com> wrote:
>>> On 7/30/2014 09:34, Jeff Layton wrote:
>>>> From: Trond Myklebust <trond.myklebust@primarydata.com>
>>>>
>>>> Preparation for removing the client_mutex.
>>>>
>>>> Convert the open owner hash table into a per-client table and protect it
>>>> using the nfs4_client->cl_lock spin lock.
>>>>
>>>> Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
>>>> ---
>>>>  fs/nfsd/netns.h     |   1 -
>>>>  fs/nfsd/nfs4state.c | 187 ++++++++++++++++++++++++----------------------------
>>>>  fs/nfsd/state.h     |   1 +
>>>>  3 files changed, 86 insertions(+), 103 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/netns.h b/fs/nfsd/netns.h
>>>> index a71d14413d39..e1f479c162b5 100644
>>>> --- a/fs/nfsd/netns.h
>>>> +++ b/fs/nfsd/netns.h
>>>> @@ -63,7 +63,6 @@ struct nfsd_net {
>>>>       struct rb_root conf_name_tree;
>>>>       struct list_head *unconf_id_hashtbl;
>>>>       struct rb_root unconf_name_tree;
>>>> -     struct list_head *ownerstr_hashtbl;
>>>
>>> I send a patch "NFSD: Rervert "knfsd: locks: flag NFSv4-owned locks"" before,
>>> http://comments.gmane.org/gmane.linux.nfs/64382
>>>
>>> nfsd needs the hashtbl to find the lockowner for locking by owner from
>>> fl->fl_owner stored in struct file_lock, but without nfs_client.
>>
>> Why? We're not currently doing that.
>
> Although not doing that right now, but there is a bug for getting the right ld_owner
> in nfs4_set_lock_denied.
>
> If denying locks, vfs don't copy fl->fl_lmops to the returned file_lock, so that,
> fl->fl_lmops always be NULL, nfsd never return the owner who holds the conflock.
>
> If we want fix this problem, needs finding the lockowner from struct file_lock.

Do we really care enough about fixing nfs4_set_lock_denied enough to
do so at the cost of reducing overall scalability of locking state?
We will always be faking up the clientid etc for local locks. Are
there any clients out there that actually inspect the clientid on a
result of NFS4ERR_DENIED and that will break if we give them a fake
for non-local locks?

-- 
Trond Myklebust

Linux NFS client maintainer, PrimaryData

trond.myklebust@primarydata.com