[06/16] xfs_repair: fix rmapbt record order check
diff mbox series

Message ID 158904183079.982941.15948246247495283555.stgit@magnolia
State Superseded
Headers show
Series
  • xfs_repair: catch things that xfs_check misses
Related show

Commit Message

Darrick J. Wong May 9, 2020, 4:30 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

The rmapbt record order checks here don't quite work properly.  For
non-shared filesystems, we fail to check that the startblock of the nth
record comes entirely after the previous record.

However, for filesystems with shared blocks (reflink) we correctly check
that the startblock/owner/offset of the nth record comes after the
previous one.

Therefore, make the reflink fs checks use "laststartblock" to preserve
that functionality while making the non-reflink fs checks use
"lastblock" to fix the problem outlined above.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/scan.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Christoph Hellwig May 10, 2020, 7:33 a.m. UTC | #1
On Sat, May 09, 2020 at 09:30:30AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The rmapbt record order checks here don't quite work properly.  For
> non-shared filesystems, we fail to check that the startblock of the nth
> record comes entirely after the previous record.
> 
> However, for filesystems with shared blocks (reflink) we correctly check
> that the startblock/owner/offset of the nth record comes after the
> previous one.
> 
> Therefore, make the reflink fs checks use "laststartblock" to preserve
> that functionality while making the non-reflink fs checks use
> "lastblock" to fix the problem outlined above.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/scan.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/repair/scan.c b/repair/scan.c
> index 7c46ab89..7508f7e8 100644
> --- a/repair/scan.c
> +++ b/repair/scan.c
> @@ -925,15 +925,15 @@ struct rmap_priv {
>  static bool
>  rmap_in_order(
>  	xfs_agblock_t	b,
> -	xfs_agblock_t	lastblock,
> +	xfs_agblock_t	laststartblock,
>  	uint64_t	owner,
>  	uint64_t	lastowner,
>  	uint64_t	offset,
>  	uint64_t	lastoffset)
>  {
> -	if (b > lastblock)
> +	if (b > laststartblock)
>  		return true;
> -	else if (b < lastblock)
> +	else if (b < laststartblock)
>  		return false;
>  
>  	if (owner > lastowner)

So this is just a variable rename and looks obviously fine.

> @@ -964,6 +964,7 @@ scan_rmapbt(
>  	int			hdr_errors = 0;
>  	int			numrecs;
>  	int			state;
> +	xfs_agblock_t		laststartblock = 0;
>  	xfs_agblock_t		lastblock = 0;
>  	uint64_t		lastowner = 0;
>  	uint64_t		lastoffset = 0;
> @@ -1101,14 +1102,15 @@ _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
>  			/* Check for out of order records. */
>  			if (i == 0) {
>  advance:
> -				lastblock = b;
> +				laststartblock = b;
> +				lastblock = end - 1;
>  				lastowner = owner;
>  				lastoffset = offset;
>  			} else {
>  				bool bad;
>  
>  				if (xfs_sb_version_hasreflink(&mp->m_sb))
> -					bad = !rmap_in_order(b, lastblock,
> +					bad = !rmap_in_order(b, laststartblock,
>  							owner, lastowner,
>  							offset, lastoffset);
>  				else

This looks correct, but really hard to read. I'll send a follow on
cleanup.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Patch
diff mbox series

diff --git a/repair/scan.c b/repair/scan.c
index 7c46ab89..7508f7e8 100644
--- a/repair/scan.c
+++ b/repair/scan.c
@@ -925,15 +925,15 @@  struct rmap_priv {
 static bool
 rmap_in_order(
 	xfs_agblock_t	b,
-	xfs_agblock_t	lastblock,
+	xfs_agblock_t	laststartblock,
 	uint64_t	owner,
 	uint64_t	lastowner,
 	uint64_t	offset,
 	uint64_t	lastoffset)
 {
-	if (b > lastblock)
+	if (b > laststartblock)
 		return true;
-	else if (b < lastblock)
+	else if (b < laststartblock)
 		return false;
 
 	if (owner > lastowner)
@@ -964,6 +964,7 @@  scan_rmapbt(
 	int			hdr_errors = 0;
 	int			numrecs;
 	int			state;
+	xfs_agblock_t		laststartblock = 0;
 	xfs_agblock_t		lastblock = 0;
 	uint64_t		lastowner = 0;
 	uint64_t		lastoffset = 0;
@@ -1101,14 +1102,15 @@  _("%s rmap btree block claimed (state %d), agno %d, bno %d, suspect %d\n"),
 			/* Check for out of order records. */
 			if (i == 0) {
 advance:
-				lastblock = b;
+				laststartblock = b;
+				lastblock = end - 1;
 				lastowner = owner;
 				lastoffset = offset;
 			} else {
 				bool bad;
 
 				if (xfs_sb_version_hasreflink(&mp->m_sb))
-					bad = !rmap_in_order(b, lastblock,
+					bad = !rmap_in_order(b, laststartblock,
 							owner, lastowner,
 							offset, lastoffset);
 				else