Ocfs2 devel - May 2009 - [PATCH] ocfs2_cluster_lock: code cleanup for redundant assignment

In fs/ocfs2/dlmglue.c:ocfs2_cluster_lock(), after label 'out:' the code
is:

1373         if (wait && arg_flags & OCFS2_LOCK_NONBLOCK &&
1374             mw.mw_mask & (OCFS2_LOCK_BUSY|OCFS2_LOCK_BLOCKED)) {
1375                 wait = 0;
1376                 if (lockres_remove_mask_waiter(lockres, &mw))
1377                         ret = -EAGAIN;
1378                 else
1379                         goto again;
1380         }

On L1375 variable 'wait' is assigned to 0. But if execution path goes to
L1379
and jumps to label 'again:' on L1262, there is already an assignment to
'wait'
on L1263:

1262 again:
1263         wait = 0;
1264

The previous 'wait = 0' on L1375 is redundant in this case. This patch
removes
such a redundant variable assignment.

Signed-off-by: Coly Li <coly.li at suse.de>
---
 fs/ocfs2/dlmglue.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
index e15fc7d..b6060e3 100644
--- a/fs/ocfs2/dlmglue.c
+++ b/fs/ocfs2/dlmglue.c
@@ -1372,10 +1372,10 @@ out:
 	 */
 	if (wait && arg_flags & OCFS2_LOCK_NONBLOCK &&
 	    mw.mw_mask & (OCFS2_LOCK_BUSY|OCFS2_LOCK_BLOCKED)) {
-		wait = 0;
-		if (lockres_remove_mask_waiter(lockres, &mw))
+		if (lockres_remove_mask_waiter(lockres, &mw)) {
+			wait = 0;
 			ret = -EAGAIN;
-		else
+		} else
 			goto again;
 	}
 	if (wait) {
-- 
Coly Li
SuSE Labs

While this patch may be correct, we have to take into consideration
that we have to backport patches to 1.4, etc. In that light, is this
patch worth it?

Coly Li wrote:
> In fs/ocfs2/dlmglue.c:ocfs2_cluster_lock(), after label 'out:' the
code is:
>
> 1373         if (wait && arg_flags & OCFS2_LOCK_NONBLOCK
&&
> 1374             mw.mw_mask & (OCFS2_LOCK_BUSY|OCFS2_LOCK_BLOCKED)) {
> 1375                 wait = 0;
> 1376                 if (lockres_remove_mask_waiter(lockres, &mw))
> 1377                         ret = -EAGAIN;
> 1378                 else
> 1379                         goto again;
> 1380         }
>
> On L1375 variable 'wait' is assigned to 0. But if execution path
goes to L1379
> and jumps to label 'again:' on L1262, there is already an
assignment to 'wait'
> on L1263:
>
> 1262 again:
> 1263         wait = 0;
> 1264
>
> The previous 'wait = 0' on L1375 is redundant in this case. This
patch removes
> such a redundant variable assignment.
>
> Signed-off-by: Coly Li <coly.li at suse.de>
> ---
>  fs/ocfs2/dlmglue.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/fs/ocfs2/dlmglue.c b/fs/ocfs2/dlmglue.c
> index e15fc7d..b6060e3 100644
> --- a/fs/ocfs2/dlmglue.c
> +++ b/fs/ocfs2/dlmglue.c
> @@ -1372,10 +1372,10 @@ out:
>  	 */
>  	if (wait && arg_flags & OCFS2_LOCK_NONBLOCK &&
>  	    mw.mw_mask & (OCFS2_LOCK_BUSY|OCFS2_LOCK_BLOCKED)) {
> -		wait = 0;
> -		if (lockres_remove_mask_waiter(lockres, &mw))
> +		if (lockres_remove_mask_waiter(lockres, &mw)) {
> +			wait = 0;
>  			ret = -EAGAIN;
> -		else
> +		} else
>  			goto again;
>  	}
>  	if (wait) {
>

On Fri, May 08, 2009 at 02:30:52AM +0800, Coly Li wrote:
> In fs/ocfs2/dlmglue.c:ocfs2_cluster_lock(), after label 'out:' the
code is:
> 
> 1373         if (wait && arg_flags & OCFS2_LOCK_NONBLOCK
&&
> 1374             mw.mw_mask & (OCFS2_LOCK_BUSY|OCFS2_LOCK_BLOCKED)) {
> 1375                 wait = 0;
> 1376                 if (lockres_remove_mask_waiter(lockres, &mw))
> 1377                         ret = -EAGAIN;
> 1378                 else
> 1379                         goto again;
> 1380         }
> 
> On L1375 variable 'wait' is assigned to 0. But if execution path
goes to L1379
> and jumps to label 'again:' on L1262, there is already an
assignment to 'wait'
> on L1263:
> 
> 1262 again:
> 1263         wait = 0;
> 1264
> 
> The previous 'wait = 0' on L1375 is redundant in this case. This
patch removes
> such a redundant variable assignment.
	NAK.  You're correct that the assignment is redundant, but the
compiler is smart enough to figure it out.  The current code is more
readable - it's obvious right there that NONBLOCk &&
BLOCKED|BUSY means we shouldn't wait.  With your change, it's not
obvious - someone has to go down through the code to discover that wait
is cleared at again:.  Even if they find that wait is cleared, they
don't have an obvious clue as to the reason.

Joel

-- 

"I always thought the hardest questions were those I could not answer.
 Now I know they are the ones I can never ask."
			- Charlie Watkins

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

Sunil Mushran Wrote:
> While this patch may be correct, we have to take into consideration
> that we have to backport patches to 1.4, etc. In that light, is this
> patch worth it?
> 
When I check dlm related code in ocfs2, this line makes me uncomfortable, after
sending out the patch, I feel better :))

I understand the comments from you and Jeol, and partially agree with you all :)
Thanks for the response.

-- 
Coly Li
SuSE Labs