* [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log
@ 2016-09-14 4:15 Joseph Qi
2016-09-14 7:57 ` Eric Ren
0 siblings, 1 reply; 4+ messages in thread
From: Joseph Qi @ 2016-09-14 4:15 UTC (permalink / raw)
To: ocfs2-devel
If ocfs2_reserve_cluster_bitmap_bits fails with ENOSPC, it will try to
free truncate log and then retry. Since ocfs2_try_to_free_truncate_log
will lock/unlock global bitmap inode, we have to unlock it before
calling this function. But when retry reserve and it fails with no
global bitmap inode lock taken, it will unlock again in error handling
branch and BUG.
This issue also exists if no need retry and then ocfs2_inode_lock fails.
So fix it.
Fixes: 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space in
truncate log"
Signed-off-by: Jospeh Qi <joseph.qi@huawei.com>
Signed-off-by: Jiufei Xue <xuejiufei@huawei.com>
---
fs/ocfs2/suballoc.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c
index ea47120..041453b 100644
--- a/fs/ocfs2/suballoc.c
+++ b/fs/ocfs2/suballoc.c
@@ -1199,14 +1199,23 @@ retry:
inode_unlock((*ac)->ac_inode);
ret = ocfs2_try_to_free_truncate_log(osb, bits_wanted);
- if (ret == 1)
+ if (ret == 1) {
+ iput((*ac)->ac_inode);
+ (*ac)->ac_inode = NULL;
goto retry;
+ }
if (ret < 0)
mlog_errno(ret);
inode_lock((*ac)->ac_inode);
- ocfs2_inode_lock((*ac)->ac_inode, NULL, 1);
+ status = ocfs2_inode_lock((*ac)->ac_inode, NULL, 1);
+ if (status < 0) {
+ inode_unlock((*ac)->ac_inode);
+ iput((*ac)->ac_inode);
+ (*ac)->ac_inode = NULL;
+ goto bail;
+ }
}
if (status < 0) {
if (status != -ENOSPC)
--
1.8.4.3
^ permalink raw reply related [flat|nested] 4+ messages in thread* [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log 2016-09-14 4:15 [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log Joseph Qi @ 2016-09-14 7:57 ` Eric Ren 2016-09-14 8:13 ` Joseph Qi 0 siblings, 1 reply; 4+ messages in thread From: Eric Ren @ 2016-09-14 7:57 UTC (permalink / raw) To: ocfs2-devel Hello Joseph, Thanks for fixing up this. On 09/14/2016 12:15 PM, Joseph Qi wrote: > If ocfs2_reserve_cluster_bitmap_bits fails with ENOSPC, it will try to > free truncate log and then retry. Since ocfs2_try_to_free_truncate_log > will lock/unlock global bitmap inode, we have to unlock it before > calling this function. But when retry reserve and it fails with no You mean the retry succeeds by "retry reserve", right? I fail to understand in which situation the retry will fail to get global bitmap inode lock. Because I didn't see this problem when I tested my patch, could you explain a bit more? Eric > global bitmap inode lock taken, it will unlock again in error handling > branch and BUG. > This issue also exists if no need retry and then ocfs2_inode_lock fails. > So fix it. > > Fixes: 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space in > truncate log" > Signed-off-by: Jospeh Qi <joseph.qi@huawei.com> > Signed-off-by: Jiufei Xue <xuejiufei@huawei.com> > --- > fs/ocfs2/suballoc.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c > index ea47120..041453b 100644 > --- a/fs/ocfs2/suballoc.c > +++ b/fs/ocfs2/suballoc.c > @@ -1199,14 +1199,23 @@ retry: > inode_unlock((*ac)->ac_inode); > > ret = ocfs2_try_to_free_truncate_log(osb, bits_wanted); > - if (ret == 1) > + if (ret == 1) { > + iput((*ac)->ac_inode); > + (*ac)->ac_inode = NULL; > goto retry; > + } > > if (ret < 0) > mlog_errno(ret); > > inode_lock((*ac)->ac_inode); > - ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); > + status = ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); > + if (status < 0) { > + inode_unlock((*ac)->ac_inode); > + iput((*ac)->ac_inode); > + (*ac)->ac_inode = NULL; > + goto bail; > + } > } > if (status < 0) { > if (status != -ENOSPC) ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log 2016-09-14 7:57 ` Eric Ren @ 2016-09-14 8:13 ` Joseph Qi 2016-09-18 5:07 ` Eric Ren 0 siblings, 1 reply; 4+ messages in thread From: Joseph Qi @ 2016-09-14 8:13 UTC (permalink / raw) To: ocfs2-devel Hi Eric, On 2016/9/14 15:57, Eric Ren wrote: > Hello Joseph, > > Thanks for fixing up this. > > On 09/14/2016 12:15 PM, Joseph Qi wrote: >> If ocfs2_reserve_cluster_bitmap_bits fails with ENOSPC, it will try to >> free truncate log and then retry. Since ocfs2_try_to_free_truncate_log >> will lock/unlock global bitmap inode, we have to unlock it before >> calling this function. But when retry reserve and it fails with no > You mean the retry succeeds by "retry reserve", right? I fail to understand in which situation > the retry will fail to get global bitmap inode lock. Because I didn't see this problem when I > tested my patch, could you explain a bit more? > > Eric Before retry it has inode unlocked, but ac inode is still valid. And if inode lock fails this time, it will goto bail and do inode unlock again. Thanks, Joseph >> global bitmap inode lock taken, it will unlock again in error handling >> branch and BUG. >> This issue also exists if no need retry and then ocfs2_inode_lock fails. >> So fix it. >> >> Fixes: 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space in >> truncate log" >> Signed-off-by: Jospeh Qi <joseph.qi@huawei.com> >> Signed-off-by: Jiufei Xue <xuejiufei@huawei.com> >> --- >> fs/ocfs2/suballoc.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c >> index ea47120..041453b 100644 >> --- a/fs/ocfs2/suballoc.c >> +++ b/fs/ocfs2/suballoc.c >> @@ -1199,14 +1199,23 @@ retry: >> inode_unlock((*ac)->ac_inode); >> >> ret = ocfs2_try_to_free_truncate_log(osb, bits_wanted); >> - if (ret == 1) >> + if (ret == 1) { >> + iput((*ac)->ac_inode); >> + (*ac)->ac_inode = NULL; >> goto retry; >> + } >> >> if (ret < 0) >> mlog_errno(ret); >> >> inode_lock((*ac)->ac_inode); >> - ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); >> + status = ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); >> + if (status < 0) { >> + inode_unlock((*ac)->ac_inode); >> + iput((*ac)->ac_inode); >> + (*ac)->ac_inode = NULL; >> + goto bail; >> + } >> } >> if (status < 0) { >> if (status != -ENOSPC) > > > > . > ^ permalink raw reply [flat|nested] 4+ messages in thread
* [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log 2016-09-14 8:13 ` Joseph Qi @ 2016-09-18 5:07 ` Eric Ren 0 siblings, 0 replies; 4+ messages in thread From: Eric Ren @ 2016-09-18 5:07 UTC (permalink / raw) To: ocfs2-devel Hello Joseph, On 09/14/2016 04:13 PM, Joseph Qi wrote: > Hi Eric, > > On 2016/9/14 15:57, Eric Ren wrote: >> Hello Joseph, >> >> Thanks for fixing up this. >> >> On 09/14/2016 12:15 PM, Joseph Qi wrote: >>> If ocfs2_reserve_cluster_bitmap_bits fails with ENOSPC, it will try to >>> free truncate log and then retry. Since ocfs2_try_to_free_truncate_log >>> will lock/unlock global bitmap inode, we have to unlock it before >>> calling this function. But when retry reserve and it fails with no >> You mean the retry succeeds by "retry reserve", right? I fail to understand in which situation >> the retry will fail to get global bitmap inode lock. Because I didn't see this problem when I >> tested my patch, could you explain a bit more? >> >> Eric > Before retry it has inode unlocked, but ac inode is still valid. And > if inode lock fails this time, it will goto bail and do inode unlock > again. Yeah, I see this point, thanks. I am also wondering when we will fail to lock the global bitmap inode? BTW, I'm guessing you mean "retry deserves" by "retry reserve"? Eric > > Thanks, > Joseph > >>> global bitmap inode lock taken, it will unlock again in error handling >>> branch and BUG. >>> This issue also exists if no need retry and then ocfs2_inode_lock fails. >>> So fix it. >>> >>> Fixes: 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space in >>> truncate log" >>> Signed-off-by: Jospeh Qi <joseph.qi@huawei.com> >>> Signed-off-by: Jiufei Xue <xuejiufei@huawei.com> >>> --- >>> fs/ocfs2/suballoc.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/ocfs2/suballoc.c b/fs/ocfs2/suballoc.c >>> index ea47120..041453b 100644 >>> --- a/fs/ocfs2/suballoc.c >>> +++ b/fs/ocfs2/suballoc.c >>> @@ -1199,14 +1199,23 @@ retry: >>> inode_unlock((*ac)->ac_inode); >>> >>> ret = ocfs2_try_to_free_truncate_log(osb, bits_wanted); >>> - if (ret == 1) >>> + if (ret == 1) { >>> + iput((*ac)->ac_inode); >>> + (*ac)->ac_inode = NULL; >>> goto retry; >>> + } >>> >>> if (ret < 0) >>> mlog_errno(ret); >>> >>> inode_lock((*ac)->ac_inode); >>> - ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); >>> + status = ocfs2_inode_lock((*ac)->ac_inode, NULL, 1); >>> + if (status < 0) { >>> + inode_unlock((*ac)->ac_inode); >>> + iput((*ac)->ac_inode); >>> + (*ac)->ac_inode = NULL; >>> + goto bail; >>> + } >>> } >>> if (status < 0) { >>> if (status != -ENOSPC) >> >> >> . >> > > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-09-18 5:07 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-14 4:15 [Ocfs2-devel] [PATCH] ocfs2: fix double unlock in case retry after free truncate log Joseph Qi 2016-09-14 7:57 ` Eric Ren 2016-09-14 8:13 ` Joseph Qi 2016-09-18 5:07 ` Eric Ren
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.