[Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode

[heming.zhao at suse.com]

On 6/5/22 10:40, Junxiao Bi wrote:
> 
>> 在 2022年6月4日，下午6:48，Heming Zhao <heming.zhao at suse.com> 写道：
>>
>> sorry for my last reply. thunderbird messed up format. I resend my reply
>> with neomutt, please check it.
>> (no new change between the last messed-up mail and this mail.)
>>
>>> On Sun, Jun 05, 2022 at 12:48:18AM +0000, Junxiao Bi wrote:
>>>
>>>
>>>>> 在 2022年6月4日，下午4:17，heming.zhao at suse.com 写道：
>>>>
>>>> On 6/5/22 06:27, Junxiao Bi wrote:
>>>>>> On 6/4/22 3:10 AM, heming.zhao at suse.com wrote:
>>>>>> Hello Junxiao,
>>>>>>
>>>>>> On 6/4/22 06:31, Junxiao Bi via Ocfs2-devel wrote:
>>>>>>> When unlink a dlmfs, first it will invoke dlmfs_unlink(), and then invoke
>>>>>>> dlmfs_evict_inode(), user_dlm_destroy_lock() is invoked in both places,
>>>>>>> the second one from dlmfs_evict_inode() will get EBUSY error because
>>>>>>> USER_LOCK_IN_TEARDOWN is already set in lockres. This doesn't affect
>>>>>>> any function, just the error log is anonying.
>>>>>>>
>>>>>>> Signed-off-by: Junxiao Bi <junxiao.bi at oracle.com>
>>>>>>> ---
>>>>>>>   fs/ocfs2/dlmfs/dlmfs.c | 14 +++++++++++---
>>>>>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
>>>>>>> index e360543ad7e7..a120610dff7e 100644
>>>>>>> --- a/fs/ocfs2/dlmfs/dlmfs.c
>>>>>>> +++ b/fs/ocfs2/dlmfs/dlmfs.c
>>>>>>> @@ -296,17 +296,25 @@ static void dlmfs_evict_inode(struct inode *inode)
>>>>>>>   {
>>>>>>>       int status;
>>>>>>>       struct dlmfs_inode_private *ip;
>>>>>>> +    struct user_lock_res *lockres;
>>>>>>> +    int destroyed;
>>>>>>>         clear_inode(inode);
>>>>>>>         mlog(0, "inode %lu\n", inode->i_ino);
>>>>>>>         ip = DLMFS_I(inode);
>>>>>>> +    lockres = &ip->ip_lockres;
>>>>>>>         if (S_ISREG(inode->i_mode)) {
>>>>>>> -        status = user_dlm_destroy_lock(&ip->ip_lockres);
>>>>>>> -        if (status < 0)
>>>>>>> -            mlog_errno(status);
>>>>>>> +        spin_lock(&lockres->l_lock);
>>>>>>> +        destroyed = !!(lockres->l_flags & USER_LOCK_IN_TEARDOWN);
>>>>>>> +        spin_unlock(&lockres->l_lock);
>>>>>>
>>>>>> This area code does the same work in user_dlm_destroy_lock(). Why not to give another
>>>>>> errno (e.g -EAGAIN) for user_dlm_destroy_lock when l_flags contains USER_LOCK_IN_TEARDOWN.
>>>>>> then change 'if (status < 0)' to 'if (status < 0 && status != -EAGAIN)'
>>>>> I don't think we should do that. It's reasonable for user_dlm_destroy_lock() to return EBUSY in that case. EAGAIN is for the case, you invoke it second time you may succeed. That's not the case here.
>>>>
>>>> I agree the EAGAIN is not good, but do you think the errno idea is reasonable?
>>>> user_dlm_destroy_lock only has two callers: dlmfs_evict_inode & dlmfs_unlink.
>>>> the code logic is clear, we can choose another errno, or even create a new one.
>>>> it costs too much to use spin_lock to avoid print an error log.
>>> Regarding cost, the suggested way has even higher cost, the spin lock can’t be avoided unless you don’t access the lockres and an extra function call was also added.
>>> Actually that function shouldn’t be invoked because it was already invoked once in this flow. I don’t think we should change return value of that function, EBUSY looks most reasonable return value for me.
>>
>> I can't see why my idea cost more.
>> The patch ('NEW_ERRNO' should be changed) with my idea:
>>
>> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c
>> index e360543ad7e7..dd47556b07fa 100644
>> --- a/fs/ocfs2/dlmfs/dlmfs.c
>> +++ b/fs/ocfs2/dlmfs/dlmfs.c
>> @@ -305,7 +305,7 @@ static void dlmfs_evict_inode(struct inode *inode)
>>
>>      if (S_ISREG(inode->i_mode)) {
>>          status = user_dlm_destroy_lock(&ip->ip_lockres);
>> -        if (status < 0)
>> +        if (status < 0 && status != -NEW_ERRNO)
>>              mlog_errno(status);
>>          iput(ip->ip_parent);
>>          goto clear_fields;
>> diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c
>> index 617c92e7b925..93b8d7bad96e 100644
>> --- a/fs/ocfs2/dlmfs/userdlm.c
>> +++ b/fs/ocfs2/dlmfs/userdlm.c
>> @@ -600,6 +600,7 @@ int user_dlm_destroy_lock(struct user_lock_res
>> *lockres)
>>      spin_lock(&lockres->l_lock);
>>      if (lockres->l_flags & USER_LOCK_IN_TEARDOWN) {
>>          spin_unlock(&lockres->l_lock);
>> +        status = -NEW_ERRNO;
>>          goto bail;
>>      }
>>
>> your patch added many codes and add another 'if' branch.
>> - the many codes: cpu will spend more time to complete the same work.
>> - the new added 'if' branch will breaks cpu pipeline.
>>
>> my patch only changes 2 lines, maybe add 2 lines cpu instruct without adding
>> new breaking cpu pipeline case.
> Your patch invoked user_dlm_destroy_lock, that will execute also the if and spin lock. Plus function call, that’s not higher cost?

Thank you for your explanation, I got your patch meaning.
Yes, my idea cost more. Let's wait for maintainer comment.

/Heming

>>
>>>>>>
>>>>>>> +        if (!destroyed) {
>>>>>>> +            status = user_dlm_destroy_lock(lockres);
>>>>>>> +            if (status < 0)
>>>>>>> +                mlog_errno(status);
>>>>>>> +        }
>>>>>>>           iput(ip->ip_parent);
>>>>>>>           goto clear_fields;
>>>>>>>       }
>>>>>>
>>>>
>>