[PATCHv2 7/10] xfs: tidy up some goto labels

[PATCHv2 7/10] xfs: tidy up some goto labels

[Alex Elder]

Rename a label used in xlog_find_head() that I thought was poorly
chosen.  Also combine two adjacent labels xlog_find_tail() into
a single label, and give it a more generic name.

Signed-off-by: Alex Elder <aelder@sgi.com>

---
fs/xfs/xfs_log_recover.c |   83 ++++++++++++++++++++++++-----------------------
 fs/xfs/xfs_log_recover.c |   20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -739,7 +739,7 @@ xlog_find_head(
 			goto bp_err;
 		if (new_blk != -1) {
 			head_blk = new_blk;
-			goto bad_blk;
+			goto fine_tune;
 		}
 
 		/*
@@ -757,7 +757,7 @@ xlog_find_head(
 			head_blk = new_blk;
 	}
 
- bad_blk:
+fine_tune:
 	/*
 	 * Now we need to make sure head_blk is not pointing to a block in
 	 * the middle of a log record.
@@ -864,12 +864,12 @@ xlog_find_tail(
 	if (*head_blk == 0) {				/* special case */
 		error = xlog_bread(log, 0, 1, bp, &offset);
 		if (error)
-			goto bread_err;
+			goto done;
 
 		if (xlog_get_cycle(offset) == 0) {
 			*tail_blk = 0;
 			/* leave all other log inited values alone */
-			goto exit;
+			goto done;
 		}
 	}
 
@@ -880,7 +880,7 @@ xlog_find_tail(
 	for (i = (int)(*head_blk) - 1; i >= 0; i--) {
 		error = xlog_bread(log, i, 1, bp, &offset);
 		if (error)
-			goto bread_err;
+			goto done;
 
 		if (XLOG_HEADER_MAGIC_NUM == be32_to_cpu(*(__be32 *)offset)) {
 			found = 1;
@@ -897,7 +897,7 @@ xlog_find_tail(
 		for (i = log->l_logBBsize - 1; i >= (int)(*head_blk); i--) {
 			error = xlog_bread(log, i, 1, bp, &offset);
 			if (error)
-				goto bread_err;
+				goto done;
 
 			if (XLOG_HEADER_MAGIC_NUM ==
 			    be32_to_cpu(*(__be32 *)offset)) {
@@ -972,7 +972,7 @@ xlog_find_tail(
 		umount_data_blk = (i + hblks) % log->l_logBBsize;
 		error = xlog_bread(log, umount_data_blk, 1, bp, &offset);
 		if (error)
-			goto bread_err;
+			goto done;
 
 		op_head = (xlog_op_header_t *)offset;
 		if (op_head->oh_flags & XLOG_UNMOUNT_TRANS) {
@@ -1018,12 +1018,10 @@ xlog_find_tail(
 	 * But... if the -device- itself is readonly, just skip this.
 	 * We can't recover this device anyway, so it won't matter.
 	 */
-	if (!xfs_readonly_buftarg(log->l_mp->m_logdev_targp)) {
+	if (!xfs_readonly_buftarg(log->l_mp->m_logdev_targp))
 		error = xlog_clear_stale_blocks(log, tail_lsn);
-	}
 
-bread_err:
-exit:
+done:
 	xlog_put_bp(bp);
 
 	if (error)


_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCHv2 7/10] xfs: tidy up some goto labels

[Dave Chinner]

On Fri, Apr 09, 2010 at 05:29:23PM -0500, Alex Elder wrote:
> Rename a label used in xlog_find_head() that I thought was poorly
> chosen.  Also combine two adjacent labels xlog_find_tail() into
> a single label, and give it a more generic name.
> 
> Signed-off-by: Alex Elder <aelder@sgi.com>
> 
> ---
> fs/xfs/xfs_log_recover.c |   83 ++++++++++++++++++++++++-----------------------
>  fs/xfs/xfs_log_recover.c |   20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> Index: b/fs/xfs/xfs_log_recover.c
> ===================================================================
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -739,7 +739,7 @@ xlog_find_head(
>  			goto bp_err;
>  		if (new_blk != -1) {
>  			head_blk = new_blk;
> -			goto bad_blk;
> +			goto fine_tune;
>  		}
>  
>  		/*
> @@ -757,7 +757,7 @@ xlog_find_head(
>  			head_blk = new_blk;
>  	}
>  
> - bad_blk:
> +fine_tune:
>  	/*
>  	 * Now we need to make sure head_blk is not pointing to a block in
>  	 * the middle of a log record.

I don't think "fine_tune" really matches what is being done here
either. "bad_blk" makes sense when you consider that the search is
being terminated due to a log block being found that didn't match
the search criteria. i.e. it is bad.

What we are really doing there at "bad_blk" is validating the head
block we have found, so if you are going to change the label then
"validate_head" makes more sense to me...

> @@ -864,12 +864,12 @@ xlog_find_tail(
>  	if (*head_blk == 0) {				/* special case */
>  		error = xlog_bread(log, 0, 1, bp, &offset);
>  		if (error)
> -			goto bread_err;
> +			goto done;
>  
>  		if (xlog_get_cycle(offset) == 0) {
>  			*tail_blk = 0;
>  			/* leave all other log inited values alone */
> -			goto exit;
> +			goto done;
>  		}
>  	}

These changes look fine, though.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCHv2 7/10] xfs: tidy up some goto labels

[Alex Elder]

On Mon, 2010-04-12 at 16:52 +1000, Dave Chinner wrote:
> On Fri, Apr 09, 2010 at 05:29:23PM -0500, Alex Elder wrote:
> > Rename a label used in xlog_find_head() that I thought was poorly
> > chosen.  Also combine two adjacent labels xlog_find_tail() into
> > a single label, and give it a more generic name.
> > 
> > Signed-off-by: Alex Elder <aelder@sgi.com>
> > 
> > ---
> > fs/xfs/xfs_log_recover.c |   83 ++++++++++++++++++++++++-----------------------
> >  fs/xfs/xfs_log_recover.c |   20 +++++++++-----------
> >  1 file changed, 9 insertions(+), 11 deletions(-)
> > 
> > Index: b/fs/xfs/xfs_log_recover.c
> > ===================================================================
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c

. . .

> > @@ -757,7 +757,7 @@ xlog_find_head(
> >  			head_blk = new_blk;
> >  	}
> >  
> > - bad_blk:
> > +fine_tune:
> >  	/*
> >  	 * Now we need to make sure head_blk is not pointing to a block in
> >  	 * the middle of a log record.
> 
> I don't think "fine_tune" really matches what is being done here
> either. "bad_blk" makes sense when you consider that the search is
> being terminated due to a log block being found that didn't match
> the search criteria. i.e. it is bad.
> 
> What we are really doing there at "bad_blk" is validating the head
> block we have found, so if you are going to change the label then
> "validate_head" makes more sense to me...

My label came from the idea that at this point
we're refining the estimate of the head of the
log.  But I like "validate_head" just as well.
(In my brain "bad_blk" suggests a media problem;
I care more about changing it than about what
it is changed to...)

If I switch it to use your proposed label, can
I get a "Reviewed-by"?  (I won't bother re-posting
the patch.)

					-Alex

PS  I'm still working on reorganizing this file
    (including this function) some more, to kill
    off a bunch of duplicated code.



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs