Re: [Ocfs2-devel] [PATCH 15/17] fs/ocfs2/dlm: Add missing spin_unlock
On Wed, May 26, 2010 at 05:58:53PM +0200, Julia Lawall wrote:
From: Julia Lawall ju...@diku.dk
Add a spin_unlock missing on the error path. Unlock as in the other code
that leads to the leave label.
The semantic match that finds this problem is as follows:
(http://coccinelle.lip6.fr/)
// smpl
@@
expression E1;
@@
* spin_lock(E1,...);
+... when != E1
if (...) {
... when != E1
* return ...;
}
...+
* spin_unlock(E1,...);
// /smpl
Signed-off-by: Julia Lawall ju...@diku.dk
This patch is now in the 'fixes' branch of ocfs2.git.
Joel
--
In the arms of the angel, fly away from here,
From this dark, cold hotel room and the endlessness that you fear.
You are pulled from the wreckage of your silent reverie.
In the arms of the angel, may you find some comfort here.
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.bec...@oracle.com
Phone: (650) 506-8127
___
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
Re: [Ocfs2-devel] [PATCH] ocfs2: Move orphan scan work to ocfs2_wq.
On Fri, May 28, 2010 at 02:22:59PM +0800, Tao Ma wrote: We used to let orphan scan work in the default work queue, but there is a corner case which will make the system deadlock. The scenario is like this: 1. set heartbeat threadshold to 200. this will allow us to have a great chance to have a orphan scan work before our quorum decision. 2. mount node 1. 3. after 1~2 minutes, mount node 2(in order to make the bug easier to reproduce, better add maxcpus=1 to kernel command line). 4. node 1 do orphan scan work. 5. node 2 do orphan scan work. 6. node 1 do orphan scan work. After this, node 1 hold the orphan scan lock while node 2 know node 1 is the master. 7. ifdown eth2 in node 2(eth2 is what we do ocfs2 interconnection). Now when node 2 begins orphan scan, the system queue is blocked. The root cause is that both orphan scan work and quorum decision work will use the system event work queue. orphan scan has a chance of blocking the event work queue(in dlm_wait_for_node_death) so that there is no chance for quorum decision work to proceed. This patch resolve it by moving orphan scan work to ocfs2_wq. Signed-off-by: Tao Ma tao...@oracle.com This patch is now in the 'fixes' branch of ocfs2.git Joel -- The one important thing i have learned over the years is the difference between taking one's work seriously and taking one's self seriously. The first is imperative and the second is disastrous. -Margot Fonteyn Joel Becker Principal Software Developer Oracle E-mail: joel.bec...@oracle.com Phone: (650) 506-8127 ___ Ocfs2-devel mailing list Ocfs2-devel@oss.oracle.com http://oss.oracle.com/mailman/listinfo/ocfs2-devel
[Ocfs2-devel] [PATCH 2/2] ocfs2: o2dlm fix race in purge lockres and newlock (orabug 9094491)
This patch fixes the following hole.
dlmlock tries to create a new lock on a lockres that is on purge list. It calls
dlm_get_lockresource and later adds a lock to blocked list. But in this window,
dlm_thread can purge the lockres and unhash it. This will cause a BUG, as when
the AST comes back from the master lockres is not found
This patch marks the lockres with a new state DLM_LOCK_RES_IN_USE which would
protect lockres from dlm_thread purging it.
Signed-off-by: Srinivas Eeda srinivas.e...@oracle.com
---
fs/ocfs2/dlm/dlmcommon.h |1 +
fs/ocfs2/dlm/dlmlock.c |4
fs/ocfs2/dlm/dlmmaster.c |5 -
fs/ocfs2/dlm/dlmthread.c |1 +
4 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/fs/ocfs2/dlm/dlmcommon.h b/fs/ocfs2/dlm/dlmcommon.h
index 0102be3..6cf3c08 100644
--- a/fs/ocfs2/dlm/dlmcommon.h
+++ b/fs/ocfs2/dlm/dlmcommon.h
@@ -279,6 +279,7 @@ static inline void __dlm_set_joining_node(struct dlm_ctxt
*dlm,
#define DLM_LOCK_RES_DIRTY0x0008
#define DLM_LOCK_RES_IN_PROGRESS 0x0010
#define DLM_LOCK_RES_MIGRATING0x0020
+#define DLM_LOCK_RES_IN_USE 0x0100
#define DLM_LOCK_RES_DROPPING_REF 0x0040
#define DLM_LOCK_RES_BLOCK_DIRTY 0x1000
#define DLM_LOCK_RES_SETREF_INPROG0x2000
diff --git a/fs/ocfs2/dlm/dlmlock.c b/fs/ocfs2/dlm/dlmlock.c
index 777..501ac40 100644
--- a/fs/ocfs2/dlm/dlmlock.c
+++ b/fs/ocfs2/dlm/dlmlock.c
@@ -134,6 +134,8 @@ static enum dlm_status dlmlock_master(struct dlm_ctxt *dlm,
if (status != DLM_NORMAL
lock-ml.node != dlm-node_num) {
/* erf. state changed after lock was dropped. */
+ /* DLM_LOCK_RES_IN_USE is set in dlm_get_lock_resource */
+ res-state = ~DLM_LOCK_RES_IN_USE;
spin_unlock(res-spinlock);
dlm_error(status);
return status;
@@ -180,6 +182,7 @@ static enum dlm_status dlmlock_master(struct dlm_ctxt *dlm,
kick_thread = 1;
}
}
+ res-state = ~DLM_LOCK_RES_IN_USE;
/* reduce the inflight count, this may result in the lockres
* being purged below during calc_usage */
if (lock-ml.node == dlm-node_num)
@@ -246,6 +249,7 @@ static enum dlm_status dlmlock_remote(struct dlm_ctxt *dlm,
spin_lock(res-spinlock);
res-state = ~DLM_LOCK_RES_IN_PROGRESS;
+ res-state = ~DLM_LOCK_RES_IN_USE;
lock-lock_pending = 0;
if (status != DLM_NORMAL) {
if (status == DLM_RECOVERING
diff --git a/fs/ocfs2/dlm/dlmmaster.c b/fs/ocfs2/dlm/dlmmaster.c
index 9289b43..f0f2d97 100644
--- a/fs/ocfs2/dlm/dlmmaster.c
+++ b/fs/ocfs2/dlm/dlmmaster.c
@@ -719,6 +719,7 @@ lookup:
if (tmpres) {
int dropping_ref = 0;
+ tmpres-state |= DLM_LOCK_RES_IN_USE;
spin_unlock(dlm-spinlock);
spin_lock(tmpres-spinlock);
@@ -731,8 +732,10 @@ lookup:
if (tmpres-owner == dlm-node_num) {
BUG_ON(tmpres-state DLM_LOCK_RES_DROPPING_REF);
dlm_lockres_grab_inflight_ref(dlm, tmpres);
- } else if (tmpres-state DLM_LOCK_RES_DROPPING_REF)
+ } else if (tmpres-state DLM_LOCK_RES_DROPPING_REF) {
+ tmpres-state = ~DLM_LOCK_RES_IN_USE;
dropping_ref = 1;
+ }
spin_unlock(tmpres-spinlock);
/* wait until done messaging the master, drop our ref to allow
diff --git a/fs/ocfs2/dlm/dlmthread.c b/fs/ocfs2/dlm/dlmthread.c
index fb0be6c..2b82e0b 100644
--- a/fs/ocfs2/dlm/dlmthread.c
+++ b/fs/ocfs2/dlm/dlmthread.c
@@ -93,6 +93,7 @@ int __dlm_lockres_has_locks(struct dlm_lock_resource *res)
int __dlm_lockres_unused(struct dlm_lock_resource *res)
{
if (!__dlm_lockres_has_locks(res)
+ !(res-state DLM_LOCK_RES_IN_USE)
(list_empty(res-dirty) !(res-state DLM_LOCK_RES_DIRTY))) {
/* try not to scan the bitmap unless the first two
* conditions are already true */
--
1.5.6.5
___
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
Re: [Ocfs2-devel] [PATCH] ocfs2/dlm: check dlm_state under spinlock
The lock order in this code causes dead lock, not caused by your patch.
The lock order in dlm_query_join_handler is
dlm_domain_lock
-dlm-spinlock
dead locks with ..
dlm_lockres_put calls dlm_lockres_release while holding dlm-spinlock
which calls dlm_put which gets dlm_domain_lock. So the spin lock order
here is
dlm-spinlock
- dlm_domain_lock
On 6/15/2010 9:08 PM, Wengang Wang wrote:
We should check dlm-dlm_state under dlm-spinlock though maybe in this case
it
doesn't hurt.
Signed-off-by: Wengang Wang wen.gang.w...@oracle.com
---
fs/ocfs2/dlm/dlmdomain.c | 13 ++---
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
index 6b5a492..ab82add 100644
--- a/fs/ocfs2/dlm/dlmdomain.c
+++ b/fs/ocfs2/dlm/dlmdomain.c
@@ -796,7 +796,7 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
spin_lock(dlm_domain_lock);
dlm = __dlm_lookup_domain_full(query-domain, query-name_len);
if (!dlm)
- goto unlock_respond;
+ goto unlock_domain_respond;
/*
* There is a small window where the joining node may not see the
@@ -811,7 +811,7 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
have node %u in its nodemap\n,
query-node_idx, nodenum);
packet.code = JOIN_DISALLOW;
- goto unlock_respond;
+ goto unlock_domain_respond;
}
}
nodenum++;
@@ -821,9 +821,9 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
* to be put in someone's domain map.
* Also, explicitly disallow joining at certain troublesome
* times (ie. during recovery). */
- if (dlm dlm-dlm_state != DLM_CTXT_LEAVING) {
+ spin_lock(dlm-spinlock);
+ if (dlm-dlm_state != DLM_CTXT_LEAVING) {
int bit = query-node_idx;
- spin_lock(dlm-spinlock);
if (dlm-dlm_state == DLM_CTXT_NEW
dlm-joining_node == DLM_LOCK_RES_OWNER_UNKNOWN) {
@@ -869,10 +869,9 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
__dlm_set_joining_node(dlm, query-node_idx);
}
}
-
- spin_unlock(dlm-spinlock);
}
-unlock_respond:
+ spin_unlock(dlm-spinlock);
+unlock_domain_respond:
spin_unlock(dlm_domain_lock);
respond:
___
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
Re: [Ocfs2-devel] [PATCH] ocfs2/dlm: check dlm_state under spinlock
Thanks for review :)
The second path has problem. I think the correct order should be dlm_domain_lock
and then dlm-spinlock.
I will try to fix it.
regards,
wengang.
On 10-06-15 22:00, Srinivas Eeda wrote:
The lock order in this code causes dead lock, not caused by your
patch. The lock order in dlm_query_join_handler is
dlm_domain_lock
-dlm-spinlock
dead locks with ..
dlm_lockres_put calls dlm_lockres_release while holding
dlm-spinlock which calls dlm_put which gets dlm_domain_lock. So the
spin lock order here is
dlm-spinlock
- dlm_domain_lock
On 6/15/2010 9:08 PM, Wengang Wang wrote:
We should check dlm-dlm_state under dlm-spinlock though maybe in this case
it
doesn't hurt.
Signed-off-by: Wengang Wang wen.gang.w...@oracle.com
---
fs/ocfs2/dlm/dlmdomain.c | 13 ++---
1 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/fs/ocfs2/dlm/dlmdomain.c b/fs/ocfs2/dlm/dlmdomain.c
index 6b5a492..ab82add 100644
--- a/fs/ocfs2/dlm/dlmdomain.c
+++ b/fs/ocfs2/dlm/dlmdomain.c
@@ -796,7 +796,7 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
spin_lock(dlm_domain_lock);
dlm = __dlm_lookup_domain_full(query-domain, query-name_len);
if (!dlm)
-goto unlock_respond;
+goto unlock_domain_respond;
/*
* There is a small window where the joining node may not see the
@@ -811,7 +811,7 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
have node %u in its nodemap\n,
query-node_idx, nodenum);
packet.code = JOIN_DISALLOW;
-goto unlock_respond;
+goto unlock_domain_respond;
}
}
nodenum++;
@@ -821,9 +821,9 @@ static int dlm_query_join_handler(struct o2net_msg *msg,
u32 len, void *data,
* to be put in someone's domain map.
* Also, explicitly disallow joining at certain troublesome
* times (ie. during recovery). */
-if (dlm dlm-dlm_state != DLM_CTXT_LEAVING) {
+spin_lock(dlm-spinlock);
+if (dlm-dlm_state != DLM_CTXT_LEAVING) {
int bit = query-node_idx;
-spin_lock(dlm-spinlock);
if (dlm-dlm_state == DLM_CTXT_NEW
dlm-joining_node == DLM_LOCK_RES_OWNER_UNKNOWN) {
@@ -869,10 +869,9 @@ static int dlm_query_join_handler(struct o2net_msg
*msg, u32 len, void *data,
__dlm_set_joining_node(dlm, query-node_idx);
}
}
-
-spin_unlock(dlm-spinlock);
}
-unlock_respond:
+spin_unlock(dlm-spinlock);
+unlock_domain_respond:
spin_unlock(dlm_domain_lock);
respond:
___
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
