Skip to contentSkip to conversation

ocfs2 fix o2dlm dlm run purgelist

PATCH New Srinivas Eeda 4 participants 24 messages Jun 16, 2010 – Jun 21, 2010 OCFS2

lore
Share & tools
lore

Srinivas Eeda

author
#0 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Signed-off-by Srinivas Eeda <[email protected]>
---
 fs/ocfs2/dlm/dlmthread.c |  125 +++++++++++++++++++++++-----------------------
 1 files changed, 63 insertions(+), 62 deletions(-)
fs/ocfs2/dlm/dlmthread.c +63 -62
index 11a6d1f..fb0be6c 100644
@@ -158,39 +158,17 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 	int master;
 	int ret = 0;
 
-	spin_lock(&res->spinlock);
-	if (!__dlm_lockres_unused(res)) {
-		mlog(0, "%s:%.*s: tried to purge but not unused\n",
-		     dlm->name, res->lockname.len, res->lockname.name);
-		__dlm_print_one_lock_resource(res);
-		spin_unlock(&res->spinlock);
-		BUG();
-	}
-
-	if (res->state & DLM_LOCK_RES_MIGRATING) {
-		mlog(0, "%s:%.*s: Delay dropref as this lockres is "
-		     "being remastered\n", dlm->name, res->lockname.len,
-		     res->lockname.name);
-		/* Re-add the lockres to the end of the purge list */
-		if (!list_empty(&res->purge)) {
-			list_del_init(&res->purge);
-			list_add_tail(&res->purge, &dlm->purge_list);
-		}
-		spin_unlock(&res->spinlock);
-		return 0;
-	}
-
 	master = (res->owner == dlm->node_num);
 
 	if (!master)
 		res->state |= DLM_LOCK_RES_DROPPING_REF;
-	spin_unlock(&res->spinlock);
 
 	mlog(0, "purging lockres %.*s, master = %d\n", res->lockname.len,
 	     res->lockname.name, master);
 
 	if (!master) {
 		/* drop spinlock...  retake below */
+		spin_unlock(&res->spinlock);
 		spin_unlock(&dlm->spinlock);
 
 		spin_lock(&res->spinlock);

@@ -208,48 +186,37 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 		mlog(0, "%s:%.*s: dlm_deref_lockres returned %d\n",
 		     dlm->name, res->lockname.len, res->lockname.name, ret);
 		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
 	}
 
-	spin_lock(&res->spinlock);
-	if (!list_empty(&res->purge)) {
-		mlog(0, "removing lockres %.*s:%p from purgelist, "
-		     "master = %d\n", res->lockname.len, res->lockname.name,
-		     res, master);
-		list_del_init(&res->purge);
-		spin_unlock(&res->spinlock);
-		dlm_lockres_put(res);
-		dlm->purge_count--;
-	} else
-		spin_unlock(&res->spinlock);
-
-	__dlm_unhash_lockres(res);
-
 	/* lockres is not in the hash now.  drop the flag and wake up
 	 * any processes waiting in dlm_get_lock_resource. */
-	if (!master) {
-		spin_lock(&res->spinlock);
+	if (!master)
 		res->state &= ~DLM_LOCK_RES_DROPPING_REF;
-		spin_unlock(&res->spinlock);
-		wake_up(&res->wq);
-	}
 	return 0;
 }
 
 static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			       int purge_now)
 {
-	unsigned int run_max, unused;
+	unsigned int run_max;
 	unsigned long purge_jiffies;
 	struct dlm_lock_resource *lockres;
+	struct dlm_lock_resource *nextres;
 
 	spin_lock(&dlm->spinlock);
 	run_max = dlm->purge_count;
 
-	while(run_max && !list_empty(&dlm->purge_list)) {
-		run_max--;
+	if (list_empty(&dlm->purge_list)) {
+		spin_unlock(&dlm->spinlock);
+		return;
+	}
+
+	lockres = list_entry(dlm->purge_list.next,
+			     struct dlm_lock_resource, purge);
 
-		lockres = list_entry(dlm->purge_list.next,
-				     struct dlm_lock_resource, purge);
+	while(run_max && lockres && !list_empty(&dlm->purge_list)) {
+		run_max--;
 
 		/* Status of the lockres *might* change so double
 		 * check. If the lockres is unused, holding the dlm

@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 		 * refs on it -- there's no need to keep the lockres
 		 * spinlock. */
 		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)
-			continue;
 
 		purge_jiffies = lockres->last_used +
 			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
 		/* Make sure that we want to be processing this guy at
 		 * this time. */
 		if (!purge_now && time_after(purge_jiffies, jiffies)) {

@@ -273,20 +237,57 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			 * in tail order, we can stop at the first
 			 * unpurgable resource -- anyone added after
 			 * him will have a greater last_used value */
+			spin_unlock(&lockres->spinlock);
 			break;
 		}
 
-		dlm_lockres_get(lockres);
-
+		/* If lockres is being used, or migrating purge next lockres */
+		if (!__dlm_lockres_unused(lockres) ||
+		    (lockres->state & DLM_LOCK_RES_MIGRATING)) {
+			if (!list_is_last(&lockres->purge, &dlm->purge_list))
+				nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+			else
+				nextres = NULL;
+			spin_unlock(&lockres->spinlock);
+			lockres = nextres;
+			continue;
+		}
+		
 		/* This may drop and reacquire the dlm spinlock if it
 		 * has to do migration. */
-		if (dlm_purge_lockres(dlm, lockres))
-			BUG();
-
-		dlm_lockres_put(lockres);
-
-		/* Avoid adding any scheduling latencies */
-		cond_resched_lock(&dlm->spinlock);
+		dlm_purge_lockres(dlm, lockres);
+		
+		/* before we free the lockres we get the next lockres */
+		if (list_empty(&lockres->purge))
+			/* Shouldn't be in this state. Start from beginning */
+			nextres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
+		else if (!list_is_last(&lockres->purge, &dlm->purge_list))
+			nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+		else
+			nextres = NULL;
+
+		if (__dlm_lockres_unused(lockres)) {
+			if (!list_empty(&lockres->purge)) {
+				list_del_init(&lockres->purge);
+				dlm->purge_count--;
+			}
+			__dlm_unhash_lockres(lockres);
+			spin_unlock(&lockres->spinlock);
+			wake_up(&lockres->wq);
+			dlm_lockres_put(lockres);
+		} else
+			spin_unlock(&lockres->spinlock);
+		lockres = nextres;
+
+		/* Avoid adding any scheduling latencies. If dlm spinlock is
+		 * dropped, retry again from the beginning as purgelist could
+		 * have been modified */
+		if (cond_resched_lock(&dlm->spinlock))
+			lockres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
 	}
 
 	spin_unlock(&dlm->spinlock);

@@ -733,7 +734,7 @@ in_progress:
 			/* unlikely, but we may need to give time to
 			 * other tasks */
 			if (!--n) {
-				mlog(0, "throttling dlm_thread\n");
+				mlog(0, "throttling dlm_thread n=%d\n", n);
 				break;
 			}
 		}
Signature · 1 line
1.5.6.5
author
#1 lore raw

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 <[email protected]>
---
 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(-)
fs/ocfs2/dlm/dlmcommon.h +1 -0
index 0102be3..6cf3c08 100644
@@ -279,6 +279,7 @@ static inline void __dlm_set_joining_node(struct dlm_ctxt *dlm,
 #define DLM_LOCK_RES_DIRTY                0x00000008
 #define DLM_LOCK_RES_IN_PROGRESS          0x00000010
 #define DLM_LOCK_RES_MIGRATING            0x00000020
+#define DLM_LOCK_RES_IN_USE               0x00000100
 #define DLM_LOCK_RES_DROPPING_REF         0x00000040
 #define DLM_LOCK_RES_BLOCK_DIRTY          0x00001000
 #define DLM_LOCK_RES_SETREF_INPROG        0x00002000
fs/ocfs2/dlm/dlmlock.c +4 -0
index 7333377..501ac40 100644
@@ -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 &&
fs/ocfs2/dlm/dlmmaster.c +4 -1
index 9289b43..f0f2d97 100644
@@ -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
fs/ocfs2/dlm/dlmthread.c +1 -0
index fb0be6c..2b82e0b 100644
@@ -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 */
Signature · 1 line
1.5.6.5
#2 lore raw

PATCH 2/2 ocfs2: o2dlm fix race in purge lockres and newlock (orabug 9094491)

This patch looks ok on the surface. Should be usable. But checking into the repo is another matter.

My problem is that this flag is very much like inflight_locks but is not the same. inflight_locks are taken only on lockres' that are mastered by the node. This new flag does the same but for non mastered locks too. A better solution will be to merge the two. And that means cleaning up inflight_locks.

The reason for the NAK for check in is that the code is adding more messiness to an area that is already very messy.

Sunil

Quoted 95 lines · Srinivas Eeda

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<[email protected]>
---
  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(-)
fs/ocfs2/dlm/dlmcommon.h +1 -0
index 0102be3..6cf3c08 100644
@@ -279,6 +279,7 @@ static inline void __dlm_set_joining_node(struct dlm_ctxt *dlm,
  #define DLM_LOCK_RES_DIRTY                0x00000008
  #define DLM_LOCK_RES_IN_PROGRESS          0x00000010
  #define DLM_LOCK_RES_MIGRATING            0x00000020
+#define DLM_LOCK_RES_IN_USE               0x00000100
  #define DLM_LOCK_RES_DROPPING_REF         0x00000040
  #define DLM_LOCK_RES_BLOCK_DIRTY          0x00001000
  #define DLM_LOCK_RES_SETREF_INPROG        0x00002000
fs/ocfs2/dlm/dlmlock.c +4 -0
index 7333377..501ac40 100644
@@ -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&&
fs/ocfs2/dlm/dlmmaster.c +4 -1
index 9289b43..f0f2d97 100644
@@ -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
fs/ocfs2/dlm/dlmthread.c +1 -0
index fb0be6c..2b82e0b 100644
@@ -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 */
author
#3 lore raw

PATCH 2/2 ocfs2: o2dlm fix race in purge lockres and newlock (orabug 9094491)

Quoted 2 lines · Sunil Mushran

This patch looks ok on the surface. Should be usable. But checking into the repo is another matter.

ok, won't checkin but will give this as one-off fix for now.

Quoted 6 lines

My problem is that this flag is very much like inflight_locks but is not the same. inflight_locks are taken only on lockres' that are mastered by the node. This new flag does the same but for non mastered locks too. A better solution will be to merge the two. And that means cleaning up inflight_locks.

will look into this.

Quoted 112 lines

The reason for the NAK for check in is that the code is adding more messiness to an area that is already very messy.

Sunil

Quoted 104 lines · Srinivas Eeda

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<[email protected]>
---
  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(-)
fs/ocfs2/dlm/dlmcommon.h +1 -0
index 0102be3..6cf3c08 100644
@@ -279,6 +279,7 @@ static inline void __dlm_set_joining_node(struct 
 dlm_ctxt *dlm,
  #define DLM_LOCK_RES_DIRTY                0x00000008
  #define DLM_LOCK_RES_IN_PROGRESS          0x00000010
  #define DLM_LOCK_RES_MIGRATING            0x00000020
+#define DLM_LOCK_RES_IN_USE               0x00000100
  #define DLM_LOCK_RES_DROPPING_REF         0x00000040
  #define DLM_LOCK_RES_BLOCK_DIRTY          0x00001000

  #define DLM_LOCK_RES_SETREF_INPROG        0x00002000
fs/ocfs2/dlm/dlmlock.c +2 -0
index 7333377..501ac40 100644
@@ -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;
fs/ocfs2/dlm/dlmlock.c +1 -0
@@ -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)
fs/ocfs2/dlm/dlmlock.c +1 -0
@@ -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&&
fs/ocfs2/dlm/dlmmaster.c +4 -1
index 9289b43..f0f2d97 100644
@@ -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
fs/ocfs2/dlm/dlmthread.c +1 -0
index fb0be6c..2b82e0b 100644
@@ -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 */

#4 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Srinivas Eeda

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

Regards,
wengang.

Quoted 228 lines · Srinivas Eeda

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Signed-off-by Srinivas Eeda <[email protected]>
---
 fs/ocfs2/dlm/dlmthread.c |  125 +++++++++++++++++++++++-----------------------
 1 files changed, 63 insertions(+), 62 deletions(-)
fs/ocfs2/dlm/dlmthread.c +63 -62
index 11a6d1f..fb0be6c 100644
@@ -158,39 +158,17 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 	int master;
 	int ret = 0;
 
-	spin_lock(&res->spinlock);
-	if (!__dlm_lockres_unused(res)) {
-		mlog(0, "%s:%.*s: tried to purge but not unused\n",
-		     dlm->name, res->lockname.len, res->lockname.name);
-		__dlm_print_one_lock_resource(res);
-		spin_unlock(&res->spinlock);
-		BUG();
-	}
-
-	if (res->state & DLM_LOCK_RES_MIGRATING) {
-		mlog(0, "%s:%.*s: Delay dropref as this lockres is "
-		     "being remastered\n", dlm->name, res->lockname.len,
-		     res->lockname.name);
-		/* Re-add the lockres to the end of the purge list */
-		if (!list_empty(&res->purge)) {
-			list_del_init(&res->purge);
-			list_add_tail(&res->purge, &dlm->purge_list);
-		}
-		spin_unlock(&res->spinlock);
-		return 0;
-	}
-
 	master = (res->owner == dlm->node_num);
 
 	if (!master)
 		res->state |= DLM_LOCK_RES_DROPPING_REF;
-	spin_unlock(&res->spinlock);
 
 	mlog(0, "purging lockres %.*s, master = %d\n", res->lockname.len,
 	     res->lockname.name, master);
 
 	if (!master) {
 		/* drop spinlock...  retake below */
+		spin_unlock(&res->spinlock);
 		spin_unlock(&dlm->spinlock);
 
 		spin_lock(&res->spinlock);

@@ -208,48 +186,37 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 		mlog(0, "%s:%.*s: dlm_deref_lockres returned %d\n",
 		     dlm->name, res->lockname.len, res->lockname.name, ret);
 		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
 	}
 
-	spin_lock(&res->spinlock);
-	if (!list_empty(&res->purge)) {
-		mlog(0, "removing lockres %.*s:%p from purgelist, "
-		     "master = %d\n", res->lockname.len, res->lockname.name,
-		     res, master);
-		list_del_init(&res->purge);
-		spin_unlock(&res->spinlock);
-		dlm_lockres_put(res);
-		dlm->purge_count--;
-	} else
-		spin_unlock(&res->spinlock);
-
-	__dlm_unhash_lockres(res);
-
 	/* lockres is not in the hash now.  drop the flag and wake up
 	 * any processes waiting in dlm_get_lock_resource. */
-	if (!master) {
-		spin_lock(&res->spinlock);
+	if (!master)
 		res->state &= ~DLM_LOCK_RES_DROPPING_REF;
-		spin_unlock(&res->spinlock);
-		wake_up(&res->wq);
-	}
 	return 0;
 }
 
 static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			       int purge_now)
 {
-	unsigned int run_max, unused;
+	unsigned int run_max;
 	unsigned long purge_jiffies;
 	struct dlm_lock_resource *lockres;
+	struct dlm_lock_resource *nextres;
 
 	spin_lock(&dlm->spinlock);
 	run_max = dlm->purge_count;
 
-	while(run_max && !list_empty(&dlm->purge_list)) {
-		run_max--;
+	if (list_empty(&dlm->purge_list)) {
+		spin_unlock(&dlm->spinlock);
+		return;
+	}
+
+	lockres = list_entry(dlm->purge_list.next,
+			     struct dlm_lock_resource, purge);
 
-		lockres = list_entry(dlm->purge_list.next,
-				     struct dlm_lock_resource, purge);
+	while(run_max && lockres && !list_empty(&dlm->purge_list)) {
+		run_max--;
 
 		/* Status of the lockres *might* change so double
 		 * check. If the lockres is unused, holding the dlm

@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 		 * refs on it -- there's no need to keep the lockres
 		 * spinlock. */
 		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)
-			continue;
 
 		purge_jiffies = lockres->last_used +
 			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
 		/* Make sure that we want to be processing this guy at
 		 * this time. */
 		if (!purge_now && time_after(purge_jiffies, jiffies)) {

@@ -273,20 +237,57 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			 * in tail order, we can stop at the first
 			 * unpurgable resource -- anyone added after
 			 * him will have a greater last_used value */
+			spin_unlock(&lockres->spinlock);
 			break;
 		}
 
-		dlm_lockres_get(lockres);
-
+		/* If lockres is being used, or migrating purge next lockres */
+		if (!__dlm_lockres_unused(lockres) ||
+		    (lockres->state & DLM_LOCK_RES_MIGRATING)) {
+			if (!list_is_last(&lockres->purge, &dlm->purge_list))
+				nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+			else
+				nextres = NULL;
+			spin_unlock(&lockres->spinlock);
+			lockres = nextres;
+			continue;
+		}
+		
 		/* This may drop and reacquire the dlm spinlock if it
 		 * has to do migration. */
-		if (dlm_purge_lockres(dlm, lockres))
-			BUG();
-
-		dlm_lockres_put(lockres);
-
-		/* Avoid adding any scheduling latencies */
-		cond_resched_lock(&dlm->spinlock);
+		dlm_purge_lockres(dlm, lockres);
+		
+		/* before we free the lockres we get the next lockres */
+		if (list_empty(&lockres->purge))
+			/* Shouldn't be in this state. Start from beginning */
+			nextres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
+		else if (!list_is_last(&lockres->purge, &dlm->purge_list))
+			nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+		else
+			nextres = NULL;
+
+		if (__dlm_lockres_unused(lockres)) {
+			if (!list_empty(&lockres->purge)) {
+				list_del_init(&lockres->purge);
+				dlm->purge_count--;
+			}
+			__dlm_unhash_lockres(lockres);
+			spin_unlock(&lockres->spinlock);
+			wake_up(&lockres->wq);
+			dlm_lockres_put(lockres);
+		} else
+			spin_unlock(&lockres->spinlock);
+		lockres = nextres;
+
+		/* Avoid adding any scheduling latencies. If dlm spinlock is
+		 * dropped, retry again from the beginning as purgelist could
+		 * have been modified */
+		if (cond_resched_lock(&dlm->spinlock))
+			lockres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
 	}
 
 	spin_unlock(&dlm->spinlock);

@@ -733,7 +734,7 @@ in_progress:
 			/* unlikely, but we may need to give time to
 			 * other tasks */
 			if (!--n) {
-				mlog(0, "throttling dlm_thread\n");
+				mlog(0, "throttling dlm_thread n=%d\n", n);
 				break;
 			}
 		}
Signature · 7 lines
1.5.6.5


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

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 5 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So

Yes, that's a possibility. But there is not much we could do to cover that window other than making the non master nodes to avoid such races. Patch 2/2 fixes one such race.

Quoted 5 lines

1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

patch 2/2 fixes this race. dlm_get_lock_resource will either wait for the lockres to get purged and starts everything fresh or marks the lockres in use so dlm_thread won't purge it.

Quoted 3 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

right, not a good idea to send deref again. We have to fix those cases.

Quoted 4 lines

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

if you are referring to the hole in dlmlock_remote, patch 2/2 fixes it. Please review that patch and let me know :)

Quoted 233 lines

Regards,
wengang.

Quoted 229 lines · Srinivas Eeda

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Signed-off-by Srinivas Eeda <[email protected]>
---
 fs/ocfs2/dlm/dlmthread.c |  125 +++++++++++++++++++++++-----------------------
 1 files changed, 63 insertions(+), 62 deletions(-)
fs/ocfs2/dlm/dlmthread.c +63 -62
index 11a6d1f..fb0be6c 100644
@@ -158,39 +158,17 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 	int master;
 	int ret = 0;
 
-	spin_lock(&res->spinlock);
-	if (!__dlm_lockres_unused(res)) {
-		mlog(0, "%s:%.*s: tried to purge but not unused\n",
-		     dlm->name, res->lockname.len, res->lockname.name);
-		__dlm_print_one_lock_resource(res);
-		spin_unlock(&res->spinlock);
-		BUG();
-	}
-
-	if (res->state & DLM_LOCK_RES_MIGRATING) {
-		mlog(0, "%s:%.*s: Delay dropref as this lockres is "
-		     "being remastered\n", dlm->name, res->lockname.len,
-		     res->lockname.name);
-		/* Re-add the lockres to the end of the purge list */
-		if (!list_empty(&res->purge)) {
-			list_del_init(&res->purge);
-			list_add_tail(&res->purge, &dlm->purge_list);
-		}
-		spin_unlock(&res->spinlock);
-		return 0;
-	}
-
 	master = (res->owner == dlm->node_num);
 
 	if (!master)
 		res->state |= DLM_LOCK_RES_DROPPING_REF;
-	spin_unlock(&res->spinlock);
 
 	mlog(0, "purging lockres %.*s, master = %d\n", res->lockname.len,
 	     res->lockname.name, master);
 
 	if (!master) {
 		/* drop spinlock...  retake below */
+		spin_unlock(&res->spinlock);
 		spin_unlock(&dlm->spinlock);
 
 		spin_lock(&res->spinlock);

@@ -208,48 +186,37 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
 		mlog(0, "%s:%.*s: dlm_deref_lockres returned %d\n",
 		     dlm->name, res->lockname.len, res->lockname.name, ret);
 		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
 	}
 
-	spin_lock(&res->spinlock);
-	if (!list_empty(&res->purge)) {
-		mlog(0, "removing lockres %.*s:%p from purgelist, "
-		     "master = %d\n", res->lockname.len, res->lockname.name,
-		     res, master);
-		list_del_init(&res->purge);
-		spin_unlock(&res->spinlock);
-		dlm_lockres_put(res);
-		dlm->purge_count--;
-	} else
-		spin_unlock(&res->spinlock);
-
-	__dlm_unhash_lockres(res);
-
 	/* lockres is not in the hash now.  drop the flag and wake up
 	 * any processes waiting in dlm_get_lock_resource. */
-	if (!master) {
-		spin_lock(&res->spinlock);
+	if (!master)
 		res->state &= ~DLM_LOCK_RES_DROPPING_REF;
-		spin_unlock(&res->spinlock);
-		wake_up(&res->wq);
-	}
 	return 0;
 }
 
 static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			       int purge_now)
 {
-	unsigned int run_max, unused;
+	unsigned int run_max;
 	unsigned long purge_jiffies;
 	struct dlm_lock_resource *lockres;
+	struct dlm_lock_resource *nextres;
 
 	spin_lock(&dlm->spinlock);
 	run_max = dlm->purge_count;
 
-	while(run_max && !list_empty(&dlm->purge_list)) {
-		run_max--;
+	if (list_empty(&dlm->purge_list)) {
+		spin_unlock(&dlm->spinlock);
+		return;
+	}
+
+	lockres = list_entry(dlm->purge_list.next,
+			     struct dlm_lock_resource, purge);
 
-		lockres = list_entry(dlm->purge_list.next,
-				     struct dlm_lock_resource, purge);
+	while(run_max && lockres && !list_empty(&dlm->purge_list)) {
+		run_max--;
 
 		/* Status of the lockres *might* change so double
 		 * check. If the lockres is unused, holding the dlm

@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 		 * refs on it -- there's no need to keep the lockres
 		 * spinlock. */
 		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)
-			continue;
 
 		purge_jiffies = lockres->last_used +
 			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
 		/* Make sure that we want to be processing this guy at
 		 * this time. */
 		if (!purge_now && time_after(purge_jiffies, jiffies)) {

@@ -273,20 +237,57 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 			 * in tail order, we can stop at the first
 			 * unpurgable resource -- anyone added after
 			 * him will have a greater last_used value */
+			spin_unlock(&lockres->spinlock);
 			break;
 		}
 
-		dlm_lockres_get(lockres);
-
+		/* If lockres is being used, or migrating purge next lockres */
+		if (!__dlm_lockres_unused(lockres) ||
+		    (lockres->state & DLM_LOCK_RES_MIGRATING)) {
+			if (!list_is_last(&lockres->purge, &dlm->purge_list))
+				nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+			else
+				nextres = NULL;
+			spin_unlock(&lockres->spinlock);
+			lockres = nextres;
+			continue;
+		}
+		
 		/* This may drop and reacquire the dlm spinlock if it
 		 * has to do migration. */
-		if (dlm_purge_lockres(dlm, lockres))
-			BUG();
-
-		dlm_lockres_put(lockres);
-
-		/* Avoid adding any scheduling latencies */
-		cond_resched_lock(&dlm->spinlock);
+		dlm_purge_lockres(dlm, lockres);
+		
+		/* before we free the lockres we get the next lockres */
+		if (list_empty(&lockres->purge))
+			/* Shouldn't be in this state. Start from beginning */
+			nextres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
+		else if (!list_is_last(&lockres->purge, &dlm->purge_list))
+			nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+		else
+			nextres = NULL;
+
+		if (__dlm_lockres_unused(lockres)) {
+			if (!list_empty(&lockres->purge)) {
+				list_del_init(&lockres->purge);
+				dlm->purge_count--;
+			}
+			__dlm_unhash_lockres(lockres);
+			spin_unlock(&lockres->spinlock);
+			wake_up(&lockres->wq);
+			dlm_lockres_put(lockres);
+		} else
+			spin_unlock(&lockres->spinlock);
+		lockres = nextres;
+
+		/* Avoid adding any scheduling latencies. If dlm spinlock is
+		 * dropped, retry again from the beginning as purgelist could
+		 * have been modified */
+		if (cond_resched_lock(&dlm->spinlock))
+			lockres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
 	}
 
 	spin_unlock(&dlm->spinlock);

@@ -733,7 +734,7 @@ in_progress:
 			/* unlikely, but we may need to give time to
 			 * other tasks */
 			if (!--n) {
-				mlog(0, "throttling dlm_thread\n");
+				mlog(0, "throttling dlm_thread n=%d\n", n);
 				break;
 			}
 		}
Signature · 7 lines
1.5.6.5


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

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 8 lines · Srinivas Eeda
Quoted 4 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So

Yes, that's a possibility. But there is not much we could do to cover that window other than making the non master nodes to avoid such races. Patch 2/2 fixes one such race.

Yes, we should make non master nodes to avoid such races. But you only fixed one of such races by patch 2/2 :).
And probably we can't make sure how many such races there. A know case is dlm_mig_lockres_handler(). We dropped dlm->spinlock and res->spinlock after dlm_lookup_lockres(). Here it can be set DROPPING_REF state in dlm_thread(). dlm_thread() then drop the spinlocks and set deref msg. Before dlm_thread() take the spinlocks back, dlm_mig_lockres_handler() continues, A lock(s) is added to the lockres. The lockres became in use then. dlm_thread() take back the spinlocks, found the lockres is in use, keep it in hashtable and in purge list.

Your patch 2/2 fixes that problem well.
So far I have no good idea to fix all such potential races..

regards,
wengang.

Quoted 17 lines
Quoted 4 lines

1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

patch 2/2 fixes this race. dlm_get_lock_resource will either wait for the lockres to get purged and starts everything fresh or marks the lockres in use so dlm_thread won't purge it.

Quoted 2 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

right, not a good idea to send deref again. We have to fix those cases.

Quoted 3 lines

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

if you are referring to the hole in dlmlock_remote, patch 2/2 fixes it. Please review that patch and let me know :)

Quoted 2 lines

Regards,
wengang.

#7 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Wengang Wang
Quoted 9 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

Quoted 7 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

The fix we are working towards is to ensure that we set DLM_LOCK_RES_DROPPING_REF once we are determined
to purge the lockres. As in, we should not let go of the spinlock before we have either set the flag or decided against purging that resource.

Once the flag is set, new users looking up the resource via dlm_get_lock_resource() will notice the flag and will then wait for that flag to be cleared before looking up the lockres hash again. If all goes well, the lockres will not be found (because it has since been unhashed) and it will be forced to go thru the full mastery process.

author
#8 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Sunil,

as of now, there is still a window in dlm_get_lock_resource, where it finds the lockres but it doesn't protect it from getting purged. Second patch fixes this by marking it in_use, can you please review that one as well.

Thanks,
--Srini

Quoted 34 lines · Sunil Mushran
Quoted 12 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use
again, then the lockres remains in the hash table and also in the purge list.
So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the
master. How to deal with it?

Quoted 8 lines

2) The lockres on this node is going to be purged again, it means it will send
secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

The fix we are working towards is to ensure that we set DLM_LOCK_RES_DROPPING_REF once we are determined
to purge the lockres. As in, we should not let go of the spinlock before we have either set the flag or decided against purging that resource.

Once the flag is set, new users looking up the resource via dlm_get_lock_resource() will notice the flag and will then wait for that flag to be cleared before looking up the lockres hash again. If all goes well, the lockres will not be found (because it has since been unhashed) and it will be forced to go thru the full mastery process.

#9 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Sunil Mushran
Quoted 23 lines · Sunil Mushran
Quoted 9 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

Quoted 6 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

The fix we are working towards is to ensure that we set DLM_LOCK_RES_DROPPING_REF once we are determined
to purge the lockres. As in, we should not let go of the spinlock before we have either set the flag or decided against purging that resource.

Quoted 6 lines

Once the flag is set, new users looking up the resource via dlm_get_lock_resource() will notice the flag and will then wait for that flag to be cleared before looking up the lockres hash again. If all goes well, the lockres will not be found (because it has since been unhashed) and it will be forced to go thru the full mastery process.

That is ideal.
In many cases the lockres is not got via dlm_get_lock_resource(), but via dlm_lookup_lockres()/__dlm_lookup_lockres, which doesn't set the new IN-USE state, directly. dlm_lookup_lockres() takes and drops dlm->spinlock. And some of caller of __dlm_lookup_lockres() drops the spinlock as soon as it got the lockres. Such paths access the lockres later after dropping dlm->spinlock and res->spinlock.
So there is a window that dlm_thread() get a chance to take the dlm->spinlock and res->spinlock and set the DROPPING_REF state. So whether new users can get the lockres depends on how "new" it is. If finds the lockres after DROPPING_REF state is set, sure it works well. But if it find it before DROPPING_REF is set, it won't protect the lockres from purging since even it "gets" the lockres, the lockres can still in unused state.

regards,
wengang.

#10 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 33 lines · Wengang Wang
Quoted 24 lines · Sunil Mushran
Quoted 9 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

Quoted 7 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

The fix we are working towards is to ensure that we set DLM_LOCK_RES_DROPPING_REF once we are determined
to purge the lockres. As in, we should not let go of the spinlock before we have either set the flag or decided against purging that resource.

Quoted 7 lines

Once the flag is set, new users looking up the resource via dlm_get_lock_resource() will notice the flag and will then wait for that flag to be cleared before looking up the lockres hash again. If all goes well, the lockres will not be found (because it has since been unhashed) and it will be forced to go thru the full mastery process.

Quoted 15 lines

That is ideal.
In many cases the lockres is not got via dlm_get_lock_resource(), but via dlm_lookup_lockres()/__dlm_lookup_lockres, which doesn't set the new IN-USE state, directly. dlm_lookup_lockres() takes and drops dlm->spinlock. And some of caller of __dlm_lookup_lockres() drops the spinlock as soon as it got the lockres. Such paths access the lockres later after dropping dlm->spinlock and res->spinlock.
So there is a window that dlm_thread() get a chance to take the dlm->spinlock and res->spinlock and set the DROPPING_REF state. So whether new users can get the lockres depends on how "new" it is. If finds the lockres after DROPPING_REF state is set, sure it works well. But if it find it before DROPPING_REF is set, it won't protect the lockres from purging since even it "gets" the lockres, the lockres can still in unused state.

dlm_lookup_lockres() and friends just looks up the lockres hash. dlm_get_lock_resource() also calls it. It inturn is called by dlmlock() to find and/or create lockres and create a lock on that resource.

The other calls to dlm_lookup_lockres() are from handlers and those handlers can only be tickled if a lock already exists. And if a lock exits, then we cannot be purging the lockres.

The one exception is the create_lock handler and that only comes into play on the lockres master. The inflight ref blocks removal of such lockres in the window before the lock is created.

DROPPING_REF is only valid for non-master nodes. As in, only a non-master node has to send a deref message to the master node.

Confused? Well, I think this needs to be documented. I guess I will do that after I am done with the global heartbeat business.

Sunil

#11 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 45 lines · Sunil Mushran
Quoted 29 lines · Wengang Wang
Quoted 22 lines · Sunil Mushran
Quoted 9 lines · Wengang Wang

still the question.
If you have sent DEREF request to the master, and the lockres became in-use again, then the lockres remains in the hash table and also in the purge list. So
1) If this node is the last ref, there is a possibility that the master purged the lockres after receiving DEREF request from this node. In this case, when this node does dlmlock_remote(), the lockres won't be found on the master. How to deal with it?

Quoted 6 lines

2) The lockres on this node is going to be purged again, it means it will send secondary DEREFs to the master. This is not good I think.

A thought is setting lockres->owner to DLM_LOCK_RES_OWNER_UNKNOWN after sending a DEREF request againt this lockres. Also redo master reqeust before locking on it.

The fix we are working towards is to ensure that we set DLM_LOCK_RES_DROPPING_REF once we are determined
to purge the lockres. As in, we should not let go of the spinlock before we have either set the flag or decided against purging that resource.

Quoted 6 lines

Once the flag is set, new users looking up the resource via dlm_get_lock_resource() will notice the flag and will then wait for that flag to be cleared before looking up the lockres hash again. If all goes well, the lockres will not be found (because it has since been unhashed) and it will be forced to go thru the full mastery process.

Quoted 14 lines

That is ideal.
In many cases the lockres is not got via dlm_get_lock_resource(), but via dlm_lookup_lockres()/__dlm_lookup_lockres, which doesn't set the new IN-USE state, directly. dlm_lookup_lockres() takes and drops dlm->spinlock. And some of caller of __dlm_lookup_lockres() drops the spinlock as soon as it got the lockres. Such paths access the lockres later after dropping dlm->spinlock and res->spinlock.
So there is a window that dlm_thread() get a chance to take the dlm->spinlock and res->spinlock and set the DROPPING_REF state. So whether new users can get the lockres depends on how "new" it is. If finds the lockres after DROPPING_REF state is set, sure it works well. But if it find it before DROPPING_REF is set, it won't protect the lockres from purging since even it "gets" the lockres, the lockres can still in unused state.

Quoted 3 lines

dlm_lookup_lockres() and friends just looks up the lockres hash. dlm_get_lock_resource() also calls it. It inturn is called by dlmlock() to find and/or create lockres and create a lock on that resource.

Yes you are right.

Quoted 7 lines

The other calls to dlm_lookup_lockres() are from handlers and those handlers can only be tickled if a lock already exists. And if a lock exits, then we cannot be purging the lockres.

The one exception is the create_lock handler and that only comes into play on the lockres master. The inflight ref blocks removal of such lockres in the window before the lock is created.

I think there is another exception, dlm_mig_lockres_handler(). Could you check it in my email(in this thread, to Srini, Message-ID:
<[email protected]>).

Quoted 2 lines

DROPPING_REF is only valid for non-master nodes. As in, only a non-master node has to send a deref message to the master node.

Yes I know.

Quoted 2 lines

Confused? Well, I think this needs to be documented. I guess I will do that after I am done with the global heartbeat business.

No, I am clear. Well a document will greatly helpful!

regards,
wengang.

#12 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Srinivas Eeda
Quoted 10 lines · Srinivas Eeda

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

Quoted 5 lines

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Signed-off-by Srinivas Eeda <[email protected]>

I don't really like the way you did this. You're absolutely
right that we need to hold the spinlock while setting DROPPING REF. But there's no need to lift the lockres check logic into run_purge_list.

Quoted 8 lines
fs/ocfs2/dlm/dlmthread.c +0 -4
@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 		 * refs on it -- there's no need to keep the lockres
 		 * spinlock. */
 		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)

fs/ocfs2/dlm/dlmthread.c static void dlm_run_purge_list(struct dlm_ctxt *dlm, · line 235

Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +2 -1
-			continue;
 
 		purge_jiffies = lockres->last_used +
 			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
 		/* Make sure that we want to be processing this guy at
 		 * this time. */
 		if (!purge_now && time_after(purge_jiffies, jiffies)) {

In fact, I'd move the __dlm_lockres_unused() and
purge_now||time_after() checks into dlm_purge_lockres(). It can return -EBUSY if the lockres is in use. It can return -ETIME if purge_now==0 and time_after hits. Then inside run_purge_list() you just do:

		spin_lock(&lockres->spinlock);
		ret = dlm_purge_lockres(dlm, res, purge_now);
		spin_unlock(&lockres->spinlock);
		if (ret == -EAGAIN)
			break;
		else if (ret == -EBUSY) {
			lockres = list_entry(lockres->next);
			continue;
		else if (ret)
			BUG();

	What about the dlm_lockres_get()?  That's only held while we

drop the dlm spinlock in dlm_purge_lockres(), so you can move it there. You take the kref only after the _unused() and time_after() checks.
This actually would make run_purge_list() more readable, not
less.

Joel

Signature · 9 lines
"There are only two ways to live your life. One is as though nothing
 is a miracle. The other is as though everything is a miracle."
        - Albert Einstein

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
author
#13 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 16 lines · Joel Becker
Quoted 14 lines · Srinivas Eeda

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Quoted 7 lines
Quoted 2 lines
Signed-off-by Srinivas Eeda <[email protected]>

I don't really like the way you did this. You're absolutely
right that we need to hold the spinlock while setting DROPPING REF. But there's no need to lift the lockres check logic into run_purge_list.

As of today, we always get the lockres from the head of the dlm->purge_list. If it is in use, we keep trying. If that gets purged, we move to next res which again would be on the head of the dlm->purge_list. But with my change that won't be the case. If first (few) lockres are in use, we will try the one we could purge. So we should get the next lockres before list_del_init(&res->purge) happens and hence I moved the code. If you didn't like the delete code in dlm_run_purge_list, then we have to make dlm_purge_lockres to return the next lockres that should get purged.

If you are suggesting that we move the lockres to the tail if we found it in use, then the code will be lot more readable. The current code doesn't move the unused lockres to tail, so wanted to preserve that logic as I am not sure what was the original intent. If move used lockres to tail, would you suggest we also update lockres->last_used?

Quoted 25 lines
Quoted 9 lines
fs/ocfs2/dlm/dlmthread.c +0 -4
@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
 		 * refs on it -- there's no need to keep the lockres
 		 * spinlock. */
 		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)

fs/ocfs2/dlm/dlmthread.c static void dlm_run_purge_list(struct dlm_ctxt *dlm, · line 235

Quoted 10 lines
fs/ocfs2/dlm/dlmthread.c +2 -1
-			continue;
 
 		purge_jiffies = lockres->last_used +
 			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
 		/* Make sure that we want to be processing this guy at
 		 * this time. */
 		if (!purge_now && time_after(purge_jiffies, jiffies)) {

In fact, I'd move the __dlm_lockres_unused() and
purge_now||time_after() checks into dlm_purge_lockres(). It can return -EBUSY if the lockres is in use. It can return -ETIME if purge_now==0 and time_after hits. Then inside run_purge_list() you just do:

Quoted 11 lines
		spin_lock(&lockres->spinlock);
		ret = dlm_purge_lockres(dlm, res, purge_now);
		spin_unlock(&lockres->spinlock);
		if (ret == -EAGAIN)
			break;
		else if (ret == -EBUSY) {
			lockres = list_entry(lockres->next);
			continue;
		else if (ret)
			BUG();

what would list_entry return, if the lockres was the last on the list. I was thinking it would return something random ..

Quoted 6 lines

What about the dlm_lockres_get()? That's only held while we
drop the dlm spinlock in dlm_purge_lockres(), so you can move it there. You take the kref only after the _unused() and time_after() checks.
This actually would make run_purge_list() more readable, not
less.

I agree that it is less readable now, but didn't find a better way as I am not sure how to get to the next lockres if the current lockres that got dropped is the middle one. :)

Quoted 3 lines

Joel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100617/bb534bfc/attachment-0001.html

#14 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 10 lines · Srinivas Eeda

On 6/16/2010 6:39 PM, Joel Becker wrote:
As of today, we always get the lockres from the head of the dlm->purge_list. If it is in use, we keep trying. If that gets purged, we move to next res which again would be on the head of the dlm->purge_list. But with my change that won't be the case. If first (few) lockres are in use, we will try the one we could purge. So we should get the next lockres before list_del_init(&res->purge) happens and hence I moved the code. If you didn't like the delete code in dlm_run_purge_list, then we have to make dlm_purge_lockres to return the next lockres that should get purged.

Or it does everything but the delete, and then you have
run_purge_list do the delete right after returning when the return code is zero. My problem wasn't that you moved the delete, it was that you moved all the other checks and things.

Quoted 6 lines

If you are suggesting that we move the lockres to the tail if we found it in use, then the code will be lot more readable. The current code doesn't move the unused lockres to tail, so wanted to preserve that logic as I am not sure what was the original intent. If move used lockres to tail, would you suggest we also update lockres->last_used?

Like I responded to Sunil, it sure looks like the code wants the
same order. If moving to the end is safe, that's obviously the best answer.

Quoted 2 lines

what would list_entry return, if the lockres was the last on the list. I was thinking it would return something random ..

It returns the head of the list. These are doubly-linked lists.
list_empty() actually checks whether head->next == head. You can just have the while loop say "while (lockres->list != head)" to exit at the last item. Then when you say 'lockres = list_entry(lockres->next); continue;", the while loop exits properly at the end of the list.

Joel

Signature · 9 lines
Life's Little Instruction Book #198

	"Feed a stranger's expired parking meter."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
#15 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Srinivas Eeda

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

I have attached a quick & dirty patch. See if that satisfies all the requirements.

Sunil

Quoted 213 lines · Srinivas Eeda

There are two problems in dlm_run_purgelist

1. If a lockres is found to be in use, dlm_run_purgelist keeps trying to purge the same lockres instead of trying the next lockres.

2. When a lockres is found unused, dlm_run_purgelist releases lockres spinlock before setting DLM_LOCK_RES_DROPPING_REF and calls dlm_purge_lockres. spinlock is reacquired but in this window lockres can get reused. This leads to BUG.

This patch modifies dlm_run_purgelist to skip lockres if it's in use and purge
next lockres. It also sets DLM_LOCK_RES_DROPPING_REF before releasing the
lockres spinlock protecting it from getting reused.

Signed-off-by Srinivas Eeda<[email protected]>
---
  fs/ocfs2/dlm/dlmthread.c |  125 +++++++++++++++++++++++-----------------------
  1 files changed, 63 insertions(+), 62 deletions(-)
fs/ocfs2/dlm/dlmthread.c +62 -61
index 11a6d1f..fb0be6c 100644
@@ -158,39 +158,17 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
  	int master;
  	int ret = 0;
 
-	spin_lock(&res->spinlock);
-	if (!__dlm_lockres_unused(res)) {
-		mlog(0, "%s:%.*s: tried to purge but not unused\n",
-		     dlm->name, res->lockname.len, res->lockname.name);
-		__dlm_print_one_lock_resource(res);
-		spin_unlock(&res->spinlock);
-		BUG();
-	}
-
-	if (res->state&  DLM_LOCK_RES_MIGRATING) {
-		mlog(0, "%s:%.*s: Delay dropref as this lockres is "
-		     "being remastered\n", dlm->name, res->lockname.len,
-		     res->lockname.name);
-		/* Re-add the lockres to the end of the purge list */
-		if (!list_empty(&res->purge)) {
-			list_del_init(&res->purge);
-			list_add_tail(&res->purge,&dlm->purge_list);
-		}
-		spin_unlock(&res->spinlock);
-		return 0;
-	}
-
  	master = (res->owner == dlm->node_num);
 
  	if (!master)
  		res->state |= DLM_LOCK_RES_DROPPING_REF;
-	spin_unlock(&res->spinlock);
 
  	mlog(0, "purging lockres %.*s, master = %d\n", res->lockname.len,
  	     res->lockname.name, master);
 
  	if (!master) {
  		/* drop spinlock...  retake below */
+		spin_unlock(&res->spinlock);
  		spin_unlock(&dlm->spinlock);
 
  		spin_lock(&res->spinlock);

@@ -208,48 +186,37 @@ static int dlm_purge_lockres(struct dlm_ctxt *dlm,
  		mlog(0, "%s:%.*s: dlm_deref_lockres returned %d\n",
  		     dlm->name, res->lockname.len, res->lockname.name, ret);
  		spin_lock(&dlm->spinlock);
+		spin_lock(&res->spinlock);
  	}
 
-	spin_lock(&res->spinlock);
-	if (!list_empty(&res->purge)) {
-		mlog(0, "removing lockres %.*s:%p from purgelist, "
-		     "master = %d\n", res->lockname.len, res->lockname.name,
-		     res, master);
-		list_del_init(&res->purge);
-		spin_unlock(&res->spinlock);
-		dlm_lockres_put(res);
-		dlm->purge_count--;
-	} else
-		spin_unlock(&res->spinlock);
-
-	__dlm_unhash_lockres(res);
-
  	/* lockres is not in the hash now.  drop the flag and wake up
  	 * any processes waiting in dlm_get_lock_resource. */
-	if (!master) {
-		spin_lock(&res->spinlock);
+	if (!master)
  		res->state&= ~DLM_LOCK_RES_DROPPING_REF;
-		spin_unlock(&res->spinlock);
-		wake_up(&res->wq);
-	}
  	return 0;
  }
 
  static void dlm_run_purge_list(struct dlm_ctxt *dlm,
  			       int purge_now)
  {
-	unsigned int run_max, unused;
+	unsigned int run_max;
  	unsigned long purge_jiffies;
  	struct dlm_lock_resource *lockres;
+	struct dlm_lock_resource *nextres;
 
  	spin_lock(&dlm->spinlock);
  	run_max = dlm->purge_count;
 
-	while(run_max&&  !list_empty(&dlm->purge_list)) {
-		run_max--;
+	if (list_empty(&dlm->purge_list)) {
+		spin_unlock(&dlm->spinlock);
+		return;
+	}
+
+	lockres = list_entry(dlm->purge_list.next,
+			     struct dlm_lock_resource, purge);
 
-		lockres = list_entry(dlm->purge_list.next,
-				     struct dlm_lock_resource, purge);
+	while(run_max&&  lockres&&  !list_empty(&dlm->purge_list)) {
+		run_max--;
 
  		/* Status of the lockres *might* change so double
  		 * check. If the lockres is unused, holding the dlm

@@ -257,15 +224,12 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
  		 * refs on it -- there's no need to keep the lockres
  		 * spinlock. */
  		spin_lock(&lockres->spinlock);
-		unused = __dlm_lockres_unused(lockres);
-		spin_unlock(&lockres->spinlock);
-
-		if (!unused)
-			continue;
 
  		purge_jiffies = lockres->last_used +
  			msecs_to_jiffies(DLM_PURGE_INTERVAL_MS);
 
+		mlog(0, "purging lockres %.*s\n", lockres->lockname.len,
+		     lockres->lockname.name);
  		/* Make sure that we want to be processing this guy at
  		 * this time. */
  		if (!purge_now&&  time_after(purge_jiffies, jiffies)) {

@@ -273,20 +237,57 @@ static void dlm_run_purge_list(struct dlm_ctxt *dlm,
  			 * in tail order, we can stop at the first
  			 * unpurgable resource -- anyone added after
  			 * him will have a greater last_used value */
+			spin_unlock(&lockres->spinlock);
  			break;
  		}
 
-		dlm_lockres_get(lockres);
-
+		/* If lockres is being used, or migrating purge next lockres */
+		if (!__dlm_lockres_unused(lockres) ||
+		    (lockres->state&  DLM_LOCK_RES_MIGRATING)) {
+			if (!list_is_last(&lockres->purge,&dlm->purge_list))
+				nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+			else
+				nextres = NULL;
+			spin_unlock(&lockres->spinlock);
+			lockres = nextres;
+			continue;
+		}
+		
  		/* This may drop and reacquire the dlm spinlock if it
  		 * has to do migration. */
-		if (dlm_purge_lockres(dlm, lockres))
-			BUG();
-
-		dlm_lockres_put(lockres);
-
-		/* Avoid adding any scheduling latencies */
-		cond_resched_lock(&dlm->spinlock);
+		dlm_purge_lockres(dlm, lockres);
+		
+		/* before we free the lockres we get the next lockres */
+		if (list_empty(&lockres->purge))
+			/* Shouldn't be in this state. Start from beginning */
+			nextres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
+		else if (!list_is_last(&lockres->purge,&dlm->purge_list))
+			nextres = list_entry(lockres->purge.next,
+					     struct dlm_lock_resource, purge);
+		else
+			nextres = NULL;
+
+		if (__dlm_lockres_unused(lockres)) {
+			if (!list_empty(&lockres->purge)) {
+				list_del_init(&lockres->purge);
+				dlm->purge_count--;
+			}
+			__dlm_unhash_lockres(lockres);
+			spin_unlock(&lockres->spinlock);
+			wake_up(&lockres->wq);
+			dlm_lockres_put(lockres);
+		} else
+			spin_unlock(&lockres->spinlock);
+		lockres = nextres;
+
+		/* Avoid adding any scheduling latencies. If dlm spinlock is
+		 * dropped, retry again from the beginning as purgelist could
+		 * have been modified */
+		if (cond_resched_lock(&dlm->spinlock))
+			lockres = list_entry(dlm->purge_list.next,
+					     struct dlm_lock_resource, purge);
  	}
 
  	spin_unlock(&dlm->spinlock);

fs/ocfs2/dlm/dlmthread.c in_progress: · line 740

Quoted 8 lines
fs/ocfs2/dlm/dlmthread.c +1 -1
@@ -733,7 +734,7 @@ in_progress:
  			/* unlikely, but we may need to give time to
  			 * other tasks */
  			if (!--n) {
-				mlog(0, "throttling dlm_thread\n");
+				mlog(0, "throttling dlm_thread n=%d\n", n);
  				break;
  			}
  		}

-------------- next part --------------
A non-text attachment was scrubbed...
Name: srini.patch
Type: text/x-patch
Size: 1315 bytes
Desc: not available
Url : http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100616/6601081c/attachment.bin

#16 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 2 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

For my knowledge, why not just list_move_tail()?

regards,
wengang.

#17 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Sunil Mushran
Quoted 5 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

I have attached a quick & dirty patch. See if that satisfies all the requirements.

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end violate that?

Joel

Signature · 10 lines
"All alone at the end of the evening
 When the bright lights have faded to blue.
 I was thinking about a woman who had loved me
 And I never knew"

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
author
#18 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 5 lines · Joel Becker
Quoted 3 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

Quoted 8 lines
Quoted 3 lines

I have attached a quick & dirty patch. See if that satisfies all the requirements.

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end violate that?

right. that's why I didn't want to move used lockres to tail :)

Quoted 3 lines

Joel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100617/ef913653/attachment.html

#19 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 10 lines · Srinivas Eeda
Quoted 9 lines · Joel Becker
Quoted 6 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

I have attached a quick& dirty patch. See if that satisfies all the requirements.

Quoted 5 lines
Quoted 4 lines

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end violate that?

right. that's why I didn't want to move used lockres to tail :)

No. There is no need for time ordering. Or, am I missing something?

We delay the purge incase the file system changes its mind and wants to reuse it. By delaying, we hold onto the mastery information. That's it.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100617/d9035590/attachment-0001.html

author
#20 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 15 lines · Sunil Mushran
Quoted 14 lines · Srinivas Eeda
Quoted 13 lines · Joel Becker
Quoted 6 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and list_add_tail(). That simplifies the patch a lot.

I have attached a quick & dirty patch. See if that satisfies all the requirements.

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end violate that?

Quoted 8 lines
Quoted 1 line

right. that's why I didn't want to move used lockres to tail :)

No. There is no need for time ordering. Or, am I missing something?

We delay the purge incase the file system changes its mind and wants to reuse it. By delaying, we hold onto the mastery information. That's it.

So, should we preserve this? or purge the lockres when it's on purge
list? OR we could just update the last_used and move it to end of the
list if it can be purged.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20100617/7cbbb416/attachment.html

#21 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

We don't have to update the last used to move it to tail. If it is now in use, means it is about to be taken out of the purgelist. We just need to move it from our way for the time being.

Quoted 34 lines · Srinivas Eeda
Quoted 21 lines · Sunil Mushran
Quoted 19 lines · Srinivas Eeda
Quoted 17 lines · Joel Becker
Quoted 8 lines · Sunil Mushran

One way to skip a lockres in the purgelist is to list_del_init() and
list_add_tail(). That simplifies the patch a lot.

I have attached a quick & dirty patch. See if that satisfies all the
requirements.

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end
violate that?

Quoted 9 lines
Quoted 1 line

right. that's why I didn't want to move used lockres to tail :)

No. There is no need for time ordering. Or, am I missing something?

We delay the purge incase the file system changes its mind and wants to reuse it. By delaying, we hold onto the mastery information. That's it.

So, should we preserve this? or purge the lockres when it's on purge list? OR we could just update the last_used and move it to end of the list if it can be purged.

#22 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

In reply to Sunil Mushran
Quoted 5 lines · Sunil Mushran
Quoted 4 lines · Srinivas Eeda
Quoted 3 lines · Joel Becker

As far as I can tell from reading the code, the time_after()
check is because they are time ordered. Wouldn't moving it to the end violate that?

Quoted 7 lines
Quoted 1 line

right. that's why I didn't want to move used lockres to tail :)

No. There is no need for time ordering. Or, am I missing something?

We delay the purge incase the file system changes its mind and wants to reuse it. By delaying, we hold onto the mastery information. That's it.

The comment is right there:

/* Since resources are added to the purge list
* in tail order, we can stop at the first
* unpurgable resource -- anyone added after
* him will have a greater last_used value */
break;

So we're going to break out of this loop as soon we see a later time. Anything you've moved to the back will be ignored until enough time passes that the things in front of it are candidates for purge.
You could, of course, just change the 'break' to a 'continue'
and find those, but I kind of like the short-circuit behavior. I don't know how long this list is, but walking a whole bunch of "not yet" lockreses just to hopefully find one at the end seems silly.
I think it is better to leave the busy ones at the front of the
list and just walk past them if they can't be dealt with now.

Joel

Signature · 9 lines
Life's Little Instruction Book #157 

	"Take time to smell the roses."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
#23 lore raw

PATCH 1/2 ocfs2 fix o2dlm dlm run purgelist

Quoted 13 lines · Joel Becker
Quoted 5 lines

No. There is no need for time ordering. Or, am I missing something?

We delay the purge incase the file system changes its mind and wants to reuse it. By delaying, we hold onto the mastery information. That's it.

The comment is right there:

/* Since resources are added to the purge list
* in tail order, we can stop at the first
* unpurgable resource -- anyone added after
* him will have a greater last_used value */
break;

Quoted 10 lines

So we're going to break out of this loop as soon we see a later time. Anything you've moved to the back will be ignored until enough time passes that the things in front of it are candidates for purge.
You could, of course, just change the 'break' to a 'continue'
and find those, but I kind of like the short-circuit behavior. I don't know how long this list is, but walking a whole bunch of "not yet" lockreses just to hopefully find one at the end seems silly.
I think it is better to leave the busy ones at the front of the
list and just walk past them if they can't be dealt with now.

That check can remain. The fact that a lockres in the purgelist was seen to be in use, means that another thread got to it before the dlm_thread and that that thread is about to remove that lockres from the purgelist shortly. As in, as soon as the two spinlocks are relinquished.