ocfs2/dlm: check dlm_state under spinlock

We should check dlm->dlm_state under dlm->spinlock though maybe in this case it doesn't hurt.

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

---
 fs/ocfs2/dlm/dlmdomain.c |   13 ++++++-------
 1 files changed, 6 insertions(+), 7 deletions(-)

fs/ocfs2/dlm/dlmdomain.c +6 -7

index 6b5a492..ab82add 100644
@@ -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:

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

Quoted 52 lines · Wengang Wang

We should check dlm->dlm_state under dlm->spinlock though maybe in this case it doesn't hurt.

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

---
fs/ocfs2/dlm/dlmdomain.c | 13 ++++++-------

1 files changed, 6 insertions(+), 7 deletions(-)

fs/ocfs2/dlm/dlmdomain.c +6 -7

index 6b5a492..ab82add 100644
@@ -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:

We should check dlm->dlm_state under dlm->spinlock though maybe in this case it doesn't hurt.

Quoted 2 lines · Wengang Wang

We should check dlm->dlm_state under dlm->spinlock though maybe in this case it doesn't hurt.

NAK.

dlm->dlm_state is protected by dlm_domain_lock which is held at that time.

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