Skip to contentSkip to conversation

ocfs2/dlm: remove potential deadlock

PATCH New Wengang Wang 2 participants 5 messages Jun 16, 2010 – Jun 21, 2010 OCFS2

lore
Share & tools
lore

Wengang Wang

author
#0 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock

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. dlm_lockres_put() finally calls dlm_put() which take dlm_domain_lock.

The fix is moving the locking on dlm_domain_lock to dlm_ctxt_release() from dlm_put(). dlm_ctxt_release() is only called on the release of the last reference. Any path should not be holding dlm->spinlock when dropping the "last" reference.

Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +4 -8
index ab82add..754baf2 100644
@@ -321,28 +321,24 @@ static void dlm_ctxt_release(struct kref *kref)
 
 	dlm = container_of(kref, struct dlm_ctxt, dlm_refs);
 
+	if (spin_is_locked(&dlm->spinlock))
+		BUG();
 	BUG_ON(dlm->num_joins);
 	BUG_ON(dlm->dlm_state == DLM_CTXT_JOINED);
 
+	spin_lock(&dlm_domain_lock);
 	/* we may still be in the list if we hit an error during join. */
 	list_del_init(&dlm->list);
-
 	spin_unlock(&dlm_domain_lock);
 
-	mlog(0, "freeing memory from domain %s\n", dlm->name);
-
 	wake_up(&dlm_domain_events);
-
+	mlog(0, "freeing memory from domain %s\n", dlm->name);
 	dlm_free_ctxt_mem(dlm);
-
-	spin_lock(&dlm_domain_lock);
 }
 
 void dlm_put(struct dlm_ctxt *dlm)
 {
-	spin_lock(&dlm_domain_lock);
 	kref_put(&dlm->dlm_refs, dlm_ctxt_release);
-	spin_unlock(&dlm_domain_lock);
 }
 
 static void __dlm_get(struct dlm_ctxt *dlm)
Signature · 1 line
1.6.6.1
author
#1 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock

Why atomic operations on dlm_refs need spinlock's protect?

        /* NOTE: Next three are protected by dlm_domain_lock */
        struct kref dlm_refs;
        enum dlm_ctxt_state dlm_state;
        unsigned int num_joins;

regards,
wengang.

Quoted 63 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. dlm_lockres_put() finally calls dlm_put() which take dlm_domain_lock.

The fix is moving the locking on dlm_domain_lock to dlm_ctxt_release() from dlm_put(). dlm_ctxt_release() is only called on the release of the last reference. Any path should not be holding dlm->spinlock when dropping the "last" reference.

Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +4 -8
index ab82add..754baf2 100644
@@ -321,28 +321,24 @@ static void dlm_ctxt_release(struct kref *kref)
 
 	dlm = container_of(kref, struct dlm_ctxt, dlm_refs);
 
+	if (spin_is_locked(&dlm->spinlock))
+		BUG();
 	BUG_ON(dlm->num_joins);
 	BUG_ON(dlm->dlm_state == DLM_CTXT_JOINED);
 
+	spin_lock(&dlm_domain_lock);
 	/* we may still be in the list if we hit an error during join. */
 	list_del_init(&dlm->list);
-
 	spin_unlock(&dlm_domain_lock);
 
-	mlog(0, "freeing memory from domain %s\n", dlm->name);
-
 	wake_up(&dlm_domain_events);
-
+	mlog(0, "freeing memory from domain %s\n", dlm->name);
 	dlm_free_ctxt_mem(dlm);
-
-	spin_lock(&dlm_domain_lock);
 }
 
 void dlm_put(struct dlm_ctxt *dlm)
 {
-	spin_lock(&dlm_domain_lock);
 	kref_put(&dlm->dlm_refs, dlm_ctxt_release);
-	spin_unlock(&dlm_domain_lock);
 }
 
 static void __dlm_get(struct dlm_ctxt *dlm)
Signature · 7 lines
1.6.6.1


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

PATCH 1/1 ocfs2/dlm: remove potential deadlock

hi wengang,

Quoted 6 lines · Wengang Wang

Why atomic operations on dlm_refs need spinlock's protect?

         /* NOTE: Next three are protected by dlm_domain_lock */
         struct kref dlm_refs;
         enum dlm_ctxt_state dlm_state;
         unsigned int num_joins;

I don't dive into the code, so if there is something wrong, please correct me.
AFAICS, kref_put isn't an atomic operation, it need to call release to free the container of kref.

Regards,
Tao

Quoted 73 lines

regards,
wengang.

Quoted 59 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. dlm_lockres_put() finally calls dlm_put() which take dlm_domain_lock.

The fix is moving the locking on dlm_domain_lock to dlm_ctxt_release() from dlm_put(). dlm_ctxt_release() is only called on the release of the last reference. Any path should not be holding dlm->spinlock when dropping the "last" reference.

Signed-off-by Wengang Wang<[email protected]>
---
  fs/ocfs2/dlm/dlmdomain.c |   12 ++++--------
  1 files changed, 4 insertions(+), 8 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +4 -9
index ab82add..754baf2 100644
@@ -321,28 +321,24 @@ static void dlm_ctxt_release(struct kref *kref)
 
  	dlm = container_of(kref, struct dlm_ctxt, dlm_refs);
 
+	if (spin_is_locked(&dlm->spinlock))
+		BUG();
  	BUG_ON(dlm->num_joins);
  	BUG_ON(dlm->dlm_state == DLM_CTXT_JOINED);
 
+	spin_lock(&dlm_domain_lock);
  	/* we may still be in the list if we hit an error during join. */
  	list_del_init(&dlm->list);
-
  	spin_unlock(&dlm_domain_lock);
 
-	mlog(0, "freeing memory from domain %s\n", dlm->name);
-
  	wake_up(&dlm_domain_events);
-
+	mlog(0, "freeing memory from domain %s\n", dlm->name);
  	dlm_free_ctxt_mem(dlm);
-
-	spin_lock(&dlm_domain_lock);
  }
 
  void dlm_put(struct dlm_ctxt *dlm)
  {
-	spin_lock(&dlm_domain_lock);
  	kref_put(&dlm->dlm_refs, dlm_ctxt_release);
-	spin_unlock(&dlm_domain_lock);
  }
 
  static void __dlm_get(struct dlm_ctxt *dlm)
--

1.6.6.1

Quoted 4 lines

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

_______________________________________________
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 ocfs2/dlm: remove potential deadlock

Quoted 5 lines · Tao Ma

hi wengang,

Quoted 2 lines · Wengang Wang

Why atomic operations on dlm_refs need spinlock's protect?

Quoted 8 lines
Quoted 4 lines
        /* NOTE: Next three are protected by dlm_domain_lock */
        struct kref dlm_refs;
        enum dlm_ctxt_state dlm_state;
        unsigned int num_joins;

I don't dive into the code, so if there is something wrong, please correct me.
AFAICS, kref_put isn't an atomic operation, it need to call release to free the container of kref.

Yes, it's bad :-(.

regards,
wengang.

Quoted 76 lines

Regards,
Tao

Quoted 73 lines

regards,
wengang.

Quoted 59 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. dlm_lockres_put() finally calls dlm_put() which take dlm_domain_lock.

The fix is moving the locking on dlm_domain_lock to dlm_ctxt_release() from dlm_put(). dlm_ctxt_release() is only called on the release of the last reference. Any path should not be holding dlm->spinlock when dropping the "last" reference.

Signed-off-by Wengang Wang<[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +4 -9
index ab82add..754baf2 100644
@@ -321,28 +321,24 @@ static void dlm_ctxt_release(struct kref *kref)
 
 	dlm = container_of(kref, struct dlm_ctxt, dlm_refs);
 
+	if (spin_is_locked(&dlm->spinlock))
+		BUG();
 	BUG_ON(dlm->num_joins);
 	BUG_ON(dlm->dlm_state == DLM_CTXT_JOINED);
 
+	spin_lock(&dlm_domain_lock);
 	/* we may still be in the list if we hit an error during join. */
 	list_del_init(&dlm->list);
-
 	spin_unlock(&dlm_domain_lock);
 
-	mlog(0, "freeing memory from domain %s\n", dlm->name);
-
 	wake_up(&dlm_domain_events);
-
+	mlog(0, "freeing memory from domain %s\n", dlm->name);
 	dlm_free_ctxt_mem(dlm);
-
-	spin_lock(&dlm_domain_lock);
 }
 
 void dlm_put(struct dlm_ctxt *dlm)
 {
-	spin_lock(&dlm_domain_lock);
 	kref_put(&dlm->dlm_refs, dlm_ctxt_release);
-	spin_unlock(&dlm_domain_lock);
 }
 
 static void __dlm_get(struct dlm_ctxt *dlm)
--

1.6.6.1

Quoted 4 lines

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

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

author
#4 lore raw

PATCH 1/1 ocfs2/dlm: remove potential deadlock

In reply to Wengang Wang

This patch is not good, please ignore it. I will post a revised one.

regards,
wengang.

Quoted 63 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. dlm_lockres_put() finally calls dlm_put() which take dlm_domain_lock.

The fix is moving the locking on dlm_domain_lock to dlm_ctxt_release() from dlm_put(). dlm_ctxt_release() is only called on the release of the last reference. Any path should not be holding dlm->spinlock when dropping the "last" reference.

Signed-off-by Wengang Wang <[email protected]>
---
 fs/ocfs2/dlm/dlmdomain.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)
fs/ocfs2/dlm/dlmdomain.c +4 -8
index ab82add..754baf2 100644
@@ -321,28 +321,24 @@ static void dlm_ctxt_release(struct kref *kref)
 
 	dlm = container_of(kref, struct dlm_ctxt, dlm_refs);
 
+	if (spin_is_locked(&dlm->spinlock))
+		BUG();
 	BUG_ON(dlm->num_joins);
 	BUG_ON(dlm->dlm_state == DLM_CTXT_JOINED);
 
+	spin_lock(&dlm_domain_lock);
 	/* we may still be in the list if we hit an error during join. */
 	list_del_init(&dlm->list);
-
 	spin_unlock(&dlm_domain_lock);
 
-	mlog(0, "freeing memory from domain %s\n", dlm->name);
-
 	wake_up(&dlm_domain_events);
-
+	mlog(0, "freeing memory from domain %s\n", dlm->name);
 	dlm_free_ctxt_mem(dlm);
-
-	spin_lock(&dlm_domain_lock);
 }
 
 void dlm_put(struct dlm_ctxt *dlm)
 {
-	spin_lock(&dlm_domain_lock);
 	kref_put(&dlm->dlm_refs, dlm_ctxt_release);
-	spin_unlock(&dlm_domain_lock);
 }
 
 static void __dlm_get(struct dlm_ctxt *dlm)
Signature · 7 lines
1.6.6.1


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