Skip to contentSkip to conversation

Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

PATCH New Wengang Wang 3 participants 7 messages Sep 15, 2011 – Nov 17, 2011 OCFS2

lore
Share & tools
lore

Wengang Wang

author
#0 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

When the lockres state UPCONVERT_FINISHING is cleared, we should wake up the downconvert thread incase that lockres is in the blocked queue. Currently we are not doing so and thus are at the mercy of another event waking up the dc thread.

Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dlmglue.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
fs/ocfs2/dlmglue.c +8 -1
index 7642d7c..524bd88 100644
@@ -1195,6 +1195,7 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
 						int convert)
 {
 	unsigned long flags;
+	int kick_dc;
 
 	spin_lock_irqsave(&lockres->l_lock, flags);
 	lockres_clear_flags(lockres, OCFS2_LOCK_BUSY);

@@ -1203,9 +1204,12 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
 		lockres->l_action = OCFS2_AST_INVALID;
 	else
 		lockres->l_unlock_action = OCFS2_UNLOCK_INVALID;
+	kick_dc = (lockres->l_flags & OCFS2_LOCK_QUEUED);
 	spin_unlock_irqrestore(&lockres->l_lock, flags);
 
 	wake_up(&lockres->l_event);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(ocfs2_get_lockres_osb(lockres));
 }
 
 /* Note: If we detect another process working on the lock (i.e.,

@@ -1373,6 +1377,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
 	unsigned long flags;
 	unsigned int gen;
 	int noqueue_attempted = 0;
+	int kick_dc;
 
 	ocfs2_init_mask_waiter(&mw);
 

@@ -1500,8 +1505,10 @@ update_holders:
 	ret = 0;
 unlock:
 	lockres_clear_flags(lockres, OCFS2_LOCK_UPCONVERT_FINISHING);
-
+	kick_dc = (lockres->l_flags & OCFS2_LOCK_QUEUED);
 	spin_unlock_irqrestore(&lockres->l_lock, flags);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(osb);
 out:
 	/*
 	 * This is helping work around a lock inversion between the page lock
Signature · 1 line
1.7.5.2
#1 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

I am fine with the kick in recover from dlm error. Not so in cluster lock. We have to be very very sure before meddling with that function. It is a state machine with many hidden gotchas.

So is this patch for a bug encountered or just code audit. Also, what kind testing has been done.

Quoted 55 lines · Wengang Wang

When the lockres state UPCONVERT_FINISHING is cleared, we should wake up the downconvert thread incase that lockres is in the blocked queue. Currently we are not doing so and thus are at the mercy of another event waking up the dc thread.

Signed-off-by Wengang Wang<[email protected]>
---
  fs/ocfs2/dlmglue.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)
fs/ocfs2/dlmglue.c +8 -1
index 7642d7c..524bd88 100644
@@ -1195,6 +1195,7 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
  						int convert)
  {
  	unsigned long flags;
+	int kick_dc;
 
  	spin_lock_irqsave(&lockres->l_lock, flags);
  	lockres_clear_flags(lockres, OCFS2_LOCK_BUSY);

@@ -1203,9 +1204,12 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
  		lockres->l_action = OCFS2_AST_INVALID;
  	else
  		lockres->l_unlock_action = OCFS2_UNLOCK_INVALID;
+	kick_dc = (lockres->l_flags&  OCFS2_LOCK_QUEUED);
  	spin_unlock_irqrestore(&lockres->l_lock, flags);
 
  	wake_up(&lockres->l_event);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(ocfs2_get_lockres_osb(lockres));
  }
 
  /* Note: If we detect another process working on the lock (i.e.,

@@ -1373,6 +1377,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
  	unsigned long flags;
  	unsigned int gen;
  	int noqueue_attempted = 0;
+	int kick_dc;
 
  	ocfs2_init_mask_waiter(&mw);
 

@@ -1500,8 +1505,10 @@ update_holders:
  	ret = 0;
  unlock:
  	lockres_clear_flags(lockres, OCFS2_LOCK_UPCONVERT_FINISHING);
-
+	kick_dc = (lockres->l_flags&  OCFS2_LOCK_QUEUED);
  	spin_unlock_irqrestore(&lockres->l_lock, flags);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(osb);
  out:
  	/*
  	 * This is helping work around a lock inversion between the page lock
#2 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

http://people.redhat.com/~teigland/make_panic

This test has been useful in exposing dlmglue issues.

Quoted 68 lines · Sunil Mushran

I am fine with the kick in recover from dlm error. Not so in cluster lock. We have to be very very sure before meddling with that function. It is a state machine with many hidden gotchas.

So is this patch for a bug encountered or just code audit. Also, what kind testing has been done.

Quoted 45 lines · Wengang Wang

When the lockres state UPCONVERT_FINISHING is cleared, we should wake up the downconvert thread incase that lockres is in the blocked queue. Currently we are not doing so and thus are at the mercy of another event waking up the dc thread.

Signed-off-by Wengang Wang<[email protected]>
---
   fs/ocfs2/dlmglue.c |    9 ++++++++-
   1 files changed, 8 insertions(+), 1 deletions(-)
fs/ocfs2/dlmglue.c +5 -0
index 7642d7c..524bd88 100644
@@ -1195,6 +1195,7 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
   						int convert)
   {
   	unsigned long flags;
+	int kick_dc;
 
   	spin_lock_irqsave(&lockres->l_lock, flags);
   	lockres_clear_flags(lockres, OCFS2_LOCK_BUSY);

@@ -1203,9 +1204,12 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
   		lockres->l_action = OCFS2_AST_INVALID;
   	else
   		lockres->l_unlock_action = OCFS2_UNLOCK_INVALID;
+	kick_dc = (lockres->l_flags&   OCFS2_LOCK_QUEUED);
   	spin_unlock_irqrestore(&lockres->l_lock, flags);
 
   	wake_up(&lockres->l_event);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(ocfs2_get_lockres_osb(lockres));
   }
 
   /* Note: If we detect another process working on the lock (i.e.,

@@ -1373,6 +1377,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
   	unsigned long flags;
   	unsigned int gen;
   	int noqueue_attempted = 0;
+	int kick_dc;
 
   	ocfs2_init_mask_waiter(&mw);
 

@@ -1500,8 +1505,10 @@ update_holders:
   	ret = 0;

fs/ocfs2/dlmglue.c update_holders: · line 1514

Quoted 10 lines
fs/ocfs2/dlmglue.c +3 -1
   unlock:
   	lockres_clear_flags(lockres, OCFS2_LOCK_UPCONVERT_FINISHING);
-
+	kick_dc = (lockres->l_flags&   OCFS2_LOCK_QUEUED);
   	spin_unlock_irqrestore(&lockres->l_lock, flags);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(osb);
   out:
   	/*
   	 * This is helping work around a lock inversion between the page lock

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

author
#3 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

Hi Sunil,

Quoted 11 lines · Sunil Mushran

http://people.redhat.com/~teigland/make_panic

This test has been useful in exposing dlmglue issues.

Quoted 6 lines · Sunil Mushran

I am fine with the kick in recover from dlm error. Not so in cluster lock. We have to be very very sure before meddling with that function. It is a state machine with many hidden gotchas.

So is this patch for a bug encountered or just code audit. Also, what kind testing has been done.

Yes, a bug(hang) encountered. After testing and analysis, I found the root cause. And it's NOT the dlm error case. The hang was fixed by the original patch. I will email you about the orabug and testcase(pretty easy to reproduce).

I will try http://people.redhat.com/~teigland/make_panic to see if it can find something.

thanks,
wengang.

Quoted 63 lines
Quoted 62 lines · Wengang Wang
Quoted 45 lines

When the lockres state UPCONVERT_FINISHING is cleared, we should wake up the downconvert thread incase that lockres is in the blocked queue. Currently we are not doing so and thus are at the mercy of another event waking up the dc thread.

Signed-off-by Wengang Wang<[email protected]>
---
  fs/ocfs2/dlmglue.c |    9 ++++++++-
  1 files changed, 8 insertions(+), 1 deletions(-)
fs/ocfs2/dlmglue.c +5 -0
index 7642d7c..524bd88 100644
@@ -1195,6 +1195,7 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
  						int convert)
  {
  	unsigned long flags;
+	int kick_dc;
 
  	spin_lock_irqsave(&lockres->l_lock, flags);
  	lockres_clear_flags(lockres, OCFS2_LOCK_BUSY);

@@ -1203,9 +1204,12 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
  		lockres->l_action = OCFS2_AST_INVALID;
  	else
  		lockres->l_unlock_action = OCFS2_UNLOCK_INVALID;
+	kick_dc = (lockres->l_flags&   OCFS2_LOCK_QUEUED);
  	spin_unlock_irqrestore(&lockres->l_lock, flags);
 
  	wake_up(&lockres->l_event);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(ocfs2_get_lockres_osb(lockres));
  }
 
  /* Note: If we detect another process working on the lock (i.e.,

@@ -1373,6 +1377,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
  	unsigned long flags;
  	unsigned int gen;
  	int noqueue_attempted = 0;
+	int kick_dc;
 
  	ocfs2_init_mask_waiter(&mw);
 

@@ -1500,8 +1505,10 @@ update_holders:
  	ret = 0;

fs/ocfs2/dlmglue.c update_holders: · line 1514

Quoted 10 lines
fs/ocfs2/dlmglue.c +3 -1
  unlock:
  	lockres_clear_flags(lockres, OCFS2_LOCK_UPCONVERT_FINISHING);
-
+	kick_dc = (lockres->l_flags&   OCFS2_LOCK_QUEUED);
  	spin_unlock_irqrestore(&lockres->l_lock, flags);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(osb);
  out:
  	/*
  	 * This is helping work around a lock inversion between the page lock

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

#4 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

Quoted 12 lines · Wengang Wang

Hi Sunil,

Quoted 9 lines · Sunil Mushran

http://people.redhat.com/~teigland/make_panic

This test has been useful in exposing dlmglue issues.

Quoted 4 lines · Sunil Mushran

I am fine with the kick in recover from dlm error. Not so in cluster lock. We have to be very very sure before meddling with that function. It is a state machine with many hidden gotchas.

Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

So is this patch for a bug encountered or just code audit. Also, what kind testing has been done.

Yes, a bug(hang) encountered. After testing and analysis, I found the root cause. And it's NOT the dlm error case. The hang was fixed by the original patch. I will email you about the orabug and testcase(pretty easy to reproduce).

Which original patch?

Quoted 2 lines

I will try http://people.redhat.com/~teigland/make_panic to see if it can find something.

author
#5 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

Quoted 13 lines · Sunil Mushran
Quoted 12 lines · Wengang Wang

Hi Sunil,

Quoted 9 lines · Sunil Mushran

http://people.redhat.com/~teigland/make_panic

This test has been useful in exposing dlmglue issues.

Quoted 4 lines · Sunil Mushran

I am fine with the kick in recover from dlm error. Not so in cluster lock. We have to be very very sure before meddling with that function. It is a state machine with many hidden gotchas.

Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

So is this patch for a bug encountered or just code audit. Also, what kind testing has been done.

Yes, a bug(hang) encountered. After testing and analysis, I found the root cause. And it's NOT the dlm error case. The hang was fixed by the original patch. I will email you about the orabug and testcase(pretty easy to reproduce).

Which original patch?

The first drop of this patch.

thanks,
wengang.

Quoted 4 lines
Quoted 4 lines

I will try http://people.redhat.com/~teigland/make_panic to see if it can find something.

maintainer
#6 lore raw

PATCH 1/1 Wakeup down-convert thread just after clearing OCFS2_LOCK_UPCONVERT_FINISHING -v3

In reply to Wengang Wang
Quoted 23 lines · Wengang Wang

When the lockres state UPCONVERT_FINISHING is cleared, we should wake up the downconvert thread incase that lockres is in the blocked queue. Currently we are not doing so and thus are at the mercy of another event waking up the dc thread.

Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dlmglue.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
fs/ocfs2/dlmglue.c +1 -0
index 7642d7c..524bd88 100644
@@ -1195,6 +1195,7 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
 						int convert)
 {
 	unsigned long flags;
+	int kick_dc;
 
 	spin_lock_irqsave(&lockres->l_lock, flags);
 	lockres_clear_flags(lockres, OCFS2_LOCK_BUSY);

fs/ocfs2/dlmglue.c static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres, · line 1207

Quoted 4 lines
fs/ocfs2/dlmglue.c +1 -0
@@ -1203,9 +1204,12 @@ static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres,
 		lockres->l_action = OCFS2_AST_INVALID;
 	else
 		lockres->l_unlock_action = OCFS2_UNLOCK_INVALID;
+	kick_dc = (lockres->l_flags & OCFS2_LOCK_QUEUED);

Point 1, you should be checking BLOCKED, which is how we
determine a lock that wants to downconvert.

fs/ocfs2/dlmglue.c static inline void ocfs2_recover_from_dlm_error(struct ocfs2_lock_res *lockres, · line 1212

Quoted 5 lines
fs/ocfs2/dlmglue.c +2 -0
 	spin_unlock_irqrestore(&lockres->l_lock, flags);
 
 	wake_up(&lockres->l_event);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(ocfs2_get_lockres_osb(lockres));

You're waking up the downconvert thread, but you don't know if
it needs it. You think it *might* need it. That's not OK in this code.

Quoted 12 lines
fs/ocfs2/dlmglue.c +1 -0
 }
 
 /* Note: If we detect another process working on the lock (i.e.,

@@ -1373,6 +1377,7 @@ static int __ocfs2_cluster_lock(struct ocfs2_super *osb,
 	unsigned long flags;
 	unsigned int gen;
 	int noqueue_attempted = 0;
+	int kick_dc;
 
 	ocfs2_init_mask_waiter(&mw);

fs/ocfs2/dlmglue.c update_holders: · line 1511

Quoted 8 lines
fs/ocfs2/dlmglue.c +3 -1
@@ -1500,8 +1505,10 @@ update_holders:
 	ret = 0;
 unlock:
 	lockres_clear_flags(lockres, OCFS2_LOCK_UPCONVERT_FINISHING);
-
+	kick_dc = (lockres->l_flags & OCFS2_LOCK_QUEUED);
 	spin_unlock_irqrestore(&lockres->l_lock, flags);
+	if (kick_dc)
+		ocfs2_wake_downconvert_thread(osb);

Same here. Frankly, if we were upconverting, the holder is
going to eventually unlock, which will wake the downconvert thread. That's how the machine works. See the comment where we set UPCONVERT_FINISHING. We want the upconverter to have enough quantum to get work done.
You're going to break that, because this thread might want to
get to EX, and it runs before the older thread updates its hold on the PR.

Thread 1 Thread 2 Remote
Asks for PR
Master grants PR
Asks for EX, setting BLOCKING on our node.
AST sets UPCONVERT
Asks for EX, sees
blocking on Remote
[kicks dc thread]
clears UPCONVERT
-->
_cluster_lock wakes,
clears UPCONVERT,
increments holders.

The part in [] is what you are changing. The --> is where the dc thread can steal the lock before Thread 1 increments the holder count. We're right back at the livelock that UPCONVERT_FINISHING was supposed to prevent.
I note that Thread 2 can still clear UPCONVERT and be caught by
the dc thread. But because we're not waking the dc thread, it's a very unlikely race. If you wake the dc thread, it's a guaranteed race. Also, in the current code, while it might race this time around, it doesn't the next time. No livelock.
This is why Sunil said this is a very sensitive state machine. Can I get the info on the problem you actually encountered? I
find it hard to believe the upconverter didn't eventually release his hold. That should wake the dc thread.

Joel

Signature · 7 lines
"But all my words come back to me
 In shades of mediocrity.
 Like emptiness in harmony
 I need someone to comfort me."

			http://www.jlbec.org/
			jlbec at evilplan.org