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

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.

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

> ? 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.

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