Re: [PATCH 06/16] xfs_repair: fix rmapbt record order check

[Christoph Hellwig <hch@infradead.org>]

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>