Skip to contentSkip to conversation

Fix waiting status race condition in dlm recovery

PATCH New xiaowei.hu at oracle.com 3 participants 7 messages May 25, 2012 – Jul 26, 2012 OCFS2

lore
Share & tools
lore

xiaowei.hu at oracle.com

author
#0 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

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 master
dlm_remaster_locks
-> dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue worker dlm_request_all_locks_worker return 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.

Signed-off-by Xiaowei.Hu <[email protected]>
---
 fs/ocfs2/dlm/dlmrecovery.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +9 -0
index 01ebfd0..62659e8 100644
@@ -555,6 +555,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
 	int all_nodes_done;
 	int destroy = 0;
 	int pass = 0;
+	int dying = 0;
 
 	do {
 		/* we have become recovery master.  there is no escaping

@@ -659,6 +660,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
 		list_for_each_entry(ndata, &dlm->reco.node_data, list) {
 			mlog(0, "checking recovery state of node %u\n",
 			     ndata->node_num);
+			dying = 0;
 			switch (ndata->state) {
 				case DLM_RECO_NODE_DATA_INIT:
 				case DLM_RECO_NODE_DATA_REQUESTING:

@@ -679,6 +681,13 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
 					     dlm->name, ndata->node_num,
 					     ndata->state==DLM_RECO_NODE_DATA_RECEIVING ?
 					     "receiving" : "requested");
+					spin_lock(&dlm->spinlock);
+					dying = !test_bit(ndata->node_num, dlm->live_nodes_map);
+					spin_unlock(&dlm->spinlock);
+					if (dying) {
+						ndata->state = DLM_RECO_NODE_DATA_DEAD;
+						break;
+					}
 					all_nodes_done = 0;
 					break;
 				case DLM_RECO_NODE_DATA_DONE:
Signature · 1 line
1.7.7.6
#1 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

comments inline

Quoted 56 lines · xiaowei.hu at oracle.com

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 master
dlm_remaster_locks
-> dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue worker dlm_request_all_locks_worker return 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.

Signed-off-by Xiaowei.Hu<[email protected]>
---
  fs/ocfs2/dlm/dlmrecovery.c |    9 +++++++++
  1 files changed, 9 insertions(+), 0 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +2 -0
index 01ebfd0..62659e8 100644
@@ -555,6 +555,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
  	int all_nodes_done;
  	int destroy = 0;
  	int pass = 0;
+	int dying = 0;
 
  	do {
  		/* we have become recovery master.  there is no escaping

@@ -659,6 +660,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
  		list_for_each_entry(ndata,&dlm->reco.node_data, list) {
  			mlog(0, "checking recovery state of node %u\n",
  			     ndata->node_num);
+			dying = 0;
  			switch (ndata->state) {
  				case DLM_RECO_NODE_DATA_INIT:
  				case DLM_RECO_NODE_DATA_REQUESTING:

@@ -679,6 +681,13 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node)
  					     dlm->name, ndata->node_num,
  					     ndata->state==DLM_RECO_NODE_DATA_RECEIVING ?
  					     "receiving" : "requested");

fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 693

Quoted 10 lines
fs/ocfs2/dlm/dlmrecovery.c +7 -0
+					spin_lock(&dlm->spinlock);
+					dying = !test_bit(ndata->node_num, dlm->live_nodes_map);
+					spin_unlock(&dlm->spinlock);
+					if (dying) {
+						ndata->state = DLM_RECO_NODE_DATA_DEAD;
+						break;
+					}
  					all_nodes_done = 0;
  					break;
  				case DLM_RECO_NODE_DATA_DONE:

fix seems to address the issue, but can you please add a function dlm_is_node_in_livemap similar to dlm_is_node_dead so that it' improves readability. You can then add the following to check if the node is still alive
+ if (!dlm_is_node_in_livemap(dlm, ndata->node_num))
+ ndate->state = DLM_RECO_NODE_DATA_DEAD;
+ else
+ all_nodes_done = 0;

author
#2 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

Thanks Srini ,
This sounds good, I tried to use dlm_is_node_dead in this patch , 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 will reformat the patch.

Thanks,
Xiaowei

Quoted 87 lines · srinivas eeda

comments inline

Quoted 74 lines · xiaowei.hu at oracle.com

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 master
dlm_remaster_locks
-> dlm_requeset_all_locks
this send request locks msg to B
received the msg from A, queue worker
dlm_request_all_locks_worker
return 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.

Signed-off-by Xiaowei.Hu<[email protected]>
---
  fs/ocfs2/dlm/dlmrecovery.c |    9 +++++++++
  1 files changed, 9 insertions(+), 0 deletions(-)
fs/ocfs2/dlm/dlmrecovery.c +1 -0
index 01ebfd0..62659e8 100644
@@ -555,6 +555,7 @@ static int dlm_remaster_locks(struct dlm_ctxt 
 *dlm, u8 dead_node)
      int all_nodes_done;
      int destroy = 0;
      int pass = 0;
+    int dying = 0;
 
      do {

          /* we have become recovery master.  there is no escaping
fs/ocfs2/dlm/dlmrecovery.c +1 -0
@@ -659,6 +660,7 @@ static int dlm_remaster_locks(struct dlm_ctxt 
 *dlm, u8 dead_node)
          list_for_each_entry(ndata,&dlm->reco.node_data, list) {
              mlog(0, "checking recovery state of node %u\n",
                   ndata->node_num);
+            dying = 0;
              switch (ndata->state) {
                  case DLM_RECO_NODE_DATA_INIT:

case DLM_RECO_NODE_DATA_REQUESTING:

fs/ocfs2/dlm/dlmrecovery.c +7 -0
@@ -679,6 +681,13 @@ static int dlm_remaster_locks(struct dlm_ctxt 
 *dlm, u8 dead_node)
                           dlm->name, ndata->node_num,
                           ndata->state==DLM_RECO_NODE_DATA_RECEIVING ?
                           "receiving" : "requested");
+                    spin_lock(&dlm->spinlock);
+                    dying = !test_bit(ndata->node_num, 
 dlm->live_nodes_map);
+                    spin_unlock(&dlm->spinlock);
+                    if (dying) {
+                        ndata->state = DLM_RECO_NODE_DATA_DEAD;
+                        break;
+                    }
                      all_nodes_done = 0;
Quoted 2 lines
                      break;
                  case DLM_RECO_NODE_DATA_DONE:

fix seems to address the issue, but can you please add a function dlm_is_node_in_livemap similar to dlm_is_node_dead so that it' improves readability. You can then add the following to check if the node is still alive

(quoted diff) +4 -0
+        if (!dlm_is_node_in_livemap(dlm, ndata->node_num))
+            ndate->state = DLM_RECO_NODE_DATA_DEAD;
+        else
+            all_nodes_done = 0;
#3 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

In reply to xiaowei.hu at oracle.com
Quoted 37 lines · On Thu, May 24, 2012 at 10:53 PM, <[email protected]> wrote:
fs/ocfs2/dlm/dlmrecovery.c +1 -0
index 01ebfd0..62659e8 100644
@@ -555,6 +555,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8
 dead_node)
       int all_nodes_done;
       int destroy = 0;
       int pass = 0;
+       int dying = 0;
 
       do {

               /* we have become recovery master.  there is no escaping
fs/ocfs2/dlm/dlmrecovery.c +1 -0
@@ -659,6 +660,7 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8
 dead_node)
               list_for_each_entry(ndata, &dlm->reco.node_data, list) {
                       mlog(0, "checking recovery state of node %u\n",
                            ndata->node_num);
+                       dying = 0;
                       switch (ndata->state) {
                               case DLM_RECO_NODE_DATA_INIT:

case DLM_RECO_NODE_DATA_REQUESTING:

fs/ocfs2/dlm/dlmrecovery.c +0 -0
@@ -679,6 +681,13 @@ static int dlm_remaster_locks(struct dlm_ctxt *dlm,
 u8 dead_node)
                                            dlm->name, ndata->node_num,

ndata->state==DLM_RECO_NODE_DATA_RECEIVING ?

                                            "receiving" : "requested");
fs/ocfs2/dlm/dlmrecovery.c +2 -0
+                                       spin_lock(&dlm->spinlock);
+                                       dying = !test_bit(ndata->node_num,

dlm->live_nodes_map);

fs/ocfs2/dlm/dlmrecovery.c +3 -0
+                                       spin_unlock(&dlm->spinlock);
+                                       if (dying) {
+                                               ndata->state =

DLM_RECO_NODE_DATA_DEAD;

fs/ocfs2/dlm/dlmrecovery.c static int dlm_remaster_locks(struct dlm_ctxt *dlm, u8 dead_node) · line 690

Quoted 2 lines
fs/ocfs2/dlm/dlmrecovery.c +2 -0
+                                               break;
+                                       }

I would suggest exploring adding this in dlm hb down event. Checking live map all
over the place is hacky. We do it more than we should right now. Let's not add to the
mess.

Quoted 12 lines
                                       all_nodes_done = 0;
                                       break;
                               case DLM_RECO_NODE_DATA_DONE:

--
1.7.7.6

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20120529/1080a567/attachment.html

author
#4 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

Quoted 50 lines · Sunil Mushran

On Thu, May 24, 2012 at 10:53 PM, <[email protected] <mailto:[email protected]>> wrote:

    diff --git a/fs/ocfs2/dlm/dlmrecovery.c b/fs/ocfs2/dlm/dlmrecovery.c
    index 01ebfd0..62659e8 100644
    --- a/fs/ocfs2/dlm/dlmrecovery.c
    +++ b/fs/ocfs2/dlm/dlmrecovery.c
    @@ -555,6 +555,7 @@ static int dlm_remaster_locks(struct dlm_ctxt
    *dlm, u8 dead_node)
           int all_nodes_done;
           int destroy = 0;
           int pass = 0;
    +       int dying = 0;

           do {
                   /* we have become recovery master.  there is no
    escaping
    @@ -659,6 +660,7 @@ static int dlm_remaster_locks(struct dlm_ctxt
    *dlm, u8 dead_node)
                   list_for_each_entry(ndata, &dlm->reco.node_data,
    list) {
                           mlog(0, "checking recovery state of node %u\n",
                                ndata->node_num);
    +                       dying = 0;
                           switch (ndata->state) {
                                   case DLM_RECO_NODE_DATA_INIT:
                                   case DLM_RECO_NODE_DATA_REQUESTING:
    @@ -679,6 +681,13 @@ static int dlm_remaster_locks(struct dlm_ctxt
    *dlm, u8 dead_node)
                                                dlm->name,
    ndata->node_num,
                                               
    ndata->state==DLM_RECO_NODE_DATA_RECEIVING ?
                                                "receiving" :
    "requested");
    +                                       spin_lock(&dlm->spinlock);
    +                                       dying =
    !test_bit(ndata->node_num, dlm->live_nodes_map);
    +                                       spin_unlock(&dlm->spinlock);
    +                                       if (dying) {
    +                                               ndata->state =
    DLM_RECO_NODE_DATA_DEAD;
    +                                               break;
    +                                       }
Quoted 5 lines

I would suggest exploring adding this in dlm hb down event. Checking live map all
over the place is hacky. We do it more than we should right now. Let's not add to the
mess.

HI Sunil,

Do you mean we should clear the bit in domain map in dlm hb down event directly when the node down
and check with dlm_is_node_dead at here?
Or how could we explore and ensure the node is alive during the whole migrate process?One node could die even after it sends out one locks package and before the next if there were too many locks on that lockres.

Thanks,
Xiaowei

Quoted 16 lines


                                           all_nodes_done = 0;
                                           break;
                                   case DLM_RECO_NODE_DATA_DONE:
    --
    1.7.7.6


    _______________________________________________
    Ocfs2-devel mailing list
    Ocfs2-devel at oss.oracle.com <mailto:[email protected]>
    http://oss.oracle.com/mailman/listinfo/ocfs2-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20120530/5fcb3ea7/attachment.html

#5 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

Quoted 7 lines · Xiaowei

On 05/30/2012 06:09 AM, Sunil Mushran wrote:
I would suggest exploring adding this in dlm hb down event. Checking live map all
over the place is hacky. We do it more than we should right now. Let's not add to the
mess.

Quoted 8 lines

HI Sunil,

Do you mean we should clear the bit in domain map in dlm hb down event directly when the node down
and check with dlm_is_node_dead at here?
Or how could we explore and ensure the node is alive during the whole migrate process?One node could die even after it sends out one locks package and before the next if there were too many locks on that lockres.

dlm hb down event is triggered when a node is declared dead. That's where we clean up pending mles, etc. You can add a check for recovery and add logic to change the reco state for that node there.

author
#6 lore raw

PATCH 1/1 Fix waiting status race condition in dlm recovery

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.

Thanks,
Xiaowei

Quoted 19 lines · Sunil Mushran
Quoted 7 lines · Xiaowei

On 05/30/2012 06:09 AM, Sunil Mushran wrote:
I would suggest exploring adding this in dlm hb down event. Checking live map all
over the place is hacky. We do it more than we should right now. Let's not add to the
mess.

Quoted 8 lines

HI Sunil,

Do you mean we should clear the bit in domain map in dlm hb down event directly when the node down
and check with dlm_is_node_dead at here?
Or how could we explore and ensure the node is alive during the whole migrate process?One node could die even after it sends out one locks package and before the next if there were too many locks on that lockres.

dlm hb down event is triggered when a node is declared dead. That's where we clean up pending mles, etc. You can add a check for recovery and add logic to change the reco state for that node there.