Fix waiting status race condition in dlm recovery V2
PATCH 1/1 Fix waiting status race condition in dlm recovery V2
In reply to a message not in the archiveQuoted 19 lines
From: "Xiaowei.Hu" <[email protected]>
when the master requested locks ,but one/some of the live nodes died, after it received the request msg and before send out the locks packages, the recovery will fall into endless loop,waiting for the status changed to finalize
NodeA NodeB
selected as recovery masterdlm_remaster_locks
->dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue workerdlm_request_all_locks_workerreturn 0
go on set state to requested
wait for the state become done
NodeB lost connection due to network before the worker begin, or it die.
Quoted 8 lines
NodeA still waiting for the change of reco state.
It won't end if it not get data done msg.
And at this time nodeB do not realize this (or it just died), it won't send the msg for ever, nodeA left in the recovery process forever.This patch let the recovery master check if the node still in live node map when it stay in REQUESTED status.
Hi, xiaowei,
We have reviewed this patch and have some questions:
1) in dlm_is_node_in_livemap(), I think it should use !!(test_bit(node, dlm->live_nodes_map)) to determine whether a node is live;
2) why not use dlm_is_node_dead() instead of dlm_is_node_in_livemap() in dlm_remaster_locks?
I think dlm_is_node_dead() is better because dlm->live_nodes_map may be remain when another node umount.
Thanks.
Quoted 30 lines
Signed-off-by Xiaowei.Hu <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +11
-0@@ -339,6 +339,17 @@ static int dlm_reco_master_ready(struct dlm_ctxt *dlm)return ready; } +/* returns true if node is still in the live node map +* this map is cleared before domain map,could be checked in recovery*/ +int dlm_is_node_in_livemap (struct dlm_ctxt *dlm,u8 node) +{ +int live; +spin_lock (&dlm->spinlock); + live = !test_bit (node, dlm->live_nodes_map); +spin_unlock (&dlm->spinlock); +return live; +} +/* returns true if node is no longer in the domain * could be dead or just not joined */ int dlm_is_node_dead (struct dlm_ctxt *dlm,u8 node)@@ -679,7 +690,10 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) dlm->name, ndata->node_num,
fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 699
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +4
-1ndata->state==DLM_RECO_NODE_DATA_RECEIVING ? "receiving" : "requested"); - all_nodes_done =0 ; +if (!dlm_is_node_in_livemap (dlm, ndata->node_num)) + ndata->state =DLM_RECO_NODE_DATA_DEAD ; +else + all_nodes_done =0 ;break ;case DLM_RECO_NODE_DATA_DONE : mlog(0, "%s: node %u state is done\n",
.
PATCH 1/1 Fix waiting status race condition in dlm recovery V2
Quoted 28 lines · Xue jiufei
Quoted 19 lines
From: "Xiaowei.Hu" <[email protected]>
when the master requested locks ,but one/some of the live nodes died, after it received the request msg and before send out the locks packages, the recovery will fall into endless loop,waiting for the status changed to finalize
NodeA NodeB
selected as recovery masterdlm_remaster_locks
->dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue workerdlm_request_all_locks_workerreturn 0
go on set state to requested
wait for the state become done
NodeB lost connection due to network before the worker begin, or it die.Quoted 8 lines
NodeA still waiting for the change of reco state.
It won't end if it not get data done msg.
And at this time nodeB do not realize this (or it just died), it won't send the msg for ever, nodeA left in the recovery process forever.This patch let the recovery master check if the node still in live node map when it stay in REQUESTED status.
Quoted 5 lines
Hi, xiaowei,
We have reviewed this patch and have some questions:
1) indlm_is_node_in_livemap(), I think it should use !!(test_bit(node,dlm->live_nodes_map)) to determine whether a node is live;
Hmm? test_bit(node,...) is ok.
Quoted 4 lines
2) why not use
dlm_is_node_dead()instead ofdlm_is_node_in_livemap()indlm_remaster_locks?
I thinkdlm_is_node_dead()is better becausedlm->live_nodes_mapmay be remain when another node umount.
Please refer to:
http://marc.info/?l=ocfs2-devel&m=133799814717270&w=2
Thanks,
-Jeff
Quoted 57 lines
Thanks.
Quoted 30 lines
Signed-off-by Xiaowei.Hu <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +11
-0@@ -339,6 +339,17 @@ static int dlm_reco_master_ready(struct dlm_ctxt *dlm)return ready; } +/* returns true if node is still in the live node map +* this map is cleared before domain map,could be checked in recovery*/ +int dlm_is_node_in_livemap (struct dlm_ctxt *dlm,u8 node) +{ +int live; +spin_lock (&dlm->spinlock); + live = !test_bit (node, dlm->live_nodes_map); +spin_unlock (&dlm->spinlock); +return live; +} +/* returns true if node is no longer in the domain * could be dead or just not joined */ int dlm_is_node_dead (struct dlm_ctxt *dlm,u8 node)@@ -679,7 +690,10 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) dlm->name, ndata->node_num,fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 699
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +4
-1ndata->state==DLM_RECO_NODE_DATA_RECEIVING ? "receiving" : "requested"); - all_nodes_done =0 ; +if (!dlm_is_node_in_livemap (dlm, ndata->node_num)) + ndata->state =DLM_RECO_NODE_DATA_DEAD ; +else + all_nodes_done =0 ;break ;case DLM_RECO_NODE_DATA_DONE : mlog(0, "%s: node %u state is done\n",.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 Fix waiting status race condition in dlm recovery V2
Quoted 29 lines · Jeff Liu
Quoted 27 lines · Xue jiufei
Quoted 27 lines
From: "Xiaowei.Hu" <[email protected]>
when the master requested locks ,but one/some of the live nodes died, after it received the request msg and before send out the locks packages, the recovery will fall into endless loop,waiting for the status changed to finalize
NodeA NodeB
selected as recovery masterdlm_remaster_locks
->dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue workerdlm_request_all_locks_workerreturn 0
go on set state to requested
wait for the state become done
NodeB lost connection due to network before the worker begin, or it die.NodeA still waiting for the change of reco state.
It won't end if it not get data done msg.
And at this time nodeB do not realize this (or it just died), it won't send the msg for ever, nodeA left in the recovery process forever.This patch let the recovery master check if the node still in live node map when it stay in REQUESTED status.
Quoted 11 lines
Quoted 5 lines
Hi, xiaowei,
We have reviewed this patch and have some questions:
1) indlm_is_node_in_livemap(), I think it should use !!(test_bit(node,dlm->live_nodes_map)) to determine whether a node is live;Hmm?
test_bit(node,...)is ok.Quoted 4 lines
2) why not use
dlm_is_node_dead()instead ofdlm_is_node_in_livemap()indlm_remaster_locks?
I thinkdlm_is_node_dead()is better becausedlm->live_nodes_mapmay be remain when another node umount.
For this question, I remember I explained in one of the emails, paste you that contant:
=============
Hi Sunil,
I considered your suggestion about this patch, it's possible to change the status in dlm hb down event,
but what need to change are the dlm_reco_node_data structures in dlm->reco.node_data list.
This list is initialized in dlm_remaster_locks when it begins the lock remaster and destroied before exit this function.
So it's not proper to check data in such a list from dlm hb down event, am I right?
If change the status from dlm hb down event , that means we make the recovery thread rely on more information from the hb down event, actually the dlm->live_nodes_map is marked in this event , and for others to check , right?
This race condition only happen when cluster already in recovery and a node dead during recovery. the recovery thread blocked the update of dlm->domain_map, so I fallback to check the live_nodes_map, which won't be blocked.
Please reconsider this patch.
================
Quoted 62 lines
Please refer to:
http://marc.info/?l=ocfs2-devel&m=133799814717270&w=2Thanks,
-JeffQuoted 55 lines
Thanks.
Quoted 30 lines
Signed-off-by Xiaowei.Hu <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +11
-0@@ -339,6 +339,17 @@ static int dlm_reco_master_ready(struct dlm_ctxt *dlm)return ready; } +/* returns true if node is still in the live node map +* this map is cleared before domain map,could be checked in recovery*/ +int dlm_is_node_in_livemap (struct dlm_ctxt *dlm,u8 node) +{ +int live; +spin_lock (&dlm->spinlock); + live = !test_bit (node, dlm->live_nodes_map); +spin_unlock (&dlm->spinlock); +return live; +} +/* returns true if node is no longer in the domain * could be dead or just not joined */ int dlm_is_node_dead (struct dlm_ctxt *dlm,u8 node)@@ -679,7 +690,10 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) dlm->name, ndata->node_num,fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 699
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +4
-1ndata->state==DLM_RECO_NODE_DATA_RECEIVING ? "receiving" : "requested"); - all_nodes_done =0 ; +if (!dlm_is_node_in_livemap (dlm, ndata->node_num)) + ndata->state =DLM_RECO_NODE_DATA_DEAD ; +else + all_nodes_done =0 ;break ;case DLM_RECO_NODE_DATA_DONE : mlog(0, "%s: node %u state is done\n",.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
PATCH 1/1 Fix waiting status race condition in dlm recovery V2
Hi, xiaowei,
Quoted 52 lines · xiaowei.hu
Quoted 29 lines · Jeff Liu
Quoted 27 lines · Xue jiufei
Quoted 27 lines
From: "Xiaowei.Hu" <[email protected]>
when the master requested locks ,but one/some of the live nodes died, after it received the request msg and before send out the locks packages, the recovery will fall into endless loop,waiting for the status changed to finalize
NodeA NodeB
selected as recovery masterdlm_remaster_locks
->dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue workerdlm_request_all_locks_workerreturn 0
go on set state to requested
wait for the state become done
NodeB lost connection due to network before the worker begin, or it die.NodeA still waiting for the change of reco state.
It won't end if it not get data done msg.
And at this time nodeB do not realize this (or it just died), it won't send the msg for ever, nodeA left in the recovery process forever.This patch let the recovery master check if the node still in live node map when it stay in REQUESTED status.
Quoted 11 lines
Quoted 5 lines
Hi, xiaowei,
We have reviewed this patch and have some questions:
1) indlm_is_node_in_livemap(), I think it should use !!(test_bit(node,dlm->live_nodes_map)) to determine whether a node is live;Hmm?
test_bit(node,...)is ok.Quoted 4 lines
2) why not use
dlm_is_node_dead()instead ofdlm_is_node_in_livemap()indlm_remaster_locks?
I thinkdlm_is_node_dead()is better becausedlm->live_nodes_mapmay be remain when another node umount.For this question, I remember I explained in one of the emails, paste you that contant:
=============
Hi Sunil,I considered your suggestion about this patch, it's possible to change the status in dlm hb down event,
but what need to change are thedlm_reco_node_datastructures indlm->reco.node_datalist.
This list is initialized indlm_remaster_lockswhen it begins the lock remaster and destroied before exit this function. So it's not proper to check data in such a list from dlm hb down event, am I right?
If change the status from dlm hb down event , that means we make the recovery thread rely on more information from the hb down event, actually thedlm->live_nodes_mapis marked in this event , and for others to check , right?
Quoted 4 lines
This race condition only happen when cluster already in recovery and a node dead during recovery. the recovery thread blocked the update of
dlm->domain_map, so I fallback to check thelive_nodes_map, which won't be blocked.Please reconsider this patch.
================
The update of dlm->domain_map is triggered by hb down event or DLM_BEGIN_RECO_MSG received from recovery master when a node died. I wonder Why it is blocked by recovery thread.
Quoted 64 lines
Quoted 61 lines
Please refer to:
http://marc.info/?l=ocfs2-devel&m=133799814717270&w=2Thanks,
-JeffQuoted 54 lines
Thanks.
Quoted 29 lines
Signed-off-by Xiaowei.Hu <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +10
-0@@ -339,6 +339,17 @@ static int dlm_reco_master_ready(struct dlm_ctxt *dlm)return ready; } +/* returns true if node is still in the live node map +* this map is cleared before domain map,could be checked in recovery*/ +int dlm_is_node_in_livemap(struct dlm_ctxt *dlm, u8 node) +{ + int live; +spin_lock (&dlm->spinlock); + live = !test_bit (node, dlm->live_nodes_map); +spin_unlock (&dlm->spinlock); +return live; +} +/* returns true if node is no longer in the domain * could be dead or just not joined */ int dlm_is_node_dead (struct dlm_ctxt *dlm,u8 node)@@ -679,7 +690,10 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) dlm->name, ndata->node_num,fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 699
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +4
-1ndata->state==DLM_RECO_NODE_DATA_RECEIVING ? "receiving" : "requested"); - all_nodes_done =0 ; +if (!dlm_is_node_in_livemap (dlm, ndata->node_num)) + ndata->state =DLM_RECO_NODE_DATA_DEAD ; +else + all_nodes_done =0 ;break ;case DLM_RECO_NODE_DATA_DONE : mlog(0, "%s: node %u state is done\n",.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel.
PATCH 1/1 Fix waiting status race condition in dlm recovery V2
Hi Xue jiufei,
Quoted 54 lines · Xue jiufei
Hi, xiaowei,
Quoted 52 lines · xiaowei.hu
Quoted 29 lines · Jeff Liu
Quoted 27 lines · Xue jiufei
Quoted 27 lines
From: "Xiaowei.Hu" <[email protected]>
when the master requested locks ,but one/some of the live nodes died, after it received the request msg and before send out the locks packages, the recovery will fall into endless loop,waiting for the status changed to finalize
NodeA NodeB
selected as recovery masterdlm_remaster_locks
->dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue workerdlm_request_all_locks_workerreturn 0
go on set state to requested
wait for the state become done
NodeB lost connection due to network before the worker begin, or it die.NodeA still waiting for the change of reco state.
It won't end if it not get data done msg.
And at this time nodeB do not realize this (or it just died), it won't send the msg for ever, nodeA left in the recovery process forever.This patch let the recovery master check if the node still in live node map when it stay in REQUESTED status.
Quoted 11 lines
Quoted 5 lines
Hi, xiaowei,
We have reviewed this patch and have some questions:
1) indlm_is_node_in_livemap(), I think it should use !!(test_bit(node,dlm->live_nodes_map)) to determine whether a node is live;Hmm?
test_bit(node,...)is ok.Quoted 4 lines
2) why not use
dlm_is_node_dead()instead ofdlm_is_node_in_livemap()indlm_remaster_locks?
I thinkdlm_is_node_dead()is better becausedlm->live_nodes_mapmay be remain when another node umount.For this question, I remember I explained in one of the emails, paste you that contant:
=============
Hi Sunil,I considered your suggestion about this patch, it's possible to change the status in dlm hb down event,
but what need to change are thedlm_reco_node_datastructures indlm->reco.node_datalist.
This list is initialized indlm_remaster_lockswhen it begins the lock remaster and destroied before exit this function. So it's not proper to check data in such a list from dlm hb down event, am I right?
If change the status from dlm hb down event , that means we make the recovery thread rely on more information from the hb down event, actually thedlm->live_nodes_mapis marked in this event , and for others to check , right?
Quoted 7 lines
Quoted 4 lines
This race condition only happen when cluster already in recovery and a node dead during recovery. the recovery thread blocked the update of
dlm->domain_map, so I fallback to check thelive_nodes_map, which won't be blocked.Please reconsider this patch.
================The update of
dlm->domain_mapis triggered by hb down event orDLM_BEGIN_RECO_MSGreceived from recovery master when a node died. I wonder Why it is blocked by recovery thread.
I remembered I tried to use dlm_is_node_dead in this patch at first glance too, but this function can't report another node is dead if this node already in recovery process. It was blocked to set the bit in domain_map, but the live_nodes_map could always reflect the really live nodes.
I believe it's blocked in dlm_do_local_recovery_cleanup(dlm, idx), but doesn't remember which lock exactly...
Quoted 63 lines
Quoted 62 lines
Quoted 60 lines
Please refer to:
http://marc.info/?l=ocfs2-devel&m=133799814717270&w=2Thanks,
-JeffQuoted 53 lines
Thanks.
Quoted 29 lines
Signed-off-by Xiaowei.Hu <[email protected]>--- fs/ocfs2/dlm/dlmrecovery.c | 16 +++++++++++++++- 1 files changed, 15 insertions(+), 1 deletions(-)fs/ocfs2/dlm/dlmrecovery.c +10
-0@@ -339,6 +339,17 @@ static int dlm_reco_master_ready(struct dlm_ctxt *dlm)return ready; } +/* returns true if node is still in the live node map +* this map is cleared before domain map,could be checked in recovery*/ +int dlm_is_node_in_livemap(struct dlm_ctxt *dlm, u8 node) +{ + int live; +spin_lock (&dlm->spinlock); + live = !test_bit (node, dlm->live_nodes_map); +spin_unlock (&dlm->spinlock); +return live; +} +/* returns true if node is no longer in the domain * could be dead or just not joined */ int dlm_is_node_dead (struct dlm_ctxt *dlm,u8 node)@@ -679,7 +690,10 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) dlm->name, ndata->node_num,fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 699
Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +4
-1ndata->state==DLM_RECO_NODE_DATA_RECEIVING ? "receiving" : "requested"); - all_nodes_done =0 ; +if (!dlm_is_node_in_livemap (dlm, ndata->node_num)) + ndata->state =DLM_RECO_NODE_DATA_DEAD ; +else + all_nodes_done =0 ;break ;case DLM_RECO_NODE_DATA_DONE : mlog(0, "%s: node %u state is done\n",.
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel.