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

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);
+		if (!destroyed) {
+			status = user_dlm_destroy_lock(lockres);
+			if (status < 0)
+				mlog_errno(status);
+		}
 		iput(ip->ip_parent);
 		goto clear_fields;
 	}
-- 
2.24.3 (Apple Git-128)

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)'

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/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);
	...

Thanks,
Joseph