[Cluster-devel] [GFS2 PATCH 00/12] Radical Reform of glock state machine

[Cluster-devel] [GFS2 PATCH 00/12] Radical Reform of glock state machine

[Bob Peterson]

GFS2 glocks go through a very complex and confusing series of steps to
transition from one locking mode to another. For example, to transition
from unlocked (UN) mode to Shared (SH) mode we need to "promote" the lock,
and to do so requires a series of steps such as asking permission of DLM,
fetching dlm responses, checking for remote demotion requests, granting
multiple holders, and so forth.

This is all managed by a large set of functions known as the glock state
machine. Today, the glock state machine is a disorganized chaos of
functions that are neither documented nor intuitive. It's a complete
disaster that makes no sense at all to someone coming in to the code cold.
For proof, you need only look at the fact that function do_xmote() can
call function finish_xmote(), but finish_xmote() can call do_xmote() as
well. It works really well, but the problem is: it's a house of cards.
It's easy to misunderstand the intent and get it wrong, and if you try to
make any little change, everything falls apart.

The other problem is that today's glock state machine relies completely
on the delayed work queue. That means you can't simply transition from
one state to the next; you need to queue delayed work and have the
delayed workqueue do the work. This requires an absurd number of context
switches to make the simplest change to a glock. That creates a lot of
unnecessary latency, and makes it much harder for smaller environments,
like virts, to do the job, due to a potentially very limited number of
cores.

This patch set is my first pass designed to radically reform the
gfs2 glock state machine. Each patch slowly transforms it into more and
more of a real state machine.

The first few patches merely just untangle the mess. After that, each
patch removes an old-style state and adds it as a new state for the new
state machine.

The result is a state machine that's readable, and each state is now
clearly labeled and more obvious what it's doing and what other state
it can transition to.

One primary goal here is to leave the logic completely unchanged. I
wanted to make it so that code reviewers could check to make sure I
did not break today's existing logic. I do, however, have a few
optimizations at the end. So I'm not trying to fix any bugs here.
I'm just untangling spaghetti.

Another goal was to make each patch as small and digestable as possible
to avoid any hand waving.

I'm planning future patches to reduce the context switches by making
the state machine not rely as much on the glock work queue. These
patches will speed up glocks by reducing the work queue delays, by
executing the state machine from within the process context. For
example, there are only a few cases in which we need to ensure a
glock demote happen later (minimum hold time).
---
Bob Peterson (12):
  GFS2: Make do_xmote determine its own gh parameter
  GFS2: Eliminate a goto in finish_xmote
  GFS2: Baby step toward a real state machine: finish_xmote
  GFS2: Add do_xmote states to state machine
  GFS2: Make do_xmote not call the state machine again
  GFS2: Add blocking and non-blocking demote to state machine
  GFS2: Add a new GL_ST_PROMOTE state to glock state machine
  GFS2: Replace run_queue with new GL_ST_RUN state in state machine
  GFS2: Reduce redundancy in GL_ST_DEMOTE_NONBLOCK state
  GFS2: Reduce glock_work_func to a single call to state_machine
  GFS2: Add new GL_ST_UNLOCK state to reduce calls to the __ version
  GFS2: Optimization of GL_ST_UNLOCK state

 fs/gfs2/glock.c  | 319 ++++++++++++++++++++++++++++++-----------------
 fs/gfs2/glock.h  |  14 +++
 fs/gfs2/incore.h |   1 +
 3 files changed, 218 insertions(+), 116 deletions(-)

-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 01/12] GFS2: Make do_xmote determine its own gh parameter

[Bob Peterson]

This is another baby step toward a better glock state machine.
Before this patch, do_xmote was called with a gh parameter, but
only for promotes, not demotes. This patch allows do_xmote to
determine the gh autonomously.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index d356157632fe..8d0c467f8c9a 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -60,7 +60,7 @@ struct gfs2_glock_iter {
 
 typedef void (*glock_examiner) (struct gfs2_glock * gl);
 
-static void do_xmote(struct gfs2_glock *gl, struct gfs2_holder *gh, unsigned int target);
+static void do_xmote(struct gfs2_glock *gl, unsigned int target);
 
 static struct dentry *gfs2_root;
 static struct workqueue_struct *glock_workqueue;
@@ -486,12 +486,12 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 		/* Unlocked due to conversion deadlock, try again */
 		case LM_ST_UNLOCKED:
 retry:
-			do_xmote(gl, gh, gl->gl_target);
+			do_xmote(gl, gl->gl_target);
 			break;
 		/* Conversion fails, unlock and try again */
 		case LM_ST_SHARED:
 		case LM_ST_DEFERRED:
-			do_xmote(gl, gh, LM_ST_UNLOCKED);
+			do_xmote(gl, LM_ST_UNLOCKED);
 			break;
 		default: /* Everything else */
 			pr_err("wanted %u got %u\n", gl->gl_target, state);
@@ -527,17 +527,17 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 /**
  * do_xmote - Calls the DLM to change the state of a lock
  * @gl: The lock state
- * @gh: The holder (only for promotes)
  * @target: The target lock state
  *
  */
 
-static void do_xmote(struct gfs2_glock *gl, struct gfs2_holder *gh, unsigned int target)
+static void do_xmote(struct gfs2_glock *gl, unsigned int target)
 __releases(&gl->gl_lockref.lock)
 __acquires(&gl->gl_lockref.lock)
 {
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
+	struct gfs2_holder *gh = find_first_waiter(gl);
 	unsigned int lck_flags = (unsigned int)(gh ? gh->gh_flags : 0);
 	int ret;
 
@@ -658,7 +658,7 @@ __acquires(&gl->gl_lockref.lock)
 		if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
 			do_error(gl, 0); /* Fail queued try locks */
 	}
-	do_xmote(gl, gh, gl->gl_target);
+	do_xmote(gl, gl->gl_target);
 	return;
 }
 
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 02/12] GFS2: Eliminate a goto in finish_xmote

[Bob Peterson]

This is another baby step toward a better glock state machine.
This patch eliminates a goto in function finish_xmote so we can
begin unraveling the cryptic logic with later patches.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8d0c467f8c9a..8f2721515ff5 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -472,11 +472,11 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 					list_move_tail(&gh->gh_list, &gl->gl_holders);
 				gh = find_first_waiter(gl);
 				gl->gl_target = gh->gh_state;
-				goto retry;
-			}
-			/* Some error or failed "try lock" - report it */
-			if ((ret & LM_OUT_ERROR) ||
-			    (gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB))) {
+				state = LM_ST_UNLOCKED;
+			} else if ((ret & LM_OUT_ERROR) ||
+				   (gh->gh_flags & (LM_FLAG_TRY |
+						    LM_FLAG_TRY_1CB))) {
+				/* An error or failed "try lock" - report it */
 				gl->gl_target = gl->gl_state;
 				do_error(gl, ret);
 				goto out;
@@ -485,7 +485,6 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 		switch(state) {
 		/* Unlocked due to conversion deadlock, try again */
 		case LM_ST_UNLOCKED:
-retry:
 			do_xmote(gl, gl->gl_target);
 			break;
 		/* Conversion fails, unlock and try again */
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 03/12] GFS2: Baby step toward a real state machine: finish_xmote

[Bob Peterson]

This patch adds a new function state_machine and some hooks to
call it. For this early version, we've only got two states:
idle and finish_xmote. Later, many more will be added.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c  | 74 +++++++++++++++++++++++++++++++++++++++---------
 fs/gfs2/glock.h  |  5 ++++
 fs/gfs2/incore.h |  1 +
 3 files changed, 66 insertions(+), 14 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 8f2721515ff5..b5ba0abca51f 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -61,6 +61,7 @@ struct gfs2_glock_iter {
 typedef void (*glock_examiner) (struct gfs2_glock * gl);
 
 static void do_xmote(struct gfs2_glock *gl, unsigned int target);
+static void state_machine(struct gfs2_glock *gl, int new_state);
 
 static struct dentry *gfs2_root;
 static struct workqueue_struct *glock_workqueue;
@@ -442,18 +443,16 @@ static void gfs2_demote_wake(struct gfs2_glock *gl)
 /**
  * finish_xmote - The DLM has replied to one of our lock requests
  * @gl: The glock
- * @ret: The status from the DLM
  *
  */
 
-static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
+static void finish_xmote(struct gfs2_glock *gl)
 {
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
 	struct gfs2_holder *gh;
-	unsigned state = ret & LM_OUT_ST_MASK;
+	unsigned state = gl->gl_reply & LM_OUT_ST_MASK;
 	int rv;
 
-	spin_lock(&gl->gl_lockref.lock);
 	trace_gfs2_glock_state_change(gl, state);
 	state_change(gl, state);
 	gh = find_first_waiter(gl);
@@ -467,18 +466,18 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 	if (unlikely(state != gl->gl_target)) {
 		if (gh && !test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags)) {
 			/* move to back of queue and try next entry */
-			if (ret & LM_OUT_CANCELED) {
+			if (gl->gl_reply & LM_OUT_CANCELED) {
 				if ((gh->gh_flags & LM_FLAG_PRIORITY) == 0)
 					list_move_tail(&gh->gh_list, &gl->gl_holders);
 				gh = find_first_waiter(gl);
 				gl->gl_target = gh->gh_state;
 				state = LM_ST_UNLOCKED;
-			} else if ((ret & LM_OUT_ERROR) ||
+			} else if ((gl->gl_reply & LM_OUT_ERROR) ||
 				   (gh->gh_flags & (LM_FLAG_TRY |
 						    LM_FLAG_TRY_1CB))) {
 				/* An error or failed "try lock" - report it */
 				gl->gl_target = gl->gl_state;
-				do_error(gl, ret);
+				do_error(gl, gl->gl_reply);
 				goto out;
 			}
 		}
@@ -496,7 +495,6 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 			pr_err("wanted %u got %u\n", gl->gl_target, state);
 			GLOCK_BUG_ON(gl, 1);
 		}
-		spin_unlock(&gl->gl_lockref.lock);
 		return;
 	}
 
@@ -515,12 +513,10 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 		}
 		rv = do_promote(gl);
 		if (rv == 2)
-			goto out_locked;
+			return;
 	}
 out:
 	clear_bit(GLF_LOCK, &gl->gl_flags);
-out_locked:
-	spin_unlock(&gl->gl_lockref.lock);
 }
 
 /**
@@ -572,7 +568,8 @@ __acquires(&gl->gl_lockref.lock)
 		if (ret == -EINVAL && gl->gl_target == LM_ST_UNLOCKED &&
 		    target == LM_ST_UNLOCKED &&
 		    test_bit(SDF_SKIP_DLM_UNLOCK, &sdp->sd_flags)) {
-			finish_xmote(gl, target);
+			gl->gl_reply = target;
+			state_machine(gl, GL_ST_FINISH_XMOTE);
 			gfs2_glock_queue_work(gl, 0);
 		}
 		else if (ret) {
@@ -581,7 +578,8 @@ __acquires(&gl->gl_lockref.lock)
 						   &sdp->sd_flags));
 		}
 	} else { /* lock_nolock */
-		finish_xmote(gl, target);
+		gl->gl_reply = target;
+		state_machine(gl, GL_ST_FINISH_XMOTE);
 		gfs2_glock_queue_work(gl, 0);
 	}
 
@@ -661,6 +659,53 @@ __acquires(&gl->gl_lockref.lock)
 	return;
 }
 
+/**
+ * __state_machine - the glock state machine
+ * @gl: pointer to the glock we are transitioning
+ * @new_state: The new state we need to execute
+ *
+ * This function handles state transitions for glocks.
+ * When the state_machine is called, it's given a new state that needs to be
+ * handled, but only after it becomes idle from the last call. Once called,
+ * it keeps running until the state transitions have all been resolved.
+ * The lock might be released inside some of the states, so we may need react
+ * to state changes from other calls.
+ */
+static void __state_machine(struct gfs2_glock *gl, int new_state)
+{
+	BUG_ON(!spin_is_locked(&gl->gl_lockref.lock));
+
+	do {
+		switch (gl->gl_mch) {
+		case GL_ST_IDLE:
+			gl->gl_mch = new_state;
+			new_state = GL_ST_IDLE;
+			break;
+
+		case GL_ST_FINISH_XMOTE:
+			gl->gl_mch = GL_ST_IDLE;
+			finish_xmote(gl);
+			break;
+		}
+	} while (gl->gl_mch != GL_ST_IDLE);
+}
+
+/**
+ * state_machine - the glock state machine
+ * @gl: pointer to the glock we are transitioning
+ * @new_state: The new state we need to execute
+ *
+ * Just like __state_machine but it acquires the gl_lockref lock
+ */
+static void state_machine(struct gfs2_glock *gl, int new_state)
+__releases(&gl->gl_lockref.lock)
+__acquires(&gl->gl_lockref.lock)
+{
+	spin_lock(&gl->gl_lockref.lock);
+	__state_machine(gl, new_state);
+	spin_unlock(&gl->gl_lockref.lock);
+}
+
 static void delete_work_func(struct work_struct *work)
 {
 	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_delete);
@@ -690,7 +735,7 @@ static void glock_work_func(struct work_struct *work)
 	unsigned int drop_refs = 1;
 
 	if (test_and_clear_bit(GLF_REPLY_PENDING, &gl->gl_flags)) {
-		finish_xmote(gl, gl->gl_reply);
+		state_machine(gl, GL_ST_FINISH_XMOTE);
 		drop_refs++;
 	}
 	spin_lock(&gl->gl_lockref.lock);
@@ -817,6 +862,7 @@ int gfs2_glock_get(struct gfs2_sbd *sdp, u64 number,
 	}
 
 	atomic_inc(&sdp->sd_glock_disposal);
+	gl->gl_mch = GL_ST_IDLE;
 	gl->gl_node.next = NULL;
 	gl->gl_flags = 0;
 	gl->gl_name = name;
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 5e12220cc0c2..88043d610d64 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -120,6 +120,11 @@ enum {
 #define GL_GLOCK_HOLD_INCR       (long)(HZ / 20)
 #define GL_GLOCK_HOLD_DECR       (long)(HZ / 40)
 
+enum gl_machine_states {
+	GL_ST_IDLE = 0,		/* State machine idle; no transition needed */
+	GL_ST_FINISH_XMOTE = 1,	/* Promotion/demotion complete */
+};
+
 struct lm_lockops {
 	const char *lm_proto_name;
 	int (*lm_mount) (struct gfs2_sbd *sdp, const char *table);
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index b50908211b69..afd24c3e3cf5 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -371,6 +371,7 @@ struct gfs2_glock {
 	};
 	struct rcu_head gl_rcu;
 	struct rhash_head gl_node;
+	int gl_mch; /* state machine state */
 };
 
 #define GFS2_MIN_LVB_SIZE 32	/* Min size of LVB that gfs2 supports */
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 04/12] GFS2: Add do_xmote states to state machine

[Bob Peterson]

This patch adds two new states to the glock state machine. The
first is a normal call for gl_target. The second is an abnormal
call for cases in which dlm was unable to grant our request.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 37 ++++++++++++++++++++++++++++---------
 fs/gfs2/glock.h |  3 +++
 2 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index b5ba0abca51f..b87c51479a8a 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -60,7 +60,7 @@ struct gfs2_glock_iter {
 
 typedef void (*glock_examiner) (struct gfs2_glock * gl);
 
-static void do_xmote(struct gfs2_glock *gl, unsigned int target);
+static void __state_machine(struct gfs2_glock *gl, int new_state);
 static void state_machine(struct gfs2_glock *gl, int new_state);
 
 static struct dentry *gfs2_root;
@@ -444,9 +444,12 @@ static void gfs2_demote_wake(struct gfs2_glock *gl)
  * finish_xmote - The DLM has replied to one of our lock requests
  * @gl: The glock
  *
+ * Returns: -EAGAIN if do_xmote should be called next
+ *          -EBUSY if dlm could not satisfy the request
+ *          else 0
  */
 
-static void finish_xmote(struct gfs2_glock *gl)
+static int finish_xmote(struct gfs2_glock *gl)
 {
 	const struct gfs2_glock_operations *glops = gl->gl_ops;
 	struct gfs2_holder *gh;
@@ -484,18 +487,16 @@ static void finish_xmote(struct gfs2_glock *gl)
 		switch(state) {
 		/* Unlocked due to conversion deadlock, try again */
 		case LM_ST_UNLOCKED:
-			do_xmote(gl, gl->gl_target);
-			break;
+			return -EAGAIN;
 		/* Conversion fails, unlock and try again */
 		case LM_ST_SHARED:
 		case LM_ST_DEFERRED:
-			do_xmote(gl, LM_ST_UNLOCKED);
-			break;
+			return -EBUSY;
 		default: /* Everything else */
 			pr_err("wanted %u got %u\n", gl->gl_target, state);
 			GLOCK_BUG_ON(gl, 1);
 		}
-		return;
+		return 0;
 	}
 
 	/* Fast path - we got what we asked for */
@@ -513,10 +514,11 @@ static void finish_xmote(struct gfs2_glock *gl)
 		}
 		rv = do_promote(gl);
 		if (rv == 2)
-			return;
+			return 0;
 	}
 out:
 	clear_bit(GLF_LOCK, &gl->gl_flags);
+	return 0;
 }
 
 /**
@@ -655,7 +657,7 @@ __acquires(&gl->gl_lockref.lock)
 		if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
 			do_error(gl, 0); /* Fail queued try locks */
 	}
-	do_xmote(gl, gl->gl_target);
+	__state_machine(gl, GL_ST_DO_XMOTE);
 	return;
 }
 
@@ -673,6 +675,8 @@ __acquires(&gl->gl_lockref.lock)
  */
 static void __state_machine(struct gfs2_glock *gl, int new_state)
 {
+	int ret;
+
 	BUG_ON(!spin_is_locked(&gl->gl_lockref.lock));
 
 	do {
@@ -684,6 +688,21 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
 
 		case GL_ST_FINISH_XMOTE:
 			gl->gl_mch = GL_ST_IDLE;
+			ret = finish_xmote(gl);
+			if (ret == -EBUSY)
+				gl->gl_mch = GL_ST_DO_XMOTE_UNLOCK;
+			else if (ret == -EAGAIN)
+				gl->gl_mch = GL_ST_DO_XMOTE;
+			break;
+
+		case GL_ST_DO_XMOTE:
+			gl->gl_mch = GL_ST_IDLE;
+			do_xmote(gl, gl->gl_target);
+			break;
+
+		case GL_ST_DO_XMOTE_UNLOCK:
+			gl->gl_mch = GL_ST_IDLE;
+			do_xmote(gl, LM_ST_UNLOCKED);
 			finish_xmote(gl);
 			break;
 		}
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 88043d610d64..8333bbc5a197 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -123,6 +123,9 @@ enum {
 enum gl_machine_states {
 	GL_ST_IDLE = 0,		/* State machine idle; no transition needed */
 	GL_ST_FINISH_XMOTE = 1,	/* Promotion/demotion complete */
+	GL_ST_DO_XMOTE = 2,	/* Promote or demote a waiter */
+	GL_ST_DO_XMOTE_UNLOCK = 3, /* Promote or demote a waiter after a failed
+				      state change attempt from dlm. */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 05/12] GFS2: Make do_xmote not call the state machine again

[Bob Peterson]

Before this patch, the state machine could call do_xmote which,
in turn, could call back into the state machine. This patch unravels
the logic so instead it sends back an -EAGAIN return code, which
signals the state machine to loop under the new state.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index b87c51479a8a..47b023ed8f51 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -526,9 +526,11 @@ static int finish_xmote(struct gfs2_glock *gl)
  * @gl: The lock state
  * @target: The target lock state
  *
+ * Returns: 0 if the lock is pending, or
+ *          -EAGAIN if we need to run the state machine again to finish_xmote
  */
 
-static void do_xmote(struct gfs2_glock *gl, unsigned int target)
+static int do_xmote(struct gfs2_glock *gl, unsigned int target)
 __releases(&gl->gl_lockref.lock)
 __acquires(&gl->gl_lockref.lock)
 {
@@ -536,11 +538,11 @@ __acquires(&gl->gl_lockref.lock)
 	struct gfs2_sbd *sdp = gl->gl_name.ln_sbd;
 	struct gfs2_holder *gh = find_first_waiter(gl);
 	unsigned int lck_flags = (unsigned int)(gh ? gh->gh_flags : 0);
-	int ret;
+	int ret = 0;
 
 	if (unlikely(test_bit(SDF_SHUTDOWN, &sdp->sd_flags)) &&
 	    target != LM_ST_UNLOCKED)
-		return;
+		return 0;
 	lck_flags &= (LM_FLAG_TRY | LM_FLAG_TRY_1CB | LM_FLAG_NOEXP |
 		      LM_FLAG_PRIORITY);
 	GLOCK_BUG_ON(gl, gl->gl_state == target);
@@ -571,21 +573,23 @@ __acquires(&gl->gl_lockref.lock)
 		    target == LM_ST_UNLOCKED &&
 		    test_bit(SDF_SKIP_DLM_UNLOCK, &sdp->sd_flags)) {
 			gl->gl_reply = target;
-			state_machine(gl, GL_ST_FINISH_XMOTE);
+			ret = -EAGAIN;
 			gfs2_glock_queue_work(gl, 0);
 		}
 		else if (ret) {
 			pr_err("lm_lock ret %d\n", ret);
 			GLOCK_BUG_ON(gl, !test_bit(SDF_SHUTDOWN,
 						   &sdp->sd_flags));
+			ret = 0;
 		}
 	} else { /* lock_nolock */
 		gl->gl_reply = target;
-		state_machine(gl, GL_ST_FINISH_XMOTE);
+		ret = -EAGAIN;
 		gfs2_glock_queue_work(gl, 0);
 	}
 
 	spin_lock(&gl->gl_lockref.lock);
+	return ret;
 }
 
 /**
@@ -697,13 +701,16 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
 
 		case GL_ST_DO_XMOTE:
 			gl->gl_mch = GL_ST_IDLE;
-			do_xmote(gl, gl->gl_target);
+			ret = do_xmote(gl, gl->gl_target);
+			if (ret == -EAGAIN)
+				gl->gl_mch = GL_ST_FINISH_XMOTE;
 			break;
 
 		case GL_ST_DO_XMOTE_UNLOCK:
 			gl->gl_mch = GL_ST_IDLE;
-			do_xmote(gl, LM_ST_UNLOCKED);
-			finish_xmote(gl);
+			ret = do_xmote(gl, LM_ST_UNLOCKED);
+			if (ret == -EAGAIN)
+				gl->gl_mch = GL_ST_FINISH_XMOTE;
 			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE);
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 06/12] GFS2: Add blocking and non-blocking demote to state machine

[Bob Peterson]

Before this patch, function run_queue would do special processing
before calling the state machine for the blocking and non-blocking
demote-in-progress cases. This function rolls those functions into
the state machine, which will allow us to eventually simplify with
later patches.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 44 +++++++++++++++++++++++++++++---------------
 fs/gfs2/glock.h |  2 ++
 2 files changed, 31 insertions(+), 15 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 47b023ed8f51..344ef774f68f 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -630,21 +630,9 @@ __acquires(&gl->gl_lockref.lock)
 
 	if (test_bit(GLF_DEMOTE, &gl->gl_flags) &&
 	    gl->gl_demote_state != gl->gl_state) {
-		if (find_first_holder(gl)) {
-			clear_bit(GLF_LOCK, &gl->gl_flags);
-			smp_mb__after_atomic();
-			return;
-		}
-		if (nonblock) {
-			clear_bit(GLF_LOCK, &gl->gl_flags);
-			smp_mb__after_atomic();
-			gl->gl_lockref.count++;
-			__gfs2_glock_queue_work(gl, 0);
-			return;
-		}
-		set_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags);
-		GLOCK_BUG_ON(gl, gl->gl_demote_state == LM_ST_EXCLUSIVE);
-		gl->gl_target = gl->gl_demote_state;
+		__state_machine(gl, nonblock ? GL_ST_DEMOTE_NONBLOCK :
+				GL_ST_BLOCKING_DEMOTE);
+		return;
 	} else {
 		if (test_bit(GLF_DEMOTE, &gl->gl_flags))
 			gfs2_demote_wake(gl);
@@ -712,6 +700,32 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
 			if (ret == -EAGAIN)
 				gl->gl_mch = GL_ST_FINISH_XMOTE;
 			break;
+
+		case GL_ST_BLOCKING_DEMOTE:
+			gl->gl_mch = GL_ST_IDLE;
+			if (find_first_holder(gl)) {
+				clear_bit(GLF_LOCK, &gl->gl_flags);
+				smp_mb__after_atomic();
+				break;
+			}
+			set_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags);
+			GLOCK_BUG_ON(gl, gl->gl_demote_state == LM_ST_EXCLUSIVE);
+			gl->gl_target = gl->gl_demote_state;
+			gl->gl_mch = GL_ST_DO_XMOTE;
+			break;
+
+		case GL_ST_DEMOTE_NONBLOCK:
+			gl->gl_mch = GL_ST_IDLE;
+			if (find_first_holder(gl)) {
+				clear_bit(GLF_LOCK, &gl->gl_flags);
+				smp_mb__after_atomic();
+				break;
+			}
+			clear_bit(GLF_LOCK, &gl->gl_flags);
+			smp_mb__after_atomic();
+			gl->gl_lockref.count++;
+			__gfs2_glock_queue_work(gl, 0);
+			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE);
 }
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 8333bbc5a197..c8b704a73638 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -126,6 +126,8 @@ enum gl_machine_states {
 	GL_ST_DO_XMOTE = 2,	/* Promote or demote a waiter */
 	GL_ST_DO_XMOTE_UNLOCK = 3, /* Promote or demote a waiter after a failed
 				      state change attempt from dlm. */
+	GL_ST_DEMOTE_NONBLOCK = 4, /* Demote is in progress - non-blocking */
+	GL_ST_BLOCKING_DEMOTE = 5, /* Demote is in progress - blocking */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 07/12] GFS2: Add a new GL_ST_PROMOTE state to glock state machine

[Bob Peterson]

Before this patch, function run_queue did a bunch of logic when
glocks are being promoted. This patch moves the logic to the glock
state machine and simplifies run_queue further.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 46 +++++++++++++++++++++++-----------------------
 fs/gfs2/glock.h |  1 +
 2 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 344ef774f68f..869649e15f63 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -620,37 +620,17 @@ static void run_queue(struct gfs2_glock *gl, const int nonblock)
 __releases(&gl->gl_lockref.lock)
 __acquires(&gl->gl_lockref.lock)
 {
-	struct gfs2_holder *gh = NULL;
-	int ret;
-
 	if (test_and_set_bit(GLF_LOCK, &gl->gl_flags))
 		return;
 
 	GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags));
 
 	if (test_bit(GLF_DEMOTE, &gl->gl_flags) &&
-	    gl->gl_demote_state != gl->gl_state) {
+	    gl->gl_demote_state != gl->gl_state)
 		__state_machine(gl, nonblock ? GL_ST_DEMOTE_NONBLOCK :
 				GL_ST_BLOCKING_DEMOTE);
-		return;
-	} else {
-		if (test_bit(GLF_DEMOTE, &gl->gl_flags))
-			gfs2_demote_wake(gl);
-		ret = do_promote(gl);
-		if (ret == 0) {
-			clear_bit(GLF_LOCK, &gl->gl_flags);
-			smp_mb__after_atomic();
-			return;
-		}
-		if (ret == 2)
-			return;
-		gh = find_first_waiter(gl);
-		gl->gl_target = gh->gh_state;
-		if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
-			do_error(gl, 0); /* Fail queued try locks */
-	}
-	__state_machine(gl, GL_ST_DO_XMOTE);
-	return;
+	else
+		__state_machine(gl, GL_ST_PROMOTE);
 }
 
 /**
@@ -667,6 +647,7 @@ __acquires(&gl->gl_lockref.lock)
  */
 static void __state_machine(struct gfs2_glock *gl, int new_state)
 {
+	struct gfs2_holder *gh = NULL;
 	int ret;
 
 	BUG_ON(!spin_is_locked(&gl->gl_lockref.lock));
@@ -726,6 +707,25 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
 			gl->gl_lockref.count++;
 			__gfs2_glock_queue_work(gl, 0);
 			break;
+
+		case GL_ST_PROMOTE:
+			gl->gl_mch = GL_ST_IDLE;
+			if (test_bit(GLF_DEMOTE, &gl->gl_flags))
+				gfs2_demote_wake(gl);
+			ret = do_promote(gl);
+			if (ret == 0) {
+				clear_bit(GLF_LOCK, &gl->gl_flags);
+				smp_mb__after_atomic();
+				break;
+			}
+			if (ret == 2)
+				break;
+			gh = find_first_waiter(gl);
+			gl->gl_target = gh->gh_state;
+			if (!(gh->gh_flags & (LM_FLAG_TRY | LM_FLAG_TRY_1CB)))
+				do_error(gl, 0); /* Fail queued try locks */
+			gl->gl_mch = GL_ST_DO_XMOTE;
+			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE);
 }
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index c8b704a73638..76db63d1efae 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -128,6 +128,7 @@ enum gl_machine_states {
 				      state change attempt from dlm. */
 	GL_ST_DEMOTE_NONBLOCK = 4, /* Demote is in progress - non-blocking */
 	GL_ST_BLOCKING_DEMOTE = 5, /* Demote is in progress - blocking */
+	GL_ST_PROMOTE = 6,	   /* Promote the lock */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 08/12] GFS2: Replace run_queue with new GL_ST_RUN state in state machine

[Bob Peterson]

Now we replace the run_queue function with a new state in the glock
state machine which does the same thing.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 60 +++++++++++++++++++++----------------------------
 fs/gfs2/glock.h |  1 +
 2 files changed, 27 insertions(+), 34 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 869649e15f63..bc5ac558a917 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -60,9 +60,6 @@ struct gfs2_glock_iter {
 
 typedef void (*glock_examiner) (struct gfs2_glock * gl);
 
-static void __state_machine(struct gfs2_glock *gl, int new_state);
-static void state_machine(struct gfs2_glock *gl, int new_state);
-
 static struct dentry *gfs2_root;
 static struct workqueue_struct *glock_workqueue;
 struct workqueue_struct *gfs2_delete_workqueue;
@@ -609,34 +606,11 @@ static inline struct gfs2_holder *find_first_holder(const struct gfs2_glock *gl)
 	return NULL;
 }
 
-/**
- * run_queue - do all outstanding tasks related to a glock
- * @gl: The glock in question
- * @nonblock: True if we must not block in run_queue
- *
- */
-
-static void run_queue(struct gfs2_glock *gl, const int nonblock)
-__releases(&gl->gl_lockref.lock)
-__acquires(&gl->gl_lockref.lock)
-{
-	if (test_and_set_bit(GLF_LOCK, &gl->gl_flags))
-		return;
-
-	GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS, &gl->gl_flags));
-
-	if (test_bit(GLF_DEMOTE, &gl->gl_flags) &&
-	    gl->gl_demote_state != gl->gl_state)
-		__state_machine(gl, nonblock ? GL_ST_DEMOTE_NONBLOCK :
-				GL_ST_BLOCKING_DEMOTE);
-	else
-		__state_machine(gl, GL_ST_PROMOTE);
-}
-
 /**
  * __state_machine - the glock state machine
  * @gl: pointer to the glock we are transitioning
  * @new_state: The new state we need to execute
+ * @nonblock: True if we must not block while demoting
  *
  * This function handles state transitions for glocks.
  * When the state_machine is called, it's given a new state that needs to be
@@ -645,7 +619,8 @@ __acquires(&gl->gl_lockref.lock)
  * The lock might be released inside some of the states, so we may need react
  * to state changes from other calls.
  */
-static void __state_machine(struct gfs2_glock *gl, int new_state)
+static void __state_machine(struct gfs2_glock *gl, int new_state,
+			    const int nonblock)
 {
 	struct gfs2_holder *gh = NULL;
 	int ret;
@@ -726,6 +701,22 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
 				do_error(gl, 0); /* Fail queued try locks */
 			gl->gl_mch = GL_ST_DO_XMOTE;
 			break;
+
+		case GL_ST_RUN:
+			gl->gl_mch = GL_ST_IDLE;
+			if (test_and_set_bit(GLF_LOCK, &gl->gl_flags))
+				break;
+
+			GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS,
+						  &gl->gl_flags));
+
+			if (test_bit(GLF_DEMOTE, &gl->gl_flags) &&
+			    gl->gl_demote_state != gl->gl_state)
+				gl->gl_mch = nonblock ? GL_ST_DEMOTE_NONBLOCK :
+					GL_ST_BLOCKING_DEMOTE;
+			else
+				gl->gl_mch = GL_ST_PROMOTE;
+			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE);
 }
@@ -737,12 +728,13 @@ static void __state_machine(struct gfs2_glock *gl, int new_state)
  *
  * Just like __state_machine but it acquires the gl_lockref lock
  */
-static void state_machine(struct gfs2_glock *gl, int new_state)
+static void state_machine(struct gfs2_glock *gl, int new_state,
+			  const int nonblock)
 __releases(&gl->gl_lockref.lock)
 __acquires(&gl->gl_lockref.lock)
 {
 	spin_lock(&gl->gl_lockref.lock);
-	__state_machine(gl, new_state);
+	__state_machine(gl, new_state, nonblock);
 	spin_unlock(&gl->gl_lockref.lock);
 }
 
@@ -775,7 +767,7 @@ static void glock_work_func(struct work_struct *work)
 	unsigned int drop_refs = 1;
 
 	if (test_and_clear_bit(GLF_REPLY_PENDING, &gl->gl_flags)) {
-		state_machine(gl, GL_ST_FINISH_XMOTE);
+		state_machine(gl, GL_ST_FINISH_XMOTE, 0);
 		drop_refs++;
 	}
 	spin_lock(&gl->gl_lockref.lock);
@@ -793,7 +785,7 @@ static void glock_work_func(struct work_struct *work)
 			set_bit(GLF_DEMOTE, &gl->gl_flags);
 		}
 	}
-	run_queue(gl, 0);
+	__state_machine(gl, GL_ST_RUN, 0);
 	if (delay) {
 		/* Keep one glock reference for the work we requeue. */
 		drop_refs--;
@@ -1188,7 +1180,7 @@ int gfs2_glock_nq(struct gfs2_holder *gh)
 		gl->gl_lockref.count++;
 		__gfs2_glock_queue_work(gl, 0);
 	}
-	run_queue(gl, 1);
+	__state_machine(gl, GL_ST_RUN, 1);
 	spin_unlock(&gl->gl_lockref.lock);
 
 	if (!(gh->gh_flags & GL_ASYNC))
@@ -1732,7 +1724,7 @@ void gfs2_glock_finish_truncate(struct gfs2_inode *ip)
 
 	spin_lock(&gl->gl_lockref.lock);
 	clear_bit(GLF_LOCK, &gl->gl_flags);
-	run_queue(gl, 1);
+	__state_machine(gl, GL_ST_RUN, 1);
 	spin_unlock(&gl->gl_lockref.lock);
 }
 
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 76db63d1efae..0239d3a9040c 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -129,6 +129,7 @@ enum gl_machine_states {
 	GL_ST_DEMOTE_NONBLOCK = 4, /* Demote is in progress - non-blocking */
 	GL_ST_BLOCKING_DEMOTE = 5, /* Demote is in progress - blocking */
 	GL_ST_PROMOTE = 6,	   /* Promote the lock */
+	GL_ST_RUN = 7,		/* "Run" or progress the lock */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 09/12] GFS2: Reduce redundancy in GL_ST_DEMOTE_NONBLOCK state

[Bob Peterson]

This patch just simplifies the path through the GL_ST_DEMOTE_NONBLOCK
state in the state machine. Regardless of whether a holder is found,
it still needs to clear the GLF_LOCK bit. But we need to be careful
to do it check for a holder before we release GLF_LOCK.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index bc5ac558a917..4b910bd126f5 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -672,15 +672,13 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 
 		case GL_ST_DEMOTE_NONBLOCK:
 			gl->gl_mch = GL_ST_IDLE;
-			if (find_first_holder(gl)) {
-				clear_bit(GLF_LOCK, &gl->gl_flags);
-				smp_mb__after_atomic();
-				break;
-			}
+			gh = find_first_holder(gl);
 			clear_bit(GLF_LOCK, &gl->gl_flags);
 			smp_mb__after_atomic();
-			gl->gl_lockref.count++;
-			__gfs2_glock_queue_work(gl, 0);
+			if (!gh) {
+				gl->gl_lockref.count++;
+				__gfs2_glock_queue_work(gl, 0);
+			}
 			break;
 
 		case GL_ST_PROMOTE:
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 10/12] GFS2: Reduce glock_work_func to a single call to state_machine

[Bob Peterson]

Before this patch, function glock_work_func would call into the
state machine for GL_FINISH_XMOTE, then GL_RUN, plus some work
related to dropping references and requeueing itself. This patch
moves all that functionality to a new GL_WORK state. This reduces
glock_work_func to a single call to the state machine.

The goal here is to allow for patches in the future that will
bypass the delayed workqueue altogether to improve performance.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 97 ++++++++++++++++++++++++++++---------------------
 fs/gfs2/glock.h |  1 +
 2 files changed, 56 insertions(+), 42 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 4b910bd126f5..e20bb1222c01 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -623,9 +623,25 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 			    const int nonblock)
 {
 	struct gfs2_holder *gh = NULL;
+	unsigned long delay = 0;
+	unsigned int drop_refs = 0;
 	int ret;
 
 	BUG_ON(!spin_is_locked(&gl->gl_lockref.lock));
+	if (new_state == GL_ST_WORK) {
+		drop_refs = 1;
+		/**
+		 * Before we can do the rest of the work, we need to finish
+		 * any xmotes due to a reply from dlm. Note that since we did
+		 * not change new_state, we'll drop back into GL_ST_WORK when
+		 * the GL_ST_FINISH_XMOTE completes its cycle, regardless
+		 * of how many other states it passes through.
+		 */
+		if (test_and_clear_bit(GLF_REPLY_PENDING, &gl->gl_flags)) {
+			gl->gl_mch = GL_ST_FINISH_XMOTE;
+			drop_refs++;
+		}
+	}
 
 	do {
 		switch (gl->gl_mch) {
@@ -715,8 +731,41 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 			else
 				gl->gl_mch = GL_ST_PROMOTE;
 			break;
+
+		case GL_ST_WORK:
+			if (test_bit(GLF_PENDING_DEMOTE, &gl->gl_flags) &&
+			    gl->gl_state != LM_ST_UNLOCKED &&
+			    gl->gl_demote_state != LM_ST_EXCLUSIVE) {
+				unsigned long holdtime, now = jiffies;
+
+				holdtime = gl->gl_tchange + gl->gl_hold_time;
+				if (time_before(now, holdtime))
+					delay = holdtime - now;
+
+				if (!delay) {
+					clear_bit(GLF_PENDING_DEMOTE, &gl->gl_flags);
+					set_bit(GLF_DEMOTE, &gl->gl_flags);
+				}
+			}
+			gl->gl_mch = GL_ST_RUN;
+			break;
 		}
-	} while (gl->gl_mch != GL_ST_IDLE);
+	} while (gl->gl_mch != GL_ST_IDLE || new_state != GL_ST_IDLE);
+
+	/* Now check if a delayed re-queue of the work is needed */
+	if (delay) {
+		/* Keep one glock reference for the work we requeue. */
+		drop_refs--;
+		if (gl->gl_name.ln_type != LM_TYPE_INODE)
+			delay = 0;
+		__gfs2_glock_queue_work(gl, delay);
+	}
+	/*
+	 * Drop the remaining glock references manually here. (Mind that
+	 * __gfs2_glock_queue_work depends on the lockref spinlock begin held
+	 * here as well.)
+	 */
+	gl->gl_lockref.count -= drop_refs;
 }
 
 /**
@@ -733,6 +782,10 @@ __acquires(&gl->gl_lockref.lock)
 {
 	spin_lock(&gl->gl_lockref.lock);
 	__state_machine(gl, new_state, nonblock);
+	if (new_state == GL_ST_WORK && !gl->gl_lockref.count) {
+		__gfs2_glock_put(gl);
+		return;
+	}
 	spin_unlock(&gl->gl_lockref.lock);
 }
 
@@ -760,49 +813,9 @@ static void delete_work_func(struct work_struct *work)
 
 static void glock_work_func(struct work_struct *work)
 {
-	unsigned long delay = 0;
 	struct gfs2_glock *gl = container_of(work, struct gfs2_glock, gl_work.work);
-	unsigned int drop_refs = 1;
-
-	if (test_and_clear_bit(GLF_REPLY_PENDING, &gl->gl_flags)) {
-		state_machine(gl, GL_ST_FINISH_XMOTE, 0);
-		drop_refs++;
-	}
-	spin_lock(&gl->gl_lockref.lock);
-	if (test_bit(GLF_PENDING_DEMOTE, &gl->gl_flags) &&
-	    gl->gl_state != LM_ST_UNLOCKED &&
-	    gl->gl_demote_state != LM_ST_EXCLUSIVE) {
-		unsigned long holdtime, now = jiffies;
-
-		holdtime = gl->gl_tchange + gl->gl_hold_time;
-		if (time_before(now, holdtime))
-			delay = holdtime - now;
 
-		if (!delay) {
-			clear_bit(GLF_PENDING_DEMOTE, &gl->gl_flags);
-			set_bit(GLF_DEMOTE, &gl->gl_flags);
-		}
-	}
-	__state_machine(gl, GL_ST_RUN, 0);
-	if (delay) {
-		/* Keep one glock reference for the work we requeue. */
-		drop_refs--;
-		if (gl->gl_name.ln_type != LM_TYPE_INODE)
-			delay = 0;
-		__gfs2_glock_queue_work(gl, delay);
-	}
-
-	/*
-	 * Drop the remaining glock references manually here. (Mind that
-	 * __gfs2_glock_queue_work depends on the lockref spinlock begin held
-	 * here as well.)
-	 */
-	gl->gl_lockref.count -= drop_refs;
-	if (!gl->gl_lockref.count) {
-		__gfs2_glock_put(gl);
-		return;
-	}
-	spin_unlock(&gl->gl_lockref.lock);
+	state_machine(gl, GL_ST_WORK, 0);
 }
 
 static struct gfs2_glock *find_insert_glock(struct lm_lockname *name,
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 0239d3a9040c..0b1dffb92e8a 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -130,6 +130,7 @@ enum gl_machine_states {
 	GL_ST_BLOCKING_DEMOTE = 5, /* Demote is in progress - blocking */
 	GL_ST_PROMOTE = 6,	   /* Promote the lock */
 	GL_ST_RUN = 7,		/* "Run" or progress the lock */
+	GL_ST_WORK = 8,		/* Perform general glock work */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 11/12] GFS2: Add new GL_ST_UNLOCK state to reduce calls to the __ version

[Bob Peterson]

Before this patch, the truncate code called __state_machine but
only did the unlock of GLF_UNLOCK. This patch adds a new state
GL_ST_UNLOCK does the same thing, thus allowing it to call the
regular state_machine function. This reduces the calls to the
helper.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 11 ++++++-----
 fs/gfs2/glock.h |  1 +
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index e20bb1222c01..b7b8cf16d613 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -749,6 +749,11 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 			}
 			gl->gl_mch = GL_ST_RUN;
 			break;
+
+		case GL_ST_UNLOCK:
+			clear_bit(GLF_LOCK, &gl->gl_flags);
+			gl->gl_mch = GL_ST_RUN;
+			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE || new_state != GL_ST_IDLE);
 
@@ -773,7 +778,6 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
  * @gl: pointer to the glock we are transitioning
  * @new_state: The new state we need to execute
  *
- * Just like __state_machine but it acquires the gl_lockref lock
  */
 static void state_machine(struct gfs2_glock *gl, int new_state,
 			  const int nonblock)
@@ -1733,10 +1737,7 @@ void gfs2_glock_finish_truncate(struct gfs2_inode *ip)
 	ret = gfs2_truncatei_resume(ip);
 	gfs2_assert_withdraw(gl->gl_name.ln_sbd, ret == 0);
 
-	spin_lock(&gl->gl_lockref.lock);
-	clear_bit(GLF_LOCK, &gl->gl_flags);
-	__state_machine(gl, GL_ST_RUN, 1);
-	spin_unlock(&gl->gl_lockref.lock);
+	state_machine(gl, GL_ST_UNLOCK, 1);
 }
 
 static const char *state2str(unsigned state)
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 0b1dffb92e8a..5bbf3118c7c3 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -131,6 +131,7 @@ enum gl_machine_states {
 	GL_ST_PROMOTE = 6,	   /* Promote the lock */
 	GL_ST_RUN = 7,		/* "Run" or progress the lock */
 	GL_ST_WORK = 8,		/* Perform general glock work */
+	GL_ST_UNLOCK = 9,	/* Unlock then run */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 12/12] GFS2: Optimization of GL_ST_UNLOCK state

[Bob Peterson]

The previous patch added a new GL_ST_UNLOCK state that simply
cleared the GLF_LOCK bit, then went into the RUN state. Since the
RUN state immediately sets the GLF_LOCK again and does a few other
things, we can optimize it by putting GL_ST_UNLOCK in the middle
of the GL_ST_RUN state. This avoids unlock-then-lock and extra
checks and skips directly to the resulting promotion/demotion state.

Signed-off-by: Bob Peterson <rpeterso@redhat.com>
---
 fs/gfs2/glock.c | 6 ++----
 fs/gfs2/glock.h | 2 +-
 2 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index b7b8cf16d613..56db2e78c99f 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -723,7 +723,9 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 
 			GLOCK_BUG_ON(gl, test_bit(GLF_DEMOTE_IN_PROGRESS,
 						  &gl->gl_flags));
+			/* Fall through */
 
+		case GL_ST_UNLOCK:
 			if (test_bit(GLF_DEMOTE, &gl->gl_flags) &&
 			    gl->gl_demote_state != gl->gl_state)
 				gl->gl_mch = nonblock ? GL_ST_DEMOTE_NONBLOCK :
@@ -750,10 +752,6 @@ static void __state_machine(struct gfs2_glock *gl, int new_state,
 			gl->gl_mch = GL_ST_RUN;
 			break;
 
-		case GL_ST_UNLOCK:
-			clear_bit(GLF_LOCK, &gl->gl_flags);
-			gl->gl_mch = GL_ST_RUN;
-			break;
 		}
 	} while (gl->gl_mch != GL_ST_IDLE || new_state != GL_ST_IDLE);
 
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index 5bbf3118c7c3..e1a5bc2d3f76 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -131,7 +131,7 @@ enum gl_machine_states {
 	GL_ST_PROMOTE = 6,	   /* Promote the lock */
 	GL_ST_RUN = 7,		/* "Run" or progress the lock */
 	GL_ST_WORK = 8,		/* Perform general glock work */
-	GL_ST_UNLOCK = 9,	/* Unlock then run */
+	GL_ST_UNLOCK = 9,	/* Unlock the glock */
 };
 
 struct lm_lockops {
-- 
2.17.1

[Cluster-devel] [GFS2 PATCH 00/12] Radical Reform of glock state machine

[Steven Whitehouse]

Hi,


On 09/07/18 18:50, Bob Peterson wrote:
> GFS2 glocks go through a very complex and confusing series of steps to
> transition from one locking mode to another. For example, to transition
> from unlocked (UN) mode to Shared (SH) mode we need to "promote" the lock,
> and to do so requires a series of steps such as asking permission of DLM,
> fetching dlm responses, checking for remote demotion requests, granting
> multiple holders, and so forth.
>
> This is all managed by a large set of functions known as the glock state
> machine. Today, the glock state machine is a disorganized chaos of
> functions that are neither documented nor intuitive. It's a complete
> disaster that makes no sense at all to someone coming in to the code cold.
> For proof, you need only look at the fact that function do_xmote() can
> call function finish_xmote(), but finish_xmote() can call do_xmote() as
> well. It works really well, but the problem is: it's a house of cards.
> It's easy to misunderstand the intent and get it wrong, and if you try to
> make any little change, everything falls apart.
>
> The other problem is that today's glock state machine relies completely
> on the delayed work queue. That means you can't simply transition from
> one state to the next; you need to queue delayed work and have the
> delayed workqueue do the work. This requires an absurd number of context
> switches to make the simplest change to a glock. That creates a lot of
> unnecessary latency, and makes it much harder for smaller environments,
> like virts, to do the job, due to a potentially very limited number of
> cores.
>
> This patch set is my first pass designed to radically reform the
> gfs2 glock state machine. Each patch slowly transforms it into more and
> more of a real state machine.
>
> The first few patches merely just untangle the mess. After that, each
> patch removes an old-style state and adds it as a new state for the new
> state machine.
>
> The result is a state machine that's readable, and each state is now
> clearly labeled and more obvious what it's doing and what other state
> it can transition to.
>
> One primary goal here is to leave the logic completely unchanged. I
> wanted to make it so that code reviewers could check to make sure I
> did not break today's existing logic. I do, however, have a few
> optimizations at the end. So I'm not trying to fix any bugs here.
> I'm just untangling spaghetti.
>
> Another goal was to make each patch as small and digestable as possible
> to avoid any hand waving.
>
> I'm planning future patches to reduce the context switches by making
> the state machine not rely as much on the glock work queue. These
> patches will speed up glocks by reducing the work queue delays, by
> executing the state machine from within the process context. For
> example, there are only a few cases in which we need to ensure a
> glock demote happen later (minimum hold time).

It would be good to have some test results to show what effect this has 
on context switches. I suspect it will not make a great deal of 
difference until dlm has been improved in this area. Also, certain 
operations must be done under the workqueue, otherwise they will 
deadlock, so that is also something to take into account.

What might also be useful is a change in the delayed workqueue API to 
make it easier to use for our purposes, which I don't think are really 
that strange. The other option is to split out the workqueue and the 
timer, but it is probably better to have a generic API change to avoid 
some of the things we have to do in order to set the timer correctly,

Steve.

> ---
> Bob Peterson (12):
>    GFS2: Make do_xmote determine its own gh parameter
>    GFS2: Eliminate a goto in finish_xmote
>    GFS2: Baby step toward a real state machine: finish_xmote
>    GFS2: Add do_xmote states to state machine
>    GFS2: Make do_xmote not call the state machine again
>    GFS2: Add blocking and non-blocking demote to state machine
>    GFS2: Add a new GL_ST_PROMOTE state to glock state machine
>    GFS2: Replace run_queue with new GL_ST_RUN state in state machine
>    GFS2: Reduce redundancy in GL_ST_DEMOTE_NONBLOCK state
>    GFS2: Reduce glock_work_func to a single call to state_machine
>    GFS2: Add new GL_ST_UNLOCK state to reduce calls to the __ version
>    GFS2: Optimization of GL_ST_UNLOCK state
>
>   fs/gfs2/glock.c  | 319 ++++++++++++++++++++++++++++++-----------------
>   fs/gfs2/glock.h  |  14 +++
>   fs/gfs2/incore.h |   1 +
>   3 files changed, 218 insertions(+), 116 deletions(-)
>