[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 at 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.