xfs: a few more minor xfs_log_recover.c cleanups
PATCH v2 9/10 xfs: a few more minor xfs_log_recover.c cleanups
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.
---fs/xfs/xfs_log_recover.c | 83 ++++++++++++++++++++++++-----------------------
fs/xfs/xfs_log_recover.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-)
fs/xfs/xfs_log_recover.c +15 -10
@@ -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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 4 lines · Alex Elder
Minor things that didn't seem to warrant their own individual patches:
- Inxlog_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)....
Quoted 1 line
- 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...
Quoted 3 lines
- 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Quoted 9 lines · Dave Chinner
Quoted 4 lines · Alex Elder
Minor things that didn't seem to warrant their own individual patches:
- Inxlog_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.
Quoted 5 lines
Quoted 1 line
- 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.
*/
Quoted 6 lines
Quoted 3 lines
- 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Alex Elder
Quoted 6 lines · Dave Chinner
Quoted 1 line · Alex Elder
- 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:
Quoted 8 lines
/*
* Check that the range of blocks does not containstop_on_cycle_no.
* Fill in*new_blkwith 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.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs