md-cluster: avoid deadlock on MESSAGE lock resource

PATCH Superseded Abhijit Bhopatkar 3 participants 6 messages May 8, 2015 – May 26, 2015 MD RAID

Share & tools

Abhijit Bhopatkar

author

2015-05-08 13:10 UTC #0 lore raw

PATCH 1/1 md-cluster: avoid deadlock on MESSAGE lock resource

Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.

Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.

Signed-off-by Abhijit Bhopatkar <[email protected]>

---
This has been minimally tested on a three node cluster.

 drivers/md/md-cluster.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/md/md-cluster.c +12 -2

index fcfc4b9..04ac309 100644
@@ -512,7 +512,10 @@ static void unlock_comm(struct md_cluster_info *cinfo)
  * This function performs the actual sending of the message. This function is
  * usually called after performing the encompassing operation
  * The function:
- * 1. Grabs the message lockresource in EX mode
+ * 1. Grabs the message lockresource in EX. Do not queue the request if not granted
+      immediately. This avoids deadlock with receivers when receivers try to
+      upconvert CR to EX of message lockresource. The thread will retry until the
+      request is granted.
  * 2. Copies the message to the message LVB
  * 3. Downconverts message lockresource to CR
  * 4. Upconverts ack lock resource from CR to EX. This forces the BAST on other nodes

@@ -526,12 +529,19 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
 	int slot = cinfo->slot_number - 1;
 
 	cmsg->slot = cpu_to_le32(slot);
-	/*get EX on Message*/
+
+	/* get EX on Message with noqueue flag */
+	cinfo->message_lockres->flags |= DLM_LKF_NOQUEUE;
+
+retry:
 	error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_EX);
 	if (error) {
+		if (error == -EAGAIN)
+			goto retry;
 		pr_err("md-cluster: failed to get EX on MESSAGE (%d)\n", error);
 		goto failed_message;
 	}
+	cinfo->message_lockres->flags &= ~DLM_LKF_NOQUEUE;
 
 	memcpy(cinfo->message_lockres->lksb.sb_lvbptr, (void *)cmsg,
 			sizeof(struct cluster_msg));

-- 2.1.0

Abhijit Bhopatkar

author

2015-05-08 13:14 UTC #1 lore raw

Quoted 7 lines · Abhijit Bhopatkar

Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.

Quoted 8 lines

Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.
Signed-off-by Abhijit Bhopatkar <[email protected]>
---
This has been minimally tested on a three node cluster.

I have tested standard mdadm operations (create, assemble etc). What more testing would you want me to do on this before its considered ready?

Regards,
Abhijit

Quoted 46 lines

 drivers/md/md-cluster.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/md/md-cluster.c +12 -4

index fcfc4b9..04ac309 100644
@@ -512,7 +512,10 @@ static void unlock_comm(struct md_cluster_info *cinfo)
  * This function performs the actual sending of the message. This function is
  * usually called after performing the encompassing operation
  * The function:
- * 1. Grabs the message lockresource in EX mode
+ * 1. Grabs the message lockresource in EX. Do not queue the request if not granted
+      immediately. This avoids deadlock with receivers when receivers try to
+      upconvert CR to EX of message lockresource. The thread will retry until the
+      request is granted.
  * 2. Copies the message to the message LVB
  * 3. Downconverts message lockresource to CR
  * 4. Upconverts ack lock resource from CR to EX. This forces the BAST on other nodes

@@ -526,12 +529,19 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
 	int slot = cinfo->slot_number - 1;
 
 	cmsg->slot = cpu_to_le32(slot);
-	/*get EX on Message*/
+
+	/* get EX on Message with noqueue flag */
+	cinfo->message_lockres->flags |= DLM_LKF_NOQUEUE;
+
+retry:
 	error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_EX);
 	if (error) {
+		if (error == -EAGAIN)
+			goto retry;
 		pr_err("md-cluster: failed to get EX on MESSAGE (%d)\n", error);
 		goto failed_message;
 	}
+	cinfo->message_lockres->flags &= ~DLM_LKF_NOQUEUE;
 
 	memcpy(cinfo->message_lockres->lksb.sb_lvbptr, (void *)cmsg,
 			sizeof(struct cluster_msg));
-- 2.1.0 
--

To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Lidong Zhong

2015-05-13 02:05 UTC #2 lore raw

Quoted 1 line

Quoted 1 line
Quoted 1 line
On 5/8/2015 at 09:14 PM, in message <[email protected]>, Abhijit

Quoted 12 lines · Bhopatkar

Quoted 11 lines · Abhijit Bhopatkar
Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.
Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.

Quoted 9 lines

Quoted 4 lines
Signed-off-by Abhijit Bhopatkar <[email protected]>
---
This has been minimally tested on a three node cluster.
I have tested standard mdadm operations (create, assemble etc). What more testing would you want me to do on this before its considered ready?

The patch seems fine to me. Let's see what Goldwyn's suggestion is here.

Regards,
Lidong

Quoted 62 lines

Regards,
Abhijit

drivers/md/md-cluster.c static void unlock_comm(struct md_cluster_info *cinfo) · line 512

Quoted 9 lines

 drivers/md/md-cluster.c | 14 ++++++++++++-- 
 1 file changed, 12 insertions(+), 2 deletions(-)

drivers/md/md-cluster.c +0 -0

index fcfc4b9..04ac309 100644 
@@ -512,7 +512,10 @@ static void unlock_comm(struct md_cluster_info *cinfo) 
  * This function performs the actual sending of the message. This function

drivers/md/md-cluster.c static void unlock_comm(struct md_cluster_info *cinfo)

Quoted 4 lines

drivers/md/md-cluster.c +1 -1

  * usually called after performing the encompassing operation 
  * The function: 
- * 1. Grabs the message lockresource in EX mode 
+ * 1. Grabs the message lockresource in EX. Do not queue the request if

not granted

drivers/md/md-cluster.c static void unlock_comm(struct md_cluster_info *cinfo) · line 516

Quoted 1 line

drivers/md/md-cluster.c +1 -0

+      immediately. This avoids deadlock with receivers when receivers try

drivers/md/md-cluster.c static void unlock_comm(struct md_cluster_info *cinfo) · line 517

Quoted 1 line

drivers/md/md-cluster.c +1 -0

+      upconvert CR to EX of message lockresource. The thread will retry

until the

drivers/md/md-cluster.c static void unlock_comm(struct md_cluster_info *cinfo)

Quoted 4 lines

drivers/md/md-cluster.c +1 -0

+      request is granted. 
  * 2. Copies the message to the message LVB 
  * 3. Downconverts message lockresource to CR 
  * 4. Upconverts ack lock resource from CR to EX. This forces the BAST on

other nodes

drivers/md/md-cluster.c static int __sendmsg(struct md_cluster_info *cinfo,

Quoted 1 line

drivers/md/md-cluster.c +0 -0

@@ -526,12 +529,19 @@ static int __sendmsg(struct md_cluster_info *cinfo,

struct cluster_msg *cmsg)

Quoted 26 lines

drivers/md/md-cluster.c +8 -2

 	int slot = cinfo->slot_number - 1; 
 
 	cmsg->slot = cpu_to_le32(slot); 
-	/*get EX on Message*/ 
+ 
+	/* get EX on Message with noqueue flag */ 
+	cinfo->message_lockres->flags |= DLM_LKF_NOQUEUE; 
+ 
+retry: 
 	error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_EX); 
 	if (error) { 
+		if (error == -EAGAIN) 
+			goto retry; 
 		pr_err("md-cluster: failed to get EX on MESSAGE (%d)\n", error); 
 		goto failed_message; 
 	} 
+	cinfo->message_lockres->flags &= ~DLM_LKF_NOQUEUE; 
 
 	memcpy(cinfo->message_lockres->lksb.sb_lvbptr, (void *)cmsg, 
 			sizeof(struct cluster_msg)); 
-- 2.1.0

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-raid" in 
the body of a message to [email protected] 
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Signature · 3 lines

To unsubscribe from this list: send the line "unsubscribe linux-raid" in 
the body of a message to [email protected] 
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Goldwyn Rodrigues

2015-05-16 20:58 UTC #3 lore raw

In reply to Abhijit Bhopatkar

Quoted 12 lines · Abhijit Bhopatkar

Quoted 11 lines · Abhijit Bhopatkar
Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.
Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.

Quoted 8 lines

Quoted 4 lines
Signed-off-by Abhijit Bhopatkar <[email protected]>
---
This has been minimally tested on a three node cluster.
I have tested standard mdadm operations (create, assemble etc). What more testing would you want me to do on this before its considered ready?

I am not sure how using LKF_NOQUEUE will help in this situation here. LKF_NOQUEUE primarily means do not queue if you can't grant it right away. Besides, I don't like the idea of goto loop.

The sender can still creep in between the ack and the message locks. A situation would be where the "disrupting" sender is the lock owner of all the locks and hence will not have to pay communication costs and will manage to attain the locks faster.

Perhaps DLM_LKF_HEADQUEUE or DLM_LKF_NOORDER is what you are looking for, but that again is not the complete solution.

Another idea I could think of is for the sender to downconvert TOKEN to a shared lock such as CR halfway in the communication (say after message CR), and all receivers take the TOKEN in CR mode and release it once the communication is finally over.

Regards,

Quoted 52 lines

Regards,
Abhijit

Quoted 46 lines

  drivers/md/md-cluster.c | 14 ++++++++++++--
  1 file changed, 12 insertions(+), 2 deletions(-)

drivers/md/md-cluster.c +12 -4

index fcfc4b9..04ac309 100644
@@ -512,7 +512,10 @@ static void unlock_comm(struct md_cluster_info *cinfo)
   * This function performs the actual sending of the message. This function is
   * usually called after performing the encompassing operation
   * The function:
- * 1. Grabs the message lockresource in EX mode
+ * 1. Grabs the message lockresource in EX. Do not queue the request if not granted
+      immediately. This avoids deadlock with receivers when receivers try to
+      upconvert CR to EX of message lockresource. The thread will retry until the
+      request is granted.
   * 2. Copies the message to the message LVB
   * 3. Downconverts message lockresource to CR
   * 4. Upconverts ack lock resource from CR to EX. This forces the BAST on other nodes

@@ -526,12 +529,19 @@ static int __sendmsg(struct md_cluster_info *cinfo, struct cluster_msg *cmsg)
  	int slot = cinfo->slot_number - 1;
 
  	cmsg->slot = cpu_to_le32(slot);
-	/*get EX on Message*/
+
+	/* get EX on Message with noqueue flag */
+	cinfo->message_lockres->flags |= DLM_LKF_NOQUEUE;
+
+retry:
  	error = dlm_lock_sync(cinfo->message_lockres, DLM_LOCK_EX);
  	if (error) {
+		if (error == -EAGAIN)
+			goto retry;
  		pr_err("md-cluster: failed to get EX on MESSAGE (%d)\n", error);
  		goto failed_message;
  	}
+	cinfo->message_lockres->flags &= ~DLM_LKF_NOQUEUE;
 
  	memcpy(cinfo->message_lockres->lksb.sb_lvbptr, (void *)cmsg,
  			sizeof(struct cluster_msg));
-- 2.1.0
--

To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 1 line

Goldwyn

Abhijit Bhopatkar

author

2015-05-25 14:26 UTC #4 lore raw

Quoted 26 lines · Goldwyn Rodrigues

Quoted 12 lines · Abhijit Bhopatkar
Quoted 11 lines · Abhijit Bhopatkar
Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.
Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Abhijit Bhopatkar <[email protected]>
---
This has been minimally tested on a three node cluster.
I have tested standard mdadm operations (create, assemble etc). What more testing would you want me to do on this before its considered ready?
I am not sure how using LKF_NOQUEUE will help in this situation here. LKF_NOQUEUE primarily means do not queue if you can't grant it right away. Besides, I don't like the idea of goto loop.

Quoted 9 lines

The sender can still creep in between the ack and the message locks. A situation would be where the "disrupting" sender is the lock owner of all the locks and hence will not have to pay communication costs and will manage to attain the locks faster.
Perhaps DLM_LKF_HEADQUEUE or DLM_LKF_NOORDER is what you are looking for, but that again is not the complete solution.
Another idea I could think of is for the sender to downconvert TOKEN to a shared lock such as CR halfway in the communication (say after message CR), and all receivers take the TOKEN in CR mode and release it once the communication is finally over.
Regards,

I agree about the goto pollution and yes converting receivers to use DLM_LKF_HEADQUEUE will solve the problem gracefully. Will send the new patch shortly.

However I do not understand why this is incomplete solution. The "disruptive sender" as you have called it, is already "TOKEN" owner and otherwise it will compete for TOKEN lock as usual with other senders with equal priority. Not gaining any priority over others. The changes simply make sender stall for all _receivers_ to complete their serialization and wait till all receivers convert MESSAGE lock from CR to EX to NL, nothing else changes.

Regards,

Quoted 3 lines

Quoted 3 lines
Regards,
Abhijit

<snip>

Goldwyn Rodrigues

2015-05-26 14:44 UTC #5 lore raw

Quoted 31 lines · Abhijit Bhopatkar

Quoted 30 lines · Goldwyn Rodrigues
Quoted 12 lines · Abhijit Bhopatkar
Quoted 11 lines · Abhijit Bhopatkar
Every receiver has CR lock on MESSAGE while processing the message. When every receiver releases ACK lock and for some reason fails to grab EX on MESSAGE resource in time, a waiting sender could queue an EX on MESSAGE instead. Now when receiver queues its up convert request on MESSAGE it will end up in a deadlock situation.
Setting NOQUEUE flag on MESSAGE lock resource while grabbing the EX on MESSAGE on sender will avoid this deadlock. If sender can not grab MESSAGE lock immediately it should retry until the lock is granted.
Quoted 8 lines
Quoted 4 lines
Signed-off-by Abhijit Bhopatkar <[email protected]>
---
This has been minimally tested on a three node cluster.
I have tested standard mdadm operations (create, assemble etc). What more testing would you want me to do on this before its considered ready?
I am not sure how using LKF_NOQUEUE will help in this situation here. LKF_NOQUEUE primarily means do not queue if you can't grant it right away. Besides, I don't like the idea of goto loop.
The sender can still creep in between the ack and the message locks. A situation would be where the "disrupting" sender is the lock owner of all the locks and hence will not have to pay communication costs and will manage to attain the locks faster.
Perhaps DLM_LKF_HEADQUEUE or DLM_LKF_NOORDER is what you are looking for, but that again is not the complete solution.

Quoted 9 lines

Quoted 5 lines
Another idea I could think of is for the sender to downconvert TOKEN to a shared lock such as CR halfway in the communication (say after message CR), and all receivers take the TOKEN in CR mode and release it once the communication is finally over.
Regards,
I agree about the goto pollution and yes converting receivers to use DLM_LKF_HEADQUEUE will solve the problem gracefully. Will send the new patch shortly.
However I do not understand why this is incomplete solution. The "disruptive sender" as you have called it, is already "TOKEN" owner and otherwise it will compete for TOKEN lock as usual with other senders with equal priority. Not gaining any priority over others. The changes simply make sender stall for all _receivers_ to complete their serialization and wait till all receivers convert MESSAGE lock from CR to EX to NL, nothing else changes.

Yes, you are right. I ignored an operation on the message lock resource. I will perform some tests before I signoff.

Thanks,

Signature · 1 line

Goldwyn