* [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode @ 2022-06-03 22:31 Junxiao Bi via Ocfs2-devel 2022-06-04 10:10 ` heming.zhao--- via Ocfs2-devel 2022-06-05 13:46 ` Joseph Qi via Ocfs2-devel 0 siblings, 2 replies; 12+ messages in thread From: Junxiao Bi via Ocfs2-devel @ 2022-06-03 22:31 UTC (permalink / raw) To: ocfs2-devel 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@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) _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-03 22:31 [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode Junxiao Bi via Ocfs2-devel @ 2022-06-04 10:10 ` heming.zhao--- via Ocfs2-devel 2022-06-04 22:27 ` Junxiao Bi via Ocfs2-devel 2022-06-05 13:46 ` Joseph Qi via Ocfs2-devel 1 sibling, 1 reply; 12+ messages in thread From: heming.zhao--- via Ocfs2-devel @ 2022-06-04 10:10 UTC (permalink / raw) To: Junxiao Bi, ocfs2-devel 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@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; > } _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-04 10:10 ` heming.zhao--- via Ocfs2-devel @ 2022-06-04 22:27 ` Junxiao Bi via Ocfs2-devel 2022-06-04 23:16 ` heming.zhao--- via Ocfs2-devel 0 siblings, 1 reply; 12+ messages in thread From: Junxiao Bi via Ocfs2-devel @ 2022-06-04 22:27 UTC (permalink / raw) To: heming.zhao, ocfs2-devel On 6/4/22 3:10 AM, heming.zhao@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@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; >> } > _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-04 22:27 ` Junxiao Bi via Ocfs2-devel @ 2022-06-04 23:16 ` heming.zhao--- via Ocfs2-devel 2022-06-05 0:48 ` Junxiao Bi via Ocfs2-devel 0 siblings, 1 reply; 12+ messages in thread From: heming.zhao--- via Ocfs2-devel @ 2022-06-04 23:16 UTC (permalink / raw) To: Junxiao Bi, ocfs2-devel On 6/5/22 06:27, Junxiao Bi wrote: > > On 6/4/22 3:10 AM, heming.zhao@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@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; >>> } >> _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-04 23:16 ` heming.zhao--- via Ocfs2-devel @ 2022-06-05 0:48 ` Junxiao Bi via Ocfs2-devel 2022-06-05 1:12 ` heming.zhao--- via Ocfs2-devel 2022-06-05 1:48 ` Heming Zhao via Ocfs2-devel 0 siblings, 2 replies; 12+ messages in thread From: Junxiao Bi via Ocfs2-devel @ 2022-06-05 0:48 UTC (permalink / raw) To: heming.zhao; +Cc: ocfs2-devel > 在 2022年6月4日，下午4:17，heming.zhao@suse.com 写道： > > On 6/5/22 06:27, Junxiao Bi wrote: >>> On 6/4/22 3:10 AM, heming.zhao@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@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; >>>> } >>> > _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-05 0:48 ` Junxiao Bi via Ocfs2-devel @ 2022-06-05 1:12 ` heming.zhao--- via Ocfs2-devel 2022-06-05 1:48 ` Heming Zhao via Ocfs2-devel 1 sibling, 0 replies; 12+ messages in thread From: heming.zhao--- via Ocfs2-devel @ 2022-06-05 1:12 UTC (permalink / raw) To: Junxiao Bi; +Cc: ocfs2-devel On 6/5/22 08:48, Junxiao Bi wrote: > > >> 在 2022年6月4日，下午4:17，heming.zhao@suse.com 写道： >> >> On 6/5/22 06:27, Junxiao Bi wrote: >>>> On 6/4/22 3:10 AM, heming.zhao@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@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. I can't see why my idea cost more. the patch ('NEW_ERRNO' should be changed) with my idea: diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c index e360543ad7e7..dd47556b07fa 100644 --- a/fs/ocfs2/dlmfs/dlmfs.c +++ b/fs/ocfs2/dlmfs/dlmfs.c @@ -305,7 +305,7 @@ static void dlmfs_evict_inode(struct inode *inode) if (S_ISREG(inode->i_mode)) { status = user_dlm_destroy_lock(&ip->ip_lockres); - if (status < 0) + if (status < 0 && status != NEW_ERRNO) mlog_errno(status); iput(ip->ip_parent); goto clear_fields; diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c index 617c92e7b925..93b8d7bad96e 100644 --- a/fs/ocfs2/dlmfs/userdlm.c +++ b/fs/ocfs2/dlmfs/userdlm.c @@ -600,6 +600,7 @@ int user_dlm_destroy_lock(struct user_lock_res *lockres) spin_lock(&lockres->l_lock); if (lockres->l_flags & USER_LOCK_IN_TEARDOWN) { spin_unlock(&lockres->l_lock); + status = -NEW_ERRNO; goto bail; } your patch added many codes and add another 'if' branch. - the many codes: cpu will spend more time to complete the same work. - the new added 'if' branch will breaks cpu pipeline. my patch only changes 2 lines, maybe add 2 lines cpu instruct without adding new breaking cpu pipeline case. /Heming >> >>>> >>>>> + if (!destroyed) { >>>>> + status = user_dlm_destroy_lock(lockres); >>>>> + if (status < 0) >>>>> + mlog_errno(status); >>>>> + } >>>>> iput(ip->ip_parent); >>>>> goto clear_fields; >>>>> } >>>> >> _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-05 0:48 ` Junxiao Bi via Ocfs2-devel 2022-06-05 1:12 ` heming.zhao--- via Ocfs2-devel @ 2022-06-05 1:48 ` Heming Zhao via Ocfs2-devel 2022-06-05 2:40 ` Junxiao Bi via Ocfs2-devel 1 sibling, 1 reply; 12+ messages in thread From: Heming Zhao via Ocfs2-devel @ 2022-06-05 1:48 UTC (permalink / raw) To: Junxiao Bi; +Cc: ocfs2-devel sorry for my last reply. thunderbird messed up format. I resend my reply with neomutt, please check it. (no new change between the last messed-up mail and this mail.) On Sun, Jun 05, 2022 at 12:48:18AM +0000, Junxiao Bi wrote: > > > > 在 2022年6月4日，下午4:17，heming.zhao@suse.com 写道： > > > > On 6/5/22 06:27, Junxiao Bi wrote: > >>> On 6/4/22 3:10 AM, heming.zhao@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@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. I can't see why my idea cost more. The patch ('NEW_ERRNO' should be changed) with my idea: diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c index e360543ad7e7..dd47556b07fa 100644 --- a/fs/ocfs2/dlmfs/dlmfs.c +++ b/fs/ocfs2/dlmfs/dlmfs.c @@ -305,7 +305,7 @@ static void dlmfs_evict_inode(struct inode *inode) if (S_ISREG(inode->i_mode)) { status = user_dlm_destroy_lock(&ip->ip_lockres); - if (status < 0) + if (status < 0 && status != -NEW_ERRNO) mlog_errno(status); iput(ip->ip_parent); goto clear_fields; diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c index 617c92e7b925..93b8d7bad96e 100644 --- a/fs/ocfs2/dlmfs/userdlm.c +++ b/fs/ocfs2/dlmfs/userdlm.c @@ -600,6 +600,7 @@ int user_dlm_destroy_lock(struct user_lock_res *lockres) spin_lock(&lockres->l_lock); if (lockres->l_flags & USER_LOCK_IN_TEARDOWN) { spin_unlock(&lockres->l_lock); + status = -NEW_ERRNO; goto bail; } your patch added many codes and add another 'if' branch. - the many codes: cpu will spend more time to complete the same work. - the new added 'if' branch will breaks cpu pipeline. my patch only changes 2 lines, maybe add 2 lines cpu instruct without adding new breaking cpu pipeline case. /Heming > >>> > >>>> + if (!destroyed) { > >>>> + status = user_dlm_destroy_lock(lockres); > >>>> + if (status < 0) > >>>> + mlog_errno(status); > >>>> + } > >>>> iput(ip->ip_parent); > >>>> goto clear_fields; > >>>> } > >>> > > _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-05 1:48 ` Heming Zhao via Ocfs2-devel @ 2022-06-05 2:40 ` Junxiao Bi via Ocfs2-devel 2022-06-05 3:45 ` heming.zhao--- via Ocfs2-devel 0 siblings, 1 reply; 12+ messages in thread From: Junxiao Bi via Ocfs2-devel @ 2022-06-05 2:40 UTC (permalink / raw) To: Heming Zhao; +Cc: ocfs2-devel > 在 2022年6月4日，下午6:48，Heming Zhao <heming.zhao@suse.com> 写道： > > sorry for my last reply. thunderbird messed up format. I resend my reply > with neomutt, please check it. > (no new change between the last messed-up mail and this mail.) > >> On Sun, Jun 05, 2022 at 12:48:18AM +0000, Junxiao Bi wrote: >> >> >>>> 在 2022年6月4日，下午4:17，heming.zhao@suse.com 写道： >>> >>> On 6/5/22 06:27, Junxiao Bi wrote: >>>>> On 6/4/22 3:10 AM, heming.zhao@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@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. > > I can't see why my idea cost more. > The patch ('NEW_ERRNO' should be changed) with my idea: > > diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c > index e360543ad7e7..dd47556b07fa 100644 > --- a/fs/ocfs2/dlmfs/dlmfs.c > +++ b/fs/ocfs2/dlmfs/dlmfs.c > @@ -305,7 +305,7 @@ static void dlmfs_evict_inode(struct inode *inode) > > if (S_ISREG(inode->i_mode)) { > status = user_dlm_destroy_lock(&ip->ip_lockres); > - if (status < 0) > + if (status < 0 && status != -NEW_ERRNO) > mlog_errno(status); > iput(ip->ip_parent); > goto clear_fields; > diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c > index 617c92e7b925..93b8d7bad96e 100644 > --- a/fs/ocfs2/dlmfs/userdlm.c > +++ b/fs/ocfs2/dlmfs/userdlm.c > @@ -600,6 +600,7 @@ int user_dlm_destroy_lock(struct user_lock_res > *lockres) > spin_lock(&lockres->l_lock); > if (lockres->l_flags & USER_LOCK_IN_TEARDOWN) { > spin_unlock(&lockres->l_lock); > + status = -NEW_ERRNO; > goto bail; > } > > your patch added many codes and add another 'if' branch. > - the many codes: cpu will spend more time to complete the same work. > - the new added 'if' branch will breaks cpu pipeline. > > my patch only changes 2 lines, maybe add 2 lines cpu instruct without adding > new breaking cpu pipeline case. Your patch invoked user_dlm_destroy_lock, that will execute also the if and spin lock. Plus function call, that’s not higher cost? > > /Heming > >>>>> >>>>>> + if (!destroyed) { >>>>>> + status = user_dlm_destroy_lock(lockres); >>>>>> + if (status < 0) >>>>>> + mlog_errno(status); >>>>>> + } >>>>>> iput(ip->ip_parent); >>>>>> goto clear_fields; >>>>>> } >>>>> >>> > _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-05 2:40 ` Junxiao Bi via Ocfs2-devel @ 2022-06-05 3:45 ` heming.zhao--- via Ocfs2-devel 0 siblings, 0 replies; 12+ messages in thread From: heming.zhao--- via Ocfs2-devel @ 2022-06-05 3:45 UTC (permalink / raw) To: Junxiao Bi; +Cc: ocfs2-devel On 6/5/22 10:40, Junxiao Bi wrote: > >> 在 2022年6月4日，下午6:48，Heming Zhao <heming.zhao@suse.com> 写道： >> >> sorry for my last reply. thunderbird messed up format. I resend my reply >> with neomutt, please check it. >> (no new change between the last messed-up mail and this mail.) >> >>> On Sun, Jun 05, 2022 at 12:48:18AM +0000, Junxiao Bi wrote: >>> >>> >>>>> 在 2022年6月4日，下午4:17，heming.zhao@suse.com 写道： >>>> >>>> On 6/5/22 06:27, Junxiao Bi wrote: >>>>>> On 6/4/22 3:10 AM, heming.zhao@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@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. >> >> I can't see why my idea cost more. >> The patch ('NEW_ERRNO' should be changed) with my idea: >> >> diff --git a/fs/ocfs2/dlmfs/dlmfs.c b/fs/ocfs2/dlmfs/dlmfs.c >> index e360543ad7e7..dd47556b07fa 100644 >> --- a/fs/ocfs2/dlmfs/dlmfs.c >> +++ b/fs/ocfs2/dlmfs/dlmfs.c >> @@ -305,7 +305,7 @@ static void dlmfs_evict_inode(struct inode *inode) >> >> if (S_ISREG(inode->i_mode)) { >> status = user_dlm_destroy_lock(&ip->ip_lockres); >> - if (status < 0) >> + if (status < 0 && status != -NEW_ERRNO) >> mlog_errno(status); >> iput(ip->ip_parent); >> goto clear_fields; >> diff --git a/fs/ocfs2/dlmfs/userdlm.c b/fs/ocfs2/dlmfs/userdlm.c >> index 617c92e7b925..93b8d7bad96e 100644 >> --- a/fs/ocfs2/dlmfs/userdlm.c >> +++ b/fs/ocfs2/dlmfs/userdlm.c >> @@ -600,6 +600,7 @@ int user_dlm_destroy_lock(struct user_lock_res >> *lockres) >> spin_lock(&lockres->l_lock); >> if (lockres->l_flags & USER_LOCK_IN_TEARDOWN) { >> spin_unlock(&lockres->l_lock); >> + status = -NEW_ERRNO; >> goto bail; >> } >> >> your patch added many codes and add another 'if' branch. >> - the many codes: cpu will spend more time to complete the same work. >> - the new added 'if' branch will breaks cpu pipeline. >> >> my patch only changes 2 lines, maybe add 2 lines cpu instruct without adding >> new breaking cpu pipeline case. > Your patch invoked user_dlm_destroy_lock, that will execute also the if and spin lock. Plus function call, that’s not higher cost? Thank you for your explanation, I got your patch meaning. Yes, my idea cost more. Let's wait for maintainer comment. /Heming >> >>>>>> >>>>>>> + if (!destroyed) { >>>>>>> + status = user_dlm_destroy_lock(lockres); >>>>>>> + if (status < 0) >>>>>>> + mlog_errno(status); >>>>>>> + } >>>>>>> iput(ip->ip_parent); >>>>>>> goto clear_fields; >>>>>>> } >>>>>> >>>> >> _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-03 22:31 [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode Junxiao Bi via Ocfs2-devel 2022-06-04 10:10 ` heming.zhao--- via Ocfs2-devel @ 2022-06-05 13:46 ` Joseph Qi via Ocfs2-devel 2022-06-06 15:33 ` Junxiao Bi via Ocfs2-devel 1 sibling, 1 reply; 12+ messages in thread From: Joseph Qi via Ocfs2-devel @ 2022-06-05 13:46 UTC (permalink / raw) To: Junxiao Bi, ocfs2-devel 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@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 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-05 13:46 ` Joseph Qi via Ocfs2-devel @ 2022-06-06 15:33 ` Junxiao Bi via Ocfs2-devel 2022-06-07 1:40 ` Joseph Qi via Ocfs2-devel 0 siblings, 1 reply; 12+ messages in thread From: Junxiao Bi via Ocfs2-devel @ 2022-06-06 15:33 UTC (permalink / raw) To: Joseph Qi, ocfs2-devel 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@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 _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode 2022-06-06 15:33 ` Junxiao Bi via Ocfs2-devel @ 2022-06-07 1:40 ` Joseph Qi via Ocfs2-devel 0 siblings, 0 replies; 12+ messages in thread From: Joseph Qi via Ocfs2-devel @ 2022-06-07 1:40 UTC (permalink / raw) To: Junxiao Bi, ocfs2-devel On 6/6/22 11:33 PM, Junxiao Bi wrote: > 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@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 ? > Yes, teardown would be better. Thanks, Joseph _______________________________________________ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com https://oss.oracle.com/mailman/listinfo/ocfs2-devel ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-06-07 1:40 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-06-03 22:31 [Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode Junxiao Bi via Ocfs2-devel 2022-06-04 10:10 ` heming.zhao--- via Ocfs2-devel 2022-06-04 22:27 ` Junxiao Bi via Ocfs2-devel 2022-06-04 23:16 ` heming.zhao--- via Ocfs2-devel 2022-06-05 0:48 ` Junxiao Bi via Ocfs2-devel 2022-06-05 1:12 ` heming.zhao--- via Ocfs2-devel 2022-06-05 1:48 ` Heming Zhao via Ocfs2-devel 2022-06-05 2:40 ` Junxiao Bi via Ocfs2-devel 2022-06-05 3:45 ` heming.zhao--- via Ocfs2-devel 2022-06-05 13:46 ` Joseph Qi via Ocfs2-devel 2022-06-06 15:33 ` Junxiao Bi via Ocfs2-devel 2022-06-07 1:40 ` Joseph Qi via Ocfs2-devel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.