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

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.

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;
>> ????? }
>

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;
>>> ????? }
>>