All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs: deduplicate xlog_do_recovery_pass()
@ 2014-08-21  3:18 Eric Sandeen
  2014-08-21  4:36 ` Eric Sandeen
  2014-08-21 12:36 ` Brian Foster
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Sandeen @ 2014-08-21  3:18 UTC (permalink / raw)
  To: xfs-oss

In xlog_do_recovery_pass(), there are 2 distinct cases:
non-wrapped and wrapped log recovery.

If we find a wrapped log, we recover around the end
of the log, and then handle the rest of recovery
exactly as in the non-wrapped case - using exactly the same
(duplicated) code.

Rather than having the same code in both cases, we can
get the wrapped portion out of the way first if needed,
and then recover the non-wrapped portion of the log.

There should be no functional change here, just code
reorganization & deduplication.

The patch looks a bit bigger than it really is; the last
hunk is whitespace changes (un-indenting).

Tested with xfstests "check -g log" on a stock configuration.

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

 xfs_log_recover.c |   81 ++++++++++++++++++------------------------------------
 1 file changed, 27 insertions(+), 54 deletions(-)

diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 1fd5787..08d3569 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -4132,41 +4132,13 @@ xlog_do_recovery_pass(
 	}
 
 	memset(rhash, 0, sizeof(rhash));
-	if (tail_blk <= head_blk) {
-		for (blk_no = tail_blk; blk_no < head_blk; ) {
-			error = xlog_bread(log, blk_no, hblks, hbp, &offset);
-			if (error)
-				goto bread_err2;
-
-			rhead = (xlog_rec_header_t *)offset;
-			error = xlog_valid_rec_header(log, rhead, blk_no);
-			if (error)
-				goto bread_err2;
-
-			/* blocks in data section */
-			bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
-			error = xlog_bread(log, blk_no + hblks, bblks, dbp,
-					   &offset);
-			if (error)
-				goto bread_err2;
-
-			error = xlog_unpack_data(rhead, offset, log);
-			if (error)
-				goto bread_err2;
-
-			error = xlog_recover_process_data(log,
-						rhash, rhead, offset, pass);
-			if (error)
-				goto bread_err2;
-			blk_no += bblks + hblks;
-		}
-	} else {
+	blk_no = tail_blk;
+	if (tail_blk > head_blk) {
 		/*
 		 * Perform recovery around the end of the physical log.
 		 * When the head is not on the same cycle number as the tail,
-		 * we can't do a sequential recovery as above.
+		 * we can't do a sequential recovery.
 		 */
-		blk_no = tail_blk;
 		while (blk_no < log->l_logBBsize) {
 			/*
 			 * Check for header wrapping around physical end-of-log
@@ -4280,34 +4252,35 @@ xlog_do_recovery_pass(
 
 		ASSERT(blk_no >= log->l_logBBsize);
 		blk_no -= log->l_logBBsize;
+	}
 
-		/* read first part of physical log */
-		while (blk_no < head_blk) {
-			error = xlog_bread(log, blk_no, hblks, hbp, &offset);
-			if (error)
-				goto bread_err2;
+	/* read first part of physical log */
+	while (blk_no < head_blk) {
+		error = xlog_bread(log, blk_no, hblks, hbp, &offset);
+		if (error)
+			goto bread_err2;
 
-			rhead = (xlog_rec_header_t *)offset;
-			error = xlog_valid_rec_header(log, rhead, blk_no);
-			if (error)
-				goto bread_err2;
+		rhead = (xlog_rec_header_t *)offset;
+		error = xlog_valid_rec_header(log, rhead, blk_no);
+		if (error)
+			goto bread_err2;
 
-			bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
-			error = xlog_bread(log, blk_no+hblks, bblks, dbp,
-					   &offset);
-			if (error)
-				goto bread_err2;
+		/* blocks in data section */
+		bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
+		error = xlog_bread(log, blk_no+hblks, bblks, dbp,
+				   &offset);
+		if (error)
+			goto bread_err2;
 
-			error = xlog_unpack_data(rhead, offset, log);
-			if (error)
-				goto bread_err2;
+		error = xlog_unpack_data(rhead, offset, log);
+		if (error)
+			goto bread_err2;
 
-			error = xlog_recover_process_data(log, rhash,
-							rhead, offset, pass);
-			if (error)
-				goto bread_err2;
-			blk_no += bblks + hblks;
-		}
+		error = xlog_recover_process_data(log, rhash,
+						rhead, offset, pass);
+		if (error)
+			goto bread_err2;
+		blk_no += bblks + hblks;
 	}
 
  bread_err2:

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: deduplicate xlog_do_recovery_pass()
  2014-08-21  3:18 [PATCH] xfs: deduplicate xlog_do_recovery_pass() Eric Sandeen
@ 2014-08-21  4:36 ` Eric Sandeen
  2014-08-21  4:49   ` Dave Chinner
  2014-08-21 12:36 ` Brian Foster
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2014-08-21  4:36 UTC (permalink / raw)
  To: Eric Sandeen, xfs-oss

On 8/20/14, 10:18 PM, Eric Sandeen wrote:
> In xlog_do_recovery_pass(), there are 2 distinct cases:
> non-wrapped and wrapped log recovery.
> 
> If we find a wrapped log, we recover around the end
> of the log, and then handle the rest of recovery
> exactly as in the non-wrapped case - using exactly the same
> (duplicated) code.
> 
> Rather than having the same code in both cases, we can
> get the wrapped portion out of the way first if needed,
> and then recover the non-wrapped portion of the log.
> 
> There should be no functional change here, just code
> reorganization & deduplication.
> 
> The patch looks a bit bigger than it really is; the last
> hunk is whitespace changes (un-indenting).
> 
> Tested with xfstests "check -g log" on a stock configuration.

which didn't actually hit any log wraps.  Does xfstests
really not cover wrapped log recovery?  anyway, something like this
on a small log:

[root@bp-05 xfstests]# while true; do mount /dev/sdc5 /mnt/scratch; ltp/fsstress -n 500 -d /mnt/scratch; src/godown -f /mnt/scratch; umount /mnt/scratch; done

hit plenty of log wraps w/ no problem.

-Eric

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: deduplicate xlog_do_recovery_pass()
  2014-08-21  4:36 ` Eric Sandeen
@ 2014-08-21  4:49   ` Dave Chinner
  2014-08-21  4:58     ` Eric Sandeen
  0 siblings, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2014-08-21  4:49 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Wed, Aug 20, 2014 at 11:36:40PM -0500, Eric Sandeen wrote:
> On 8/20/14, 10:18 PM, Eric Sandeen wrote:
> > In xlog_do_recovery_pass(), there are 2 distinct cases:
> > non-wrapped and wrapped log recovery.
> > 
> > If we find a wrapped log, we recover around the end
> > of the log, and then handle the rest of recovery
> > exactly as in the non-wrapped case - using exactly the same
> > (duplicated) code.
> > 
> > Rather than having the same code in both cases, we can
> > get the wrapped portion out of the way first if needed,
> > and then recover the non-wrapped portion of the log.
> > 
> > There should be no functional change here, just code
> > reorganization & deduplication.
> > 
> > The patch looks a bit bigger than it really is; the last
> > hunk is whitespace changes (un-indenting).
> > 
> > Tested with xfstests "check -g log" on a stock configuration.
> 
> which didn't actually hit any log wraps.  Does xfstests
> really not cover wrapped log recovery?  anyway, something like this
> on a small log:

xfs/016

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: deduplicate xlog_do_recovery_pass()
  2014-08-21  4:49   ` Dave Chinner
@ 2014-08-21  4:58     ` Eric Sandeen
  2014-08-21  5:12       ` Dave Chinner
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2014-08-21  4:58 UTC (permalink / raw)
  To: Dave Chinner; +Cc: Eric Sandeen, xfs-oss

On 8/20/14, 11:49 PM, Dave Chinner wrote:
> On Wed, Aug 20, 2014 at 11:36:40PM -0500, Eric Sandeen wrote:
>> On 8/20/14, 10:18 PM, Eric Sandeen wrote:
>>> In xlog_do_recovery_pass(), there are 2 distinct cases:
>>> non-wrapped and wrapped log recovery.
>>>
>>> If we find a wrapped log, we recover around the end
>>> of the log, and then handle the rest of recovery
>>> exactly as in the non-wrapped case - using exactly the same
>>> (duplicated) code.
>>>
>>> Rather than having the same code in both cases, we can
>>> get the wrapped portion out of the way first if needed,
>>> and then recover the non-wrapped portion of the log.
>>>
>>> There should be no functional change here, just code
>>> reorganization & deduplication.
>>>
>>> The patch looks a bit bigger than it really is; the last
>>> hunk is whitespace changes (un-indenting).
>>>
>>> Tested with xfstests "check -g log" on a stock configuration.
>>
>> which didn't actually hit any log wraps.  Does xfstests
>> really not cover wrapped log recovery?  anyway, something like this
>> on a small log:
> 
> xfs/016

AFAICT that doesn't ever run recovery...

-Eric

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: deduplicate xlog_do_recovery_pass()
  2014-08-21  4:58     ` Eric Sandeen
@ 2014-08-21  5:12       ` Dave Chinner
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2014-08-21  5:12 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, xfs-oss

On Wed, Aug 20, 2014 at 11:58:55PM -0500, Eric Sandeen wrote:
> On 8/20/14, 11:49 PM, Dave Chinner wrote:
> > On Wed, Aug 20, 2014 at 11:36:40PM -0500, Eric Sandeen wrote:
> >> On 8/20/14, 10:18 PM, Eric Sandeen wrote:
> >>> In xlog_do_recovery_pass(), there are 2 distinct cases:
> >>> non-wrapped and wrapped log recovery.
> >>>
> >>> If we find a wrapped log, we recover around the end
> >>> of the log, and then handle the rest of recovery
> >>> exactly as in the non-wrapped case - using exactly the same
> >>> (duplicated) code.
> >>>
> >>> Rather than having the same code in both cases, we can
> >>> get the wrapped portion out of the way first if needed,
> >>> and then recover the non-wrapped portion of the log.
> >>>
> >>> There should be no functional change here, just code
> >>> reorganization & deduplication.
> >>>
> >>> The patch looks a bit bigger than it really is; the last
> >>> hunk is whitespace changes (un-indenting).
> >>>
> >>> Tested with xfstests "check -g log" on a stock configuration.
> >>
> >> which didn't actually hit any log wraps.  Does xfstests
> >> really not cover wrapped log recovery?  anyway, something like this
> >> on a small log:
> > 
> > xfs/016
> 
> AFAICT that doesn't ever run recovery...

It does:

loop 20 times {

	mount
	<do stuff>
	unmount

	dump and process log
}

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs: deduplicate xlog_do_recovery_pass()
  2014-08-21  3:18 [PATCH] xfs: deduplicate xlog_do_recovery_pass() Eric Sandeen
  2014-08-21  4:36 ` Eric Sandeen
@ 2014-08-21 12:36 ` Brian Foster
  1 sibling, 0 replies; 6+ messages in thread
From: Brian Foster @ 2014-08-21 12:36 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs-oss

On Wed, Aug 20, 2014 at 10:18:52PM -0500, Eric Sandeen wrote:
> In xlog_do_recovery_pass(), there are 2 distinct cases:
> non-wrapped and wrapped log recovery.
> 
> If we find a wrapped log, we recover around the end
> of the log, and then handle the rest of recovery
> exactly as in the non-wrapped case - using exactly the same
> (duplicated) code.
> 
> Rather than having the same code in both cases, we can
> get the wrapped portion out of the way first if needed,
> and then recover the non-wrapped portion of the log.
> 
> There should be no functional change here, just code
> reorganization & deduplication.
> 
> The patch looks a bit bigger than it really is; the last
> hunk is whitespace changes (un-indenting).
> 
> Tested with xfstests "check -g log" on a stock configuration.
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks good to me:

Reviewed-by: Brian Foster <bfoster@redhat.com>

> 
>  xfs_log_recover.c |   81 ++++++++++++++++++------------------------------------
>  1 file changed, 27 insertions(+), 54 deletions(-)
> 
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 1fd5787..08d3569 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -4132,41 +4132,13 @@ xlog_do_recovery_pass(
>  	}
>  
>  	memset(rhash, 0, sizeof(rhash));
> -	if (tail_blk <= head_blk) {
> -		for (blk_no = tail_blk; blk_no < head_blk; ) {
> -			error = xlog_bread(log, blk_no, hblks, hbp, &offset);
> -			if (error)
> -				goto bread_err2;
> -
> -			rhead = (xlog_rec_header_t *)offset;
> -			error = xlog_valid_rec_header(log, rhead, blk_no);
> -			if (error)
> -				goto bread_err2;
> -
> -			/* blocks in data section */
> -			bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
> -			error = xlog_bread(log, blk_no + hblks, bblks, dbp,
> -					   &offset);
> -			if (error)
> -				goto bread_err2;
> -
> -			error = xlog_unpack_data(rhead, offset, log);
> -			if (error)
> -				goto bread_err2;
> -
> -			error = xlog_recover_process_data(log,
> -						rhash, rhead, offset, pass);
> -			if (error)
> -				goto bread_err2;
> -			blk_no += bblks + hblks;
> -		}
> -	} else {
> +	blk_no = tail_blk;
> +	if (tail_blk > head_blk) {
>  		/*
>  		 * Perform recovery around the end of the physical log.
>  		 * When the head is not on the same cycle number as the tail,
> -		 * we can't do a sequential recovery as above.
> +		 * we can't do a sequential recovery.
>  		 */
> -		blk_no = tail_blk;
>  		while (blk_no < log->l_logBBsize) {
>  			/*
>  			 * Check for header wrapping around physical end-of-log
> @@ -4280,34 +4252,35 @@ xlog_do_recovery_pass(
>  
>  		ASSERT(blk_no >= log->l_logBBsize);
>  		blk_no -= log->l_logBBsize;
> +	}
>  
> -		/* read first part of physical log */
> -		while (blk_no < head_blk) {
> -			error = xlog_bread(log, blk_no, hblks, hbp, &offset);
> -			if (error)
> -				goto bread_err2;
> +	/* read first part of physical log */
> +	while (blk_no < head_blk) {
> +		error = xlog_bread(log, blk_no, hblks, hbp, &offset);
> +		if (error)
> +			goto bread_err2;
>  
> -			rhead = (xlog_rec_header_t *)offset;
> -			error = xlog_valid_rec_header(log, rhead, blk_no);
> -			if (error)
> -				goto bread_err2;
> +		rhead = (xlog_rec_header_t *)offset;
> +		error = xlog_valid_rec_header(log, rhead, blk_no);
> +		if (error)
> +			goto bread_err2;
>  
> -			bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
> -			error = xlog_bread(log, blk_no+hblks, bblks, dbp,
> -					   &offset);
> -			if (error)
> -				goto bread_err2;
> +		/* blocks in data section */
> +		bblks = (int)BTOBB(be32_to_cpu(rhead->h_len));
> +		error = xlog_bread(log, blk_no+hblks, bblks, dbp,
> +				   &offset);
> +		if (error)
> +			goto bread_err2;
>  
> -			error = xlog_unpack_data(rhead, offset, log);
> -			if (error)
> -				goto bread_err2;
> +		error = xlog_unpack_data(rhead, offset, log);
> +		if (error)
> +			goto bread_err2;
>  
> -			error = xlog_recover_process_data(log, rhash,
> -							rhead, offset, pass);
> -			if (error)
> -				goto bread_err2;
> -			blk_no += bblks + hblks;
> -		}
> +		error = xlog_recover_process_data(log, rhash,
> +						rhead, offset, pass);
> +		if (error)
> +			goto bread_err2;
> +		blk_no += bblks + hblks;
>  	}
>  
>   bread_err2:
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-08-21 12:36 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-21  3:18 [PATCH] xfs: deduplicate xlog_do_recovery_pass() Eric Sandeen
2014-08-21  4:36 ` Eric Sandeen
2014-08-21  4:49   ` Dave Chinner
2014-08-21  4:58     ` Eric Sandeen
2014-08-21  5:12       ` Dave Chinner
2014-08-21 12:36 ` Brian Foster

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.