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

[Junxiao Bi]

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 ?

Thanks,

Junxiao.

> Thanks,
> Joseph