ocfs2: fix double unlock in case retry after free truncate log
PATCH 1/1 ocfs2: fix double unlock in case retry after free truncate log
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.
truncate log"
--- fs/ocfs2/suballoc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
fs/ocfs2/suballoc.c +11 -2
@@ -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 )
Signature · 1 line
1.8.4.3
PATCH 1/1 ocfs2: fix double unlock in case retry after free truncate log
Hello Joseph,
Thanks for fixing up this.
Quoted 4 lines · Joseph Qi
If
ocfs2_reserve_cluster_bitmap_bitsfails with ENOSPC, it will try to free truncate log and then retry. Sinceocfs2_try_to_free_truncate_logwill 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
Quoted 43 lines
global bitmap inode lock taken, it will unlock again in error handling branch and BUG.
This issue also exists if no need retry and thenocfs2_inode_lockfails. So fix it.Fixes 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space intruncate log"
Signed-off-by Jospeh Qi <[email protected]>Signed-off-by Jiufei Xue <[email protected]>--- fs/ocfs2/suballoc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/suballoc.c +11
-2@@ -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 )
PATCH 1/1 ocfs2: fix double unlock in case retry after free truncate log
Hi Eric,
Quoted 9 lines · Eric Ren
Hello Joseph,
Thanks for fixing up this.
Quoted 4 lines · Joseph Qi
If
ocfs2_reserve_cluster_bitmap_bitsfails with ENOSPC, it will try to free truncate log and then retry. Sinceocfs2_try_to_free_truncate_logwill lock/unlock global bitmap inode, we have to unlock it before calling this function. But when retry reserve and it fails with no
Quoted 5 lines
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
Quoted 48 lines
Quoted 33 lines
global bitmap inode lock taken, it will unlock again in error handling branch and BUG.
This issue also exists if no need retry and thenocfs2_inode_lockfails. So fix it.Fixes 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space intruncate log"
Signed-off-by Jospeh Qi <[email protected]>Signed-off-by Jiufei Xue <[email protected]>--- fs/ocfs2/suballoc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/suballoc.c +4
-2@@ -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 );fs/ocfs2/suballoc.c retry: · line 1221
Quoted 10 lines
fs/ocfs2/suballoc.c +7
-0+ 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 ).
PATCH 1/1 ocfs2: fix double unlock in case retry after free truncate log
Hello Joseph,
Quoted 12 lines · Joseph Qi
Hi Eric,
Quoted 9 lines · Eric Ren
Hello Joseph,
Thanks for fixing up this.
Quoted 4 lines · Joseph Qi
If
ocfs2_reserve_cluster_bitmap_bitsfails with ENOSPC, it will try to free truncate log and then retry. Sinceocfs2_try_to_free_truncate_logwill lock/unlock global bitmap inode, we have to unlock it before calling this function. But when retry reserve and it fails with no
Quoted 8 lines
Quoted 5 lines
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
Quoted 53 lines
Thanks,
JosephQuoted 47 lines
Quoted 33 lines
global bitmap inode lock taken, it will unlock again in error handling branch and BUG.
This issue also exists if no need retry and thenocfs2_inode_lockfails. So fix it.Fixes 2070ad1aebff ("ocfs2: retry on ENOSPC if sufficient space intruncate log"
Signed-off-by Jospeh Qi <[email protected]>Signed-off-by Jiufei Xue <[email protected]>--- fs/ocfs2/suballoc.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)fs/ocfs2/suballoc.c +4
-2@@ -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 );fs/ocfs2/suballoc.c retry: · line 1221
Quoted 10 lines
fs/ocfs2/suballoc.c +7
-0+ 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 ).