Heming Zhao
2022-Jun-05 01:48 UTC
[Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode
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 at suse.com ??? > > > > ?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. > 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; > >>>> } > >>> > >
Junxiao Bi
2022-Jun-05 02:40 UTC
[Ocfs2-devel] [PATCH] ocfs2: kill EBUSY from dlmfs_evict_inode
> ? 2022?6?4????6:48?Heming Zhao <heming.zhao at 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 at suse.com ??? >>> >>> ?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. >> 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; >>>>>> } >>>>> >>> >