On 8/18/2014 19:54, Jeff Layton wrote:
> On Sun, 17 Aug 2014 21:42:21 +0800
> Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
>
>> On 8/16/2014 21:35, Kinglong Mee wrote:
>>> On 8/15/2014 22:33, Kinglong Mee wrote:
>>>> On 8/15/2014 19:14, Jeff Layton wrote:
>>>>> On Fri, 15 Aug 2014 08:07:12 +0800
>>>>> Kinglong Mee <kinglongmee@xxxxxxxxx> wrote:
>>>>>
>>>>>> Commit d5b9026a67 ([PATCH] knfsd: locks: flag NFSv4-owned locks) using
>>>>>> fl_lmops field in file_lock for checking nfsd4 lockowner.
>>>>>>
>>>>>> But, commit 1a747ee0cc (locks: don't call ->copy_lock methods on return
>>>>>> of conflicting locks) causes the fl_lmops of conflock always be NULL.
>>>>>>
>>>>>> Also, commit 0996905f93 (lockd: posix_test_lock() should not call
>>>>>> locks_copy_lock()) caused the fl_lmops of conflock always be NULL too.
>>>>>>
>>>>>> Make sure copy the private information by fl_copy_lock() in struct
>>>>>> file_lock_operations, merge __locks_copy_lock() to fl_copy_lock().
>>>>>>
>>>>>> v3: Update based on Joe and Jeff's patch.
>>>>>>
>>>>>> Signed-off-by: Kinglong Mee <kinglongmee@xxxxxxxxx>
>>>>>> ---
>>>>>> fs/locks.c | 24 +++++++-----------------
>>>>>> include/linux/fs.h | 6 ------
>>>>>> 2 files changed, 7 insertions(+), 23 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/locks.c b/fs/locks.c
>>>>>> index cb66fb0..fe52abb 100644
>>>>>> --- a/fs/locks.c
>>>>>> +++ b/fs/locks.c
>>>>>> @@ -281,33 +281,23 @@ static void locks_copy_private(struct file_lock *new, struct file_lock *fl)
>>>>>> /*
>>>>>> * Initialize a new lock from an existing file_lock structure.
>>>>>> */
>>>>>> -void __locks_copy_lock(struct file_lock *new, const struct file_lock *fl)
>>>>>> +void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> {
>>>>>> + /* "new" must be a freshly-initialized lock */
>>>>>> + WARN_ON_ONCE(new->fl_ops);
>>>>>> +
>>>>>> new->fl_owner = fl->fl_owner;
>>>>>> new->fl_pid = fl->fl_pid;
>>>>>> - new->fl_file = NULL;
>>>>>> + new->fl_file = fl->fl_file;
>>>>>> new->fl_flags = fl->fl_flags;
>>>>>> new->fl_type = fl->fl_type;
>>>>>> new->fl_start = fl->fl_start;
>>>>>> new->fl_end = fl->fl_end;
>>>>>> new->fl_ops = NULL;
>>>>>> new->fl_lmops = NULL;
>>>>>> -}
>>>>>> -EXPORT_SYMBOL(__locks_copy_lock);
>>>>>> -
>>>>>> -void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> - /* "new" must be a freshly-initialized lock */
>>>>>> - WARN_ON_ONCE(new->fl_ops);
>>>>>> -
>>>>>> - __locks_copy_lock(new, fl);
>>>>>> - new->fl_file = fl->fl_file;
>>>>>> - new->fl_ops = fl->fl_ops;
>>>>>> - new->fl_lmops = fl->fl_lmops;
>>>>>>
>>>>>> locks_copy_private(new, fl);
>>>>>> }
>>>>>> -
>>>>>> EXPORT_SYMBOL(locks_copy_lock);
>>>>>>
>>>>>> static inline int flock_translate_cmd(int cmd) {
>>>>>> @@ -735,7 +725,7 @@ posix_test_lock(struct file *filp, struct file_lock *fl)
>>>>>> break;
>>>>>> }
>>>>>> if (cfl) {
>>>>>> - __locks_copy_lock(fl, cfl);
>>>>>> + locks_copy_lock(fl, cfl);
>>>>>> if (cfl->fl_nspid)
>>>>>> fl->fl_pid = pid_vnr(cfl->fl_nspid);
>>>>>> } else
>>>>>> @@ -941,7 +931,7 @@ static int __posix_lock_file(struct inode *inode, struct file_lock *request, str
>>>>>> if (!posix_locks_conflict(request, fl))
>>>>>> continue;
>>>>>> if (conflock)
>>>>>> - __locks_copy_lock(conflock, fl);
>>>>>> + locks_copy_lock(conflock, fl);
>>>>>> error = -EAGAIN;
>>>>>> if (!(request->fl_flags & FL_SLEEP))
>>>>>> goto out;
>>>>>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>>>>>> index 908af4f..a383a30 100644
>>>>>> --- a/include/linux/fs.h
>>>>>> +++ b/include/linux/fs.h
>>>>>> @@ -966,7 +966,6 @@ void locks_free_lock(struct file_lock *fl);
>>>>>> extern void locks_init_lock(struct file_lock *);
>>>>>> extern struct file_lock * locks_alloc_lock(void);
>>>>>> extern void locks_copy_lock(struct file_lock *, struct file_lock *);
>>>>>> -extern void __locks_copy_lock(struct file_lock *, const struct file_lock *);
>>>>>> extern void locks_remove_posix(struct file *, fl_owner_t);
>>>>>> extern void locks_remove_file(struct file *);
>>>>>> extern void locks_release_private(struct file_lock *);
>>>>>> @@ -1026,11 +1025,6 @@ static inline void locks_init_lock(struct file_lock *fl)
>>>>>> return;
>>>>>> }
>>>>>>
>>>>>> -static inline void __locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> -{
>>>>>> - return;
>>>>>> -}
>>>>>> -
>>>>>> static inline void locks_copy_lock(struct file_lock *new, struct file_lock *fl)
>>>>>> {
>>>>>> return;
>>>>>
>>>>> I'm not sure this is really what you want to do. Calling fl_copy_lock
>>>>> for a conflock looks relatively harmless for nfs and nlm. AFS though
>>>>> seems to add the lock to a list associated with the inode. That seems a
>>>>> little suspicious for a conflock and could be problematic. It may be
>>>>> best to avoid dealing with fl_ops for a conflock.
>>>>>
>>>>> Also in the case of fcntl_getlk, the struct file_lock lives on the
>>>>> stack, and locks_release_private is never called on it. You'll need to
>>>>> audit all of the current callers of __locks_copy_lock to ensure that
>>>>> any resources you end up taking references on when copying the conflock
>>>>> are eventually released.
>>>>
>>>> Sorry for my no further think about it.
>>>> I will check that again next day.
>>>
>>> I think we should not change the logical of coping lock,
>>> leave fl_ops and fl_lmops as private data as right now.
>
> Why not? I think we'd benefit from making conflock creation a more
> distinct operation.
>
>>>
>>> I have plan to,
>>> 1. move fl_owner assign from __locks_copy_lock() to locks_copy_private(),
>>> I think it should be a private data, am I right?
>>> 2. call locks_copy_private() coping private data specifically.
>>> a. add an argument for posix_test_lock() and __posix_lock_file() and etc,
>>> to point whether coping private data.
>>> b. hack the conflock's fl_flags to do the same thing as a,
>>> adds FL_NEED_PRIV fl_flags only valid for conflock.
>>>
>>> I don't think 2.a is a nice resolve, because it changes the interface
>>> and many caller don't care the private data (I think contains fl_owner)
>>> for conflock except nfsd.
>>>
>>> So, I'd like *2.b*. A draft is appended as following,
>>
>> I'm so sorry for the first draft, there is a bug of it.
>> Please using the new draft.
>>
>> thanks,
>> Kinglong Mee
>>
>
> Personally, I'm not a fan of the approach below. I don't think we need
> a new flag for this and it doesn't do anything to solve the problem
> where the lockowner could go away while you're operating on it in a
> conflock.
I just want fix the bug with minimal code change.
But, I known my fault now.
>
> I think we need several smaller changes:
>
> 1. Right now __locks_copy_lock is only used to copy conflocks. It would
> be good to rename that to something more distinct (i.e.
> locks_copy_conflock), to make it clear that we're generating a conflock
> there.
>
> 2. Set fl_lmops on conflocks, but don't set fl_ops. fl_ops are
> superfluous, since they are callbacks into the filesystem. There should
> be no need to bother the filesystem at all with info in a conflock.
> But, lock _ownership_ matters for conflocks and that's indicated by the
> fl_lmops. So you really do want to copy the fl_lmops for conflocks I
> think.
>
> 3. Add the operations you added before to fl_lmops to copy and release
> the owner (maybe even rename them lm_get_owner/lm_put_owner?), and
> ensure that the places that copy conflocks call those operations
> appropriately.
>
> That should be all that's required here.
I think I know what you are advice.
I will send an update patch as v4.
thanks,
Kinglong Mee
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
