o2dlm: check lockres validity in createlock

When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).

While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.

I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner in dlmlock_remote(), the owner crashed. So we get DLM_RECOVERING as return value and retries dlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres become stale(on lockres->onwer). Thus the code calls dlmlock_remote() infinitely.

fix 1
This patch tries to fix it by checking if the lockres is still valid(in hash table) before make request on it for each retry. It passed regression test.

fix 2
Another approch(not in this patch) is that we remove the lockres from purge list if it's there in dlm_get_lock_resource() which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock. I didn't test it.

I think fix 1 is the safer and fix 2 is the better. I choosed fix 1.

Sunil, what's your comment?

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

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

fs/ocfs2/dlm/dlmlock.c +28 -0

index 8d39e0f..02abaa1 100644
@@ -570,6 +570,7 @@ enum dlm_status dlmlock(struct dlm_ctxt *dlm, int mode,
 	struct dlm_lock_resource *res = NULL;
 	struct dlm_lock *lock = NULL;
 	int convert = 0, recovery = 0;
+	bool lockres_purged = false;
 
 	/* yes this function is a mess.
 	 * TODO: clean this up.  lots of common code in the

@@ -681,6 +682,7 @@ retry_convert:
 		if (!recovery)
 			dlm_wait_for_recovery(dlm);
 
+lookup_res:
 		/* find or create the lock resource */
 		res = dlm_get_lock_resource(dlm, name, namelen, flags);
 		if (!res) {

@@ -698,6 +700,32 @@ retry_convert:
 		lock->astdata = data;
 
 retry_lock:
+		/* there is a chance that the lockres is already be purged out
+		 * from the hash table and it can become a stale one. obviously
+		 * accessing on a stale lockres is not what we want. the case
+		 * we ever see is the owner dead(recovery didn't take care of
+		 * this lockres since it's not in hashtable), and the code keep
+		 * sending request to the dead node and getting DLM_RECOVERING
+		 * and then retrying infinitely.
+		 * we check if the lockres has been purged here and redo the
+		 * lookup if it has been to avoid infinite loop.
+		 */
+		lockres_purged = false;
+		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
+		if (unlikely(hlist_unhashed(&res->hash_node))) {
+			lockres_purged = true;
+		}
+		spin_unlock(&res->spinlock);
+		spin_unlock(&dlm->spinlock);
+		if (lockres_purged) {
+			dlm_lock_detach_lockres(lock);
+			mlog(ML_NOTICE, "going to relookup lockres %.*s\n", res->lockname.len, res->lockname.name);
+			dlm_lockres_put(res);
+			res = NULL;
+			goto lookup_res;
+		}
+
 		if (flags & LKM_VALBLK) {
 			mlog(0, "LKM_VALBLK passed by caller\n");
 

Signature · 7 lines

1.7.2.3


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

When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).

While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.

I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner in dlmlock_remote(), the owner crashed. So we get DLM_RECOVERING as return value and retries dlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres become stale(on lockres->onwer). Thus the code calls dlmlock_remote() infinitely.

fix 1
This patch tries to fix it by checking if the lockres is still valid(in hash table) before make request on it for each retry. It passed regression test.

fix 2
Another approch(not in this patch) is that we remove the lockres from purge list if it's there in dlm_get_lock_resource() which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock. I didn't test it.

I think fix 1 is the safer and fix 2 is the better. I choosed fix 1.

Sunil, what's your comment?

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

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

fs/ocfs2/dlm/dlmlock.c +28 -0

index 8d39e0f..02abaa1 100644
@@ -570,6 +570,7 @@ enum dlm_status dlmlock(struct dlm_ctxt *dlm, int mode,
  	struct dlm_lock_resource *res = NULL;
  	struct dlm_lock *lock = NULL;
  	int convert = 0, recovery = 0;
+	bool lockres_purged = false;
 
  	/* yes this function is a mess.
  	 * TODO: clean this up.  lots of common code in the

@@ -681,6 +682,7 @@ retry_convert:
  		if (!recovery)
  			dlm_wait_for_recovery(dlm);
 
+lookup_res:
  		/* find or create the lock resource */
  		res = dlm_get_lock_resource(dlm, name, namelen, flags);
  		if (!res) {

@@ -698,6 +700,32 @@ retry_convert:
  		lock->astdata = data;
 
  retry_lock:
+		/* there is a chance that the lockres is already be purged out
+		 * from the hash table and it can become a stale one. obviously
+		 * accessing on a stale lockres is not what we want. the case
+		 * we ever see is the owner dead(recovery didn't take care of
+		 * this lockres since it's not in hashtable), and the code keep
+		 * sending request to the dead node and getting DLM_RECOVERING
+		 * and then retrying infinitely.
+		 * we check if the lockres has been purged here and redo the
+		 * lookup if it has been to avoid infinite loop.
+		 */
+		lockres_purged = false;
+		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
+		if (unlikely(hlist_unhashed(&res->hash_node))) {
+			lockres_purged = true;
+		}
+		spin_unlock(&res->spinlock);
+		spin_unlock(&dlm->spinlock);
+		if (lockres_purged) {
+			dlm_lock_detach_lockres(lock);
+			mlog(ML_NOTICE, "going to relookup lockres %.*s\n", res->lockname.len, res->lockname.name);
+			dlm_lockres_put(res);
+			res = NULL;
+			goto lookup_res;
+		}
+
  		if (flags&  LKM_VALBLK) {
  			mlog(0, "LKM_VALBLK passed by caller\n");
 

Quoted 9 lines · Wengang Wang

When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).

Quoted 5 lines

While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.

Create lock follows master query. And in master query handler we add the node to the refmap. That's the race refmap was created to close. Meaning we should not run into this condition.

Which version did this problem reproduce in?

Quoted 7 lines

I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner in dlmlock_remote(), the owner crashed. So we get DLM_RECOVERING as return value and retries dlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres become stale(on lockres->onwer). Thus the code calls dlmlock_remote() infinitely.

Quoted 62 lines

fix 1
This patch tries to fix it by checking if the lockres is still valid(in hash table) before make request on it for each retry. It passed regression test.

fix 2
Another approch(not in this patch) is that we remove the lockres from purge list if it's there in dlm_get_lock_resource() which is called for only createlock case. So that the lockres can't be purged when we are in progress of createlock. I didn't test it.

I think fix 1 is the safer and fix 2 is the better. I choosed fix 1.

Sunil, what's your comment?

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

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

fs/ocfs2/dlm/dlmlock.c +21 -0

index 8d39e0f..02abaa1 100644
@@ -570,6 +570,7 @@ enum dlm_status dlmlock(struct dlm_ctxt *dlm, int mode,
   	struct dlm_lock_resource *res = NULL;
   	struct dlm_lock *lock = NULL;
   	int convert = 0, recovery = 0;
+	bool lockres_purged = false;
 
   	/* yes this function is a mess.
   	 * TODO: clean this up.  lots of common code in the

@@ -681,6 +682,7 @@ retry_convert:
   		if (!recovery)
   			dlm_wait_for_recovery(dlm);
 
+lookup_res:
   		/* find or create the lock resource */
   		res = dlm_get_lock_resource(dlm, name, namelen, flags);
   		if (!res) {

@@ -698,6 +700,32 @@ retry_convert:
   		lock->astdata = data;
 
   retry_lock:
+		/* there is a chance that the lockres is already be purged out
+		 * from the hash table and it can become a stale one. obviously
+		 * accessing on a stale lockres is not what we want. the case
+		 * we ever see is the owner dead(recovery didn't take care of
+		 * this lockres since it's not in hashtable), and the code keep
+		 * sending request to the dead node and getting DLM_RECOVERING
+		 * and then retrying infinitely.
+		 * we check if the lockres has been purged here and redo the
+		 * lookup if it has been to avoid infinite loop.
+		 */
+		lockres_purged = false;
+		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
+		if (unlikely(hlist_unhashed(&res->hash_node))) {
+			lockres_purged = true;
+		}
+		spin_unlock(&res->spinlock);
+		spin_unlock(&dlm->spinlock);
+		if (lockres_purged) {

fs/ocfs2/dlm/dlmlock.c retry_convert: · line 731

Quoted 10 lines

fs/ocfs2/dlm/dlmlock.c +7 -0

+			dlm_lock_detach_lockres(lock);
+			mlog(ML_NOTICE, "going to relookup lockres %.*s\n", res->lockname.len, res->lockname.name);
+			dlm_lockres_put(res);
+			res = NULL;
+			goto lookup_res;
+		}
+
   		if (flags&   LKM_VALBLK) {
   			mlog(0, "LKM_VALBLK passed by caller\n");
 

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

Quoted 15 lines · Sunil Mushran

Quoted 9 lines · Wengang Wang

When we are to purge a lockres, we check if it's unused.
the check includes
1. no locks attached to the lockres.
2. not dirty.
3. not in recovery.
4. no interested nodes in refmap.
If the the above four are satisfied, we are going to purge it(remove it from hash table).

Quoted 5 lines

While, when a "create lock" is in progress especially when lockres is owned remotely(spinlocks are dropped when networking), the lockres can satisfy above four condition. If it's put to purge list, it can be purged out from hash table when we are still accessing on it(sending request to owner for example). That's not what we want.

Quoted 13 lines

Create lock follows master query. And in master query handler we add the node to the refmap. That's the race refmap was created to close. Meaning we should not run into this condition.

Which version did this problem reproduce in?

Quoted 7 lines

I have met such a problem (orabug 12405575).
The lockres is in the purge list already(there is a delay for real purge work) and the create lock request comes. When we are sending network message to the owner in dlmlock_remote(), the owner crashed. So we get DLM_RECOVERING as return value and retries dlmlock_remote(). And before the owner crash, we have purged the lockres. So the lockres become stale(on lockres->onwer). Thus the code calls dlmlock_remote() infinitely.

Oh a remote master. So ignore my previous reply.

Yes, I can see the race. But the fix below lets the purge to continue and handles it afterwards. A better approach (and more efficient) would be to remove the lockres from the purge list itself.

So the race window is between the first block in dlm_get_lock_resource() and dlm_lock_remote().

See dlm->inflight_locks. Currently we use this when lockres is locally mastered. Maybe we could use the same for locally mastered too.

Quoted 11 lines · Sunil Mushran

Oh a remote master. So ignore my previous reply.

Yes, I can see the race. But the fix below lets the purge to continue and handles it afterwards. A better approach (and more efficient) would be to remove the lockres from the purge list itself.

So the race window is between the first block in dlm_get_lock_resource() and dlm_lock_remote().

See dlm->inflight_locks. Currently we use this when lockres is locally mastered. Maybe we could use the same for locally mastered too.

Sunil,

So the only purpose of inflight_locks is to prevent the lockres from being purged, right?

I think only removing the lockres from the purge list is enough.

If we hack on inflight_locks, we don't need to remove the lockres from purge list, right?