Re: [Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote
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
---
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.
Re: [Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote
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
> > ---
> > 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
Re: [Cluster-devel] [PATCH 03/13] GFS2: Eliminate a goto in finish_xmote
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
---
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
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
---
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
