[PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

[PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

[Alex Elder]

Minor things that didn't seem to warrant their own individual
patches:
- In xlog_bread_noalign(), reorder assertions so the buffer pointer
  is known to be non-null before attempting to dereference it.
- Add a more descriptive header comment for xlog_find_verify_cycle().
- Make a few additions to the comments in xlog_find_head().  Also
  rearrange some expressions in a few spots to produce the same
  result, but in a way that seems more clear what's being computed.

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

---
fs/xfs/xfs_log_recover.c |   83 ++++++++++++++++++++++++-----------------------
 fs/xfs/xfs_log_recover.c |   25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

Index: b/fs/xfs/xfs_log_recover.c
===================================================================
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -166,8 +166,8 @@ xlog_bread_noalign(
 	nbblks = round_up(nbblks, xlog_sectbb(log));
 
 	ASSERT(nbblks > 0);
-	ASSERT(BBTOB(nbblks) <= XFS_BUF_SIZE(bp));
 	ASSERT(bp);
+	ASSERT(BBTOB(nbblks) <= XFS_BUF_SIZE(bp));
 
 	XFS_BUF_SET_ADDR(bp, log->l_logBBstart + blk_no);
 	XFS_BUF_READ(bp);
@@ -387,8 +387,11 @@ xlog_find_cycle_start(
  * test.  If the region is completely good, we end up returning the same
  * last block number.
  *
- * Set blkno to -1 if we encounter no errors.  This is an invalid block number
- * since we don't ever expect logs to get this large.
+ * If we encounter a basic block containing stop_on_cycle_no as its
+ * cycle number, return that basic block offset in *new_blk.  If no
+ * basic blocks in the range specified contain stop_on_cycle_no, set
+ * *new_blk to -1.  This is an invalid block number since we don't
+ * ever expect logs to get this large.
  */
 STATIC int
 xlog_find_verify_cycle(
@@ -662,7 +665,7 @@ xlog_find_head(
 		 * In this case we want to find the first block with cycle
 		 * number matching last_half_cycle.  We expect the log to be
 		 * some variation on
-		 *        x + 1 ... | x ...
+		 *        x + 1 ... | x ... | x
 		 * The first block with cycle number x (last_half_cycle) will
 		 * be where the new head belongs.  First we do a binary search
 		 * for the first occurrence of last_half_cycle.  The binary
@@ -672,11 +675,13 @@ xlog_find_head(
 		 * the log, then we look for occurrences of last_half_cycle - 1
 		 * at the end of the log.  The cases we're looking for look
 		 * like
-		 *        x + 1 ... | x | x + 1 | x ...
-		 *                               ^ binary search stopped here
+		 *                               v binary search stopped here
+		 *        x + 1 ... | x | x + 1 | x ... | x
+		 *                   ^ but we want to locate this spot
 		 * or
-		 *        x + 1 ... | x ... | x - 1 | x
 		 *        <---------> less than scan distance
+		 *        x + 1 ... | x ... | x - 1 | x
+		 *                           ^ we want to locate this spot
 		 */
 		stop_on_cycle = last_half_cycle;
 		if ((error = xlog_find_cycle_start(log, bp, first_blk,
@@ -732,9 +737,9 @@ xlog_find_head(
 		 * certainly not the head of the log.  By searching for
 		 * last_half_cycle-1 we accomplish that.
 		 */
-		start_blk = log_bbnum - num_scan_bblks + head_blk;
 		ASSERT(head_blk <= INT_MAX &&
-			(xfs_daddr_t) num_scan_bblks - head_blk >= 0);
+			(xfs_daddr_t) num_scan_bblks >= head_blk);
+		start_blk = log_bbnum - (num_scan_bblks - head_blk);
 		if ((error = xlog_find_verify_cycle(log, start_blk,
 					num_scan_bblks - (int)head_blk,
 					(stop_on_cycle - 1), &new_blk)))
@@ -781,7 +786,7 @@ fine_tune:
 		if ((error = xlog_find_verify_log_record(log, start_blk,
 							&head_blk, 0)) == -1) {
 			/* We hit the beginning of the log during our search */
-			start_blk = log_bbnum - num_scan_bblks + head_blk;
+			start_blk = log_bbnum - (num_scan_bblks - head_blk);
 			new_blk = log_bbnum;
 			ASSERT(start_blk <= INT_MAX &&
 				(xfs_daddr_t) log_bbnum-start_blk >= 0);


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

Re: [PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

[Dave Chinner]

On Fri, Apr 09, 2010 at 05:31:06PM -0500, Alex Elder wrote:
> Minor things that didn't seem to warrant their own individual
> patches:
> - In xlog_bread_noalign(), reorder assertions so the buffer pointer
>   is known to be non-null before attempting to dereference it.

That's OK, but if you are debugging then it'll be obvious what
happend by it being a null ptr dereference rather than an assert
failure. Hence if anything I'd just kill the ASSERT(bp)....

> - Add a more descriptive header comment for xlog_find_verify_cycle().

It just describes what the code does - I don't think it make the
code any clearer and makes it more likely that if we ever change the
code the comment will then be wrong...

> - Make a few additions to the comments in xlog_find_head().  Also
>   rearrange some expressions in a few spots to produce the same
>   result, but in a way that seems more clear what's being computed.

I'd say this is probably the only bits of the patch that add value.
Can you split this one out by itself?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

[Alex Elder]

On Mon, 2010-04-12 at 17:11 +1000, Dave Chinner wrote:
> On Fri, Apr 09, 2010 at 05:31:06PM -0500, Alex Elder wrote:
> > Minor things that didn't seem to warrant their own individual
> > patches:
> > - In xlog_bread_noalign(), reorder assertions so the buffer pointer
> >   is known to be non-null before attempting to dereference it.
> 
> That's OK, but if you are debugging then it'll be obvious what
> happend by it being a null ptr dereference rather than an assert
> failure. Hence if anything I'd just kill the ASSERT(bp)....

I personally prefer proactively asserting it rather than
just deducing it was a null pointer from the wreckage.  But
whatever, we disagree and I'm OK with dropping the assert.

> > - Add a more descriptive header comment for xlog_find_verify_cycle().
> 
> It just describes what the code does - I don't think it make the
> code any clearer and makes it more likely that if we ever change the
> code the comment will then be wrong...

I still think the existing comments are confusing.  What do you
think of this instead:

/*
 * Check that the range of blocks does not contain stop_on_cycle_no.
 * Fill in *new_blk with the block offset where such block is found,
 * or with -1 (an invalid block number) if there is no such block in
 * the range.  The scan needs to occur from front to back and the
 * pointer into the region must be updated since a later routine will
 * need to perform another test.  
 */

> > - Make a few additions to the comments in xlog_find_head().  Also
> >   rearrange some expressions in a few spots to produce the same
> >   result, but in a way that seems more clear what's being computed.
> 
> I'd say this is probably the only bits of the patch that add value.
> Can you split this one out by itself?

I will re-post a v3 on this and the others I've responded to
today, just so we're all clear on what's being accepted.  I
will break this patch into two pieces, allowing for this last
bit to be considered separate from the rest.

					-Alex

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

Re: [PATCHv2 9/10] xfs: a few more minor xfs_log_recover.c cleanups

[Dave Chinner]

On Wed, Apr 14, 2010 at 04:19:12PM -0500, Alex Elder wrote:
> On Mon, 2010-04-12 at 17:11 +1000, Dave Chinner wrote:
> > On Fri, Apr 09, 2010 at 05:31:06PM -0500, Alex Elder wrote:
> > > - Add a more descriptive header comment for xlog_find_verify_cycle().
> > 
> > It just describes what the code does - I don't think it make the
> > code any clearer and makes it more likely that if we ever change the
> > code the comment will then be wrong...
> 
> I still think the existing comments are confusing.  What do you
> think of this instead:
> 
> /*
>  * Check that the range of blocks does not contain stop_on_cycle_no.
>  * Fill in *new_blk with the block offset where such block is found,
>  * or with -1 (an invalid block number) if there is no such block in
>  * the range.  The scan needs to occur from front to back and the
>  * pointer into the region must be updated since a later routine will
>  * need to perform another test.  
>  */

Yup, that's much better ;)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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