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

[Joseph Qi]

On 6/6/22 11:33 PM, Junxiao Bi wrote:
> On 6/5/22 6:46 AM, Joseph Qi wrote:
> 
>>
>> On 6/4/22 6:31 AM, Junxiao Bi 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.
>> s/anonying/annoying
>>> 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);
>>> +        if (!destroyed) {
>>> +            status = user_dlm_destroy_lock(lockres);
>>> +            if (status < 0)
>>> +                mlog_errno(status);
>>> +        }
>>>           iput(ip->ip_parent);
>>>           goto clear_fields;
>>>       }
>> As you describes, it firstly invokes unlink and then evict, but strictly
>> speaking, flag USER_LOCK_IN_TEARDOWN doesn't mean 'destroyed', it could
>> be destroying, destroyed, or even unattached.
>>
>> So how about just checking the flag like other places?
>> Something like:
>>
>>     /* Don't destroy lockres twice */
>>     spin_lock(&lockres->l_lock);
>>     (lockres->l_flags & USER_LOCK_IN_TEARDOWN) {
>>         spin_unlock(&lockres->l_lock);
>>         goto skip;
>>     }
>>     spin_unlock(&lockres->l_lock);
>>     status = user_dlm_destroy_lock(lockres);
>>     ...
>> skip:
>>     iput(ip->ip_parent);
>>     ...
> 
> Sound like the concern is about the variable name, how about s/destroyed/tearingdown ?
> 
Yes, teardown would be better.

Thanks,
Joseph