Joseph Qi
2022-Jun-05 13:46 UTC
[Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode
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
Junxiao Bi
2022-Jun-06 15:33 UTC
[Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode
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