ocfs2/dlm: remove potential deadlock -V3

PATCH New Wengang Wang 3 participants 6 messages Jul 30, 2010 – Aug 7, 2010 OCFS2

Share & tools

Wengang Wang

author

2010-07-30 13:53 UTC #0 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.

There is pathes disobey this order. That is calling dlm_lockres_put() with dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at the ref and dlm_put() locks on dlm_domain_lock.

Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dlm/dlmmaster.c |    3 ---
 1 files changed, 0 insertions(+), 3 deletions(-)

fs/ocfs2/dlm/dlmmaster.c +0 -3

index 94b97fc..8408465 100644
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
 	atomic_dec(&dlm->res_cur_count);
 
-	dlm_put(dlm);
-
 	if (!hlist_unhashed(&res->hash_node) ||
 	    !list_empty(&res->granted) ||
 	    !list_empty(&res->converting) ||

@@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 	res->inflight_locks = 0;
 
 	/* put in dlm_lockres_release */
-	dlm_grab(dlm);
 	res->dlm = dlm;
 
 	kref_init(&res->refs);

Signature · 1 line

1.7.1.1

Sunil Mushran

2010-07-30 14:45 UTC #1 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

You can also remove the comment that talks about the put.

Quoted 46 lines · Wengang Wang

When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.
There is pathes disobey this order. That is calling dlm_lockres_put() with dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at the ref and dlm_put() locks on dlm_domain_lock.
Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.
Signed-off-by Wengang Wang <[email protected]>
---
fs/ocfs2/dlm/dlmmaster.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +0 -3
index 94b97fc..8408465 100644
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
   atomic_dec(&dlm->res_cur_count);
 
-    dlm_put(dlm);
-
   if (!hlist_unhashed(&res->hash_node) ||
       !list_empty(&res->granted) ||
       !list_empty(&res->converting) ||
@@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
   res->inflight_locks = 0;
 
   /* put in dlm_lockres_release */
-    dlm_grab(dlm);
   res->dlm = dlm;
 
   kref_init(&res->refs);
Signature · 7 lines
1.7.1.1


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

Wengang Wang

author

2010-07-30 15:08 UTC #2 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

Quoted 1 line · Sunil Mushran

You can also remove the comment that talks about the put.

Yes, I should.

regards,
wengang.

Quoted 49 lines · Wengang Wang

Quoted 47 lines
When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.
There is pathes disobey this order. That is calling dlm_lockres_put() with dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at the ref and dlm_put() locks on dlm_domain_lock.
Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.
Signed-off-by Wengang Wang <[email protected]>
---
fs/ocfs2/dlm/dlmmaster.c | 3 ---
1 files changed, 0 insertions(+), 3 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +0 -3
index 94b97fc..8408465 100644
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
   atomic_dec(&dlm->res_cur_count);
 
-    dlm_put(dlm);
-
   if (!hlist_unhashed(&res->hash_node) ||
       !list_empty(&res->granted) ||
       !list_empty(&res->converting) ||
@@ -586,7 +584,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
   res->inflight_locks = 0;
 
   /* put in dlm_lockres_release */
-    dlm_grab(dlm);
   res->dlm = dlm;
 
   kref_init(&res->refs);
Signature · 7 lines
1.7.1.1


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

Wengang Wang

author

2010-07-30 15:18 UTC #3 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

In reply to Sunil Mushran

When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.

Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.

Signed-off-by Wengang Wang <[email protected]>

---
 fs/ocfs2/dlm/dlmmaster.c |    4 ----
 1 file changed, 4 deletions(-)

fs/ocfs2/dlm/dlmmaster.c +0 -4

index 94b97fc..0fe7c4d 100644
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
 	atomic_dec(&dlm->res_cur_count);
 
-	dlm_put(dlm);
-
 	if (!hlist_unhashed(&res->hash_node) ||
 	    !list_empty(&res->granted) ||
 	    !list_empty(&res->converting) ||

@@ -585,8 +583,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
 	res->migration_pending = 0;
 	res->inflight_locks = 0;
 
-	/* put in dlm_lockres_release */
-	dlm_grab(dlm);
 	res->dlm = dlm;
 
 	kref_init(&res->refs);

Signature · 1 line

1.7.1.1

Sunil Mushran

2010-07-30 17:06 UTC #4 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

Acked-by Sunil Mushran <[email protected]>

Quoted 40 lines · Wengang Wang

When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.
There is pathes disobey this order. That is calling dlm_lockres_put() with dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at the ref and dlm_put() locks on dlm_domain_lock.
Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.
Signed-off-by Wengang Wang<[email protected]>
---
  fs/ocfs2/dlm/dlmmaster.c |    4 ----
  1 file changed, 4 deletions(-)
fs/ocfs2/dlm/dlmmaster.c +0 -4
index 94b97fc..0fe7c4d 100644
@@ -511,8 +511,6 @@ static void dlm_lockres_release(struct kref *kref)
 
  	atomic_dec(&dlm->res_cur_count);
 
-	dlm_put(dlm);
-
  	if (!hlist_unhashed(&res->hash_node) ||
  	    !list_empty(&res->granted) ||
  	    !list_empty(&res->converting) ||
@@ -585,8 +583,6 @@ static void dlm_init_lockres(struct dlm_ctxt *dlm,
  	res->migration_pending = 0;
  	res->inflight_locks = 0;
 
-	/* put in dlm_lockres_release */
-	dlm_grab(dlm);
  	res->dlm = dlm;
 
  	kref_init(&res->refs);

Joel Becker

2010-08-07 18:40 UTC #5 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock -V3

In reply to Wengang Wang

Quoted 7 lines · Wengang Wang

When we need to take both dlm_domain_lock and dlm->spinlock, we should take them in order of: dlm_domain_lock then dlm->spinlock.
There is pathes disobey this order. That is calling dlm_lockres_put() with dlm->spinlock held in dlm_run_purge_list. dlm_lockres_put() calls dlm_put() at the ref and dlm_put() locks on dlm_domain_lock.

Quoted 6 lines

Fix:
Don't grab/put the dlm when the initialising/releasing lockres. That grab is not required because we don't call dlm_unregister_domain() based on refcount.
Signed-off-by Wengang Wang <[email protected]>

This patch is now in the fixes branch of ocfs2.git.

Joel

Signature · 9 lines

"If the human brain were so simple we could understand it, we would
 be so simple that we could not."
	- W. A. Clouston

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127