[Cluster-devel] [PATCH 00/13] Radical Reform of glock state machine (take 2)

[Cluster-devel] [PATCH 00/13] Radical Reform of glock state machine (take 2)

[Bob Peterson]

This is a second go at my "Radical Reform of glock state machine"
patch set. It's been rebased to the latest git tree, and had a lot more
testing (from the actual for-next tree rather than testing the ports).
This includes performance testing using iozone.

The patches are unchanged from my original post on 09 July, except that
I've added one patch at the beginning, "Remove gotos from function run_queue."

Just to reiterate: The goal here is to preserve the current logic of
today's glock state machine, so no attempts have been made (yet) to
make it faster or more sane. The goal is to eventually allow gfs2 to
reduce its use of the glock workqueue by calling the state machine
directly from the locking process, thus reducing glock latency and
greatly reducing context switches. That will be another set of patches.

I'd like to get this pushed early in the cycle so it doesn't keep
getting pushed out for various merge window cycles.

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 (13):
  GFS2: Remove gotos from function run_queue
  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  | 327 +++++++++++++++++++++++++++++------------------
 fs/gfs2/glock.h  |  14 ++
 fs/gfs2/incore.h |   1 +
 3 files changed, 221 insertions(+), 121 deletions(-)

-- 
2.19.1

[Cluster-devel] [PATCH 01/13] GFS2: Remove gotos from function run_queue

[Bob Peterson]

This patch removes all the gotos from function run_queue and inlines
the code where necessary. This is a small step toward unravelling the
logic to reorganize the glock state machine.

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

diff --git a/fs/gfs2/glock.c b/fs/gfs2/glock.c
index 05431324b262..692784faa464 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -628,10 +628,18 @@ __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))
-			goto out_unlock;
-		if (nonblock)
-			goto out_sched;
+		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;
@@ -639,29 +647,19 @@ __acquires(&gl->gl_lockref.lock)
 		if (test_bit(GLF_DEMOTE, &gl->gl_flags))
 			gfs2_demote_wake(gl);
 		ret = do_promote(gl);
-		if (ret == 0)
-			goto out_unlock;
+		if (ret == 0) {
+			clear_bit(GLF_LOCK, &gl->gl_flags);
+			smp_mb__after_atomic();
+			return;
+		}
 		if (ret == 2)
-			goto out;
+			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 */
 	}
 	do_xmote(gl, gh, gl->gl_target);
-out:
-	return;
-
-out_sched:
-	clear_bit(GLF_LOCK, &gl->gl_flags);
-	smp_mb__after_atomic();
-	gl->gl_lockref.count++;
-	__gfs2_glock_queue_work(gl, 0);
-	return;
-
-out_unlock:
-	clear_bit(GLF_LOCK, &gl->gl_flags);
-	smp_mb__after_atomic();
 	return;
 }
 
-- 
2.19.1

[Cluster-devel] [PATCH 02/13] 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 692784faa464..5f2156f15f05 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 */
 			fs_err(gl->gl_name.ln_sbd, "wanted %u got %u\n",
@@ -528,17 +528,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;
 
@@ -659,7 +659,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.19.1

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

[Steven Whitehouse]

On 19/11/18 13:29, Bob Peterson wrote:
> 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 692784faa464..5f2156f15f05 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 */
>   			fs_err(gl->gl_name.ln_sbd, "wanted %u got %u\n",
> @@ -528,17 +528,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;
>   
> @@ -659,7 +659,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;
>   }
>   
Since gh is apparently only used to get the lock flags, it would make 
more sense just to pass the lock flags rather than add in an additional 
find_first_waiter() call,

Steve.

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

[Bob Peterson]

Hi Steve,

----- Original Message -----
> 
> 
> On 19/11/18 13:29, Bob Peterson wrote:
> > 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>

(snip)

> Since gh is apparently only used to get the lock flags, it would make
> more sense just to pass the lock flags rather than add in an additional
> find_first_waiter() call,
> 
> Steve.

Perhaps I didn't put enough info into the comments for this patch.

I need to get rid of the gh parameter in order to make the glock
state machine fully autonomous. In other words, function do_xmote will
become a state in the (stand alone) state machine, which itself does not
require a gh parameter and may be called from several places under
several conditions. The state of the glock will determine that it needs
to call do_xmote, but do_xmote needs to figure it out on its own.

Before this patch, the caller does indeed know the gh pointer, but in
the future, it will replaced by a generic call to the state machine
which will not know it.

Regards,

Bob Peterson

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

[Steven Whitehouse]

Hi,


On 19/11/18 21:06, Bob Peterson wrote:
> Hi Steve,
>
> ----- Original Message -----
>>
>> On 19/11/18 13:29, Bob Peterson wrote:
>>> 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>
> (snip)
>
>> Since gh is apparently only used to get the lock flags, it would make
>> more sense just to pass the lock flags rather than add in an additional
>> find_first_waiter() call,
>>
>> Steve.
> Perhaps I didn't put enough info into the comments for this patch.
>
> I need to get rid of the gh parameter in order to make the glock
> state machine fully autonomous. In other words, function do_xmote will
> become a state in the (stand alone) state machine, which itself does not
> require a gh parameter and may be called from several places under
> several conditions. The state of the glock will determine that it needs
> to call do_xmote, but do_xmote needs to figure it out on its own.
A function can't become a state in this sense. The state in this case is 
the content of struct gfs2_glock, and the
functions define how you get from one state to another,

>
> Before this patch, the caller does indeed know the gh pointer, but in
> the future, it will replaced by a generic call to the state machine
> which will not know it.
>
> Regards,
>
> Bob Peterson

That is not relevant to the point I was making though. The point is that 
if the flags are passed to do_xmote rather than the gh, then that 
resolves the issue of needing to pass the gh and reduces the amount of 
code, since you can pass 0 flags instead of NULL gh,

Steve.

[Cluster-devel] [PATCH 03/13] 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 5f2156f15f05..6e9d53583b73 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.19.1

[Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote

[Steven Whitehouse]

On 19/11/18 13:29, Bob Peterson wrote:
> 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 5f2156f15f05..6e9d53583b73 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;
I'm not sure what you are trying to achieve here, but setting the state 
to LM_ST_UNLOCKED when it is quite possible that it is not that state, 
doesn't really seem to improve anything. Indeed, it looks more confusing 
to me, at least it was fairly clear before that the intent was to retry 
the operation which has been canceled.

> +			} 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 */

[Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote

[Bob Peterson]

Hi,

----- Original Message -----
> 
> 
> On 19/11/18 13:29, Bob Peterson wrote:
> > 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 5f2156f15f05..6e9d53583b73 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;
> I'm not sure what you are trying to achieve here, but setting the state
> to LM_ST_UNLOCKED when it is quite possible that it is not that state,
> doesn't really seem to improve anything. Indeed, it looks more confusing
> to me, at least it was fairly clear before that the intent was to retry
> the operation which has been canceled.

When finish_xmote hits this affected section of code, it's because the dlm
returned a state different from the intended state. Before this patch, it
did "goto retry" which jumps to the label inside the switch state that
handles LM_ST_UNLOCKED, after which it simply unlocks and returns.

Changing local variable "state" merely forces the code to take the same
codepath in which it calls do_xmote, unlocking and returning as it does today,
but without the goto. This makes the function more suitable to the new
autonomous state machine, which is added in a later patch.

The addition of "else if" is needed so it doesn't go down the wrong code path
at the comment: /* Some error or failed "try lock" - report it */
The logic is a bit tricky here, but is preserved from the original.

Most of the subsequent patches aren't quite as mind-bending, I promise. :)

Regards,

Bob Peterson

[Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote

[Steven Whitehouse]

Hi,


On 19/11/18 21:26, Bob Peterson wrote:
> Hi,
>
> ----- Original Message -----
>>
>> On 19/11/18 13:29, Bob Peterson wrote:
>>> 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 5f2156f15f05..6e9d53583b73 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;
>> I'm not sure what you are trying to achieve here, but setting the state
>> to LM_ST_UNLOCKED when it is quite possible that it is not that state,
>> doesn't really seem to improve anything. Indeed, it looks more confusing
>> to me, at least it was fairly clear before that the intent was to retry
>> the operation which has been canceled.
> When finish_xmote hits this affected section of code, it's because the dlm
> returned a state different from the intended state. Before this patch, it
> did "goto retry" which jumps to the label inside the switch state that
> handles LM_ST_UNLOCKED, after which it simply unlocks and returns.
>
> Changing local variable "state" merely forces the code to take the same
> codepath in which it calls do_xmote, unlocking and returning as it does today,
> but without the goto. This makes the function more suitable to the new
> autonomous state machine, which is added in a later patch.
>
> The addition of "else if" is needed so it doesn't go down the wrong code path
> at the comment: /* Some error or failed "try lock" - report it */
> The logic is a bit tricky here, but is preserved from the original.
>
> Most of the subsequent patches aren't quite as mind-bending, I promise. :)
>
> Regards,
>
> Bob Peterson
I can see that it is doing the same thing as before, but it is less 
clear why. The point about the retry label is that it is telling us what 
is going to do. Setting the state to LM_ST_UNLOCKED is more confusing, 
because the state might not be LM_ST_UNLOCKED at this point, and you are 
now forcing that state in order to get the same code path as before. 
There is no real advantage compared with the previous code that I can 
see, except that it is more confusing,

Steve.

[Cluster-devel] [PATCH 04/13] 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 6e9d53583b73..5e0eac782c32 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;
 			}
 		}
@@ -497,7 +496,6 @@ static void finish_xmote(struct gfs2_glock *gl, unsigned int ret)
 			       gl->gl_target, state);
 			GLOCK_BUG_ON(gl, 1);
 		}
-		spin_unlock(&gl->gl_lockref.lock);
 		return;
 	}
 
@@ -516,12 +514,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);
 }
 
 /**
@@ -573,7 +569,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) {
@@ -582,7 +579,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);
 	}
 
@@ -662,6 +660,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);
@@ -691,7 +736,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);
@@ -818,6 +863,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 888b62cfd6d1..1ab2ef56123a 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -388,6 +388,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.19.1

[Cluster-devel] [PATCH 05/13] 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 5e0eac782c32..b541c4053dd7 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,19 +487,17 @@ 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 */
 			fs_err(gl->gl_name.ln_sbd, "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 */
@@ -514,10 +515,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;
 }
 
 /**
@@ -656,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, gl->gl_target);
+	__state_machine(gl, GL_ST_DO_XMOTE);
 	return;
 }
 
@@ -674,6 +676,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 {
@@ -685,6 +689,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.19.1

[Cluster-devel] [PATCH 06/13] 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 b541c4053dd7..8dc98d069afa 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -527,9 +527,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)
 {
@@ -537,11 +539,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);
@@ -572,21 +574,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) {
 			fs_err(sdp, "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;
 }
 
 /**
@@ -698,13 +702,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.19.1

[Cluster-devel] [PATCH 07/13] 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 8dc98d069afa..023e2186b374 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -631,21 +631,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);
@@ -713,6 +701,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.19.1

[Cluster-devel] [PATCH 08/13] 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 023e2186b374..6048867b2d66 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -621,37 +621,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);
 }
 
 /**
@@ -668,6 +648,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));
@@ -727,6 +708,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.19.1

[Cluster-devel] [PATCH 09/13] 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 6048867b2d66..82c4614793b3 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;
@@ -610,34 +607,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
@@ -646,7 +620,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;
@@ -727,6 +702,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);
 }
@@ -738,12 +729,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);
 }
 
@@ -776,7 +768,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);
@@ -794,7 +786,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--;
@@ -1189,7 +1181,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))
@@ -1733,7 +1725,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.19.1

[Cluster-devel] [PATCH 10/13] 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 82c4614793b3..858f42e66698 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -673,15 +673,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.19.1

[Cluster-devel] [PATCH 11/13] 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 858f42e66698..22ddeda90199 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -624,9 +624,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) {
@@ -716,8 +732,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;
 }
 
 /**
@@ -734,6 +783,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);
 }
 
@@ -761,49 +814,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.19.1

[Cluster-devel] [PATCH 12/13] 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 22ddeda90199..33f4a530caf4 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -750,6 +750,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);
 
@@ -774,7 +779,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)
@@ -1734,10 +1738,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.19.1

[Cluster-devel] [PATCH 13/13] 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 33f4a530caf4..29e6d9a440ec 100644
--- a/fs/gfs2/glock.c
+++ b/fs/gfs2/glock.c
@@ -724,7 +724,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 :
@@ -751,10 +753,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.19.1