[PATCH 0/4] xfs: fixes for 5.4

[PATCH 0/4] xfs: fixes for 5.4

[Darrick J. Wong]

Hi all,

Various fixes for 5.4.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

kernel git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfs-linux.git/log/?h=xfs-5.4-fixes

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=xfs-5.4-fixes

[PATCH 1/4] xfs: bmap scrub should only scrub records once

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The inode block mapping scrub function does more work for btree format
extent maps than is absolutely necessary -- first it will walk the bmbt
and check all the entries, and then it will load the incore tree and
check every entry in that tree, possibly for a second time.

Simplify the code and decrease check runtime by separating the two
responsibilities.  The bmbt walk will make sure the incore extent
mappings are loaded, check the shape of the bmap btree (via xchk_btree)
and check that every bmbt record has a corresponding incore extent map;
and the incore extent map walk takes all the responsibility for checking
the mapping records and cross referencing them with other AG metadata.

This enables us to clean up some messy parameter handling and reduce
redundant code.  Rename a few functions to make the split of
responsibilities clearer.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
 1 file changed, 45 insertions(+), 31 deletions(-)


diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
index 1bd29fdc2ab5..f6ed6eb133a6 100644
--- a/fs/xfs/scrub/bmap.c
+++ b/fs/xfs/scrub/bmap.c
@@ -75,6 +75,7 @@ struct xchk_bmap_info {
 	xfs_fileoff_t		lastoff;
 	bool			is_rt;
 	bool			is_shared;
+	bool			was_loaded;
 	int			whichfork;
 };
 
@@ -213,25 +214,20 @@ xchk_bmap_xref_rmap(
 
 /* Cross-reference a single rtdev extent record. */
 STATIC void
-xchk_bmap_rt_extent_xref(
-	struct xchk_bmap_info	*info,
+xchk_bmap_rt_iextent_xref(
 	struct xfs_inode	*ip,
-	struct xfs_btree_cur	*cur,
+	struct xchk_bmap_info	*info,
 	struct xfs_bmbt_irec	*irec)
 {
-	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
-		return;
-
 	xchk_xref_is_used_rt_space(info->sc, irec->br_startblock,
 			irec->br_blockcount);
 }
 
 /* Cross-reference a single datadev extent record. */
 STATIC void
-xchk_bmap_extent_xref(
-	struct xchk_bmap_info	*info,
+xchk_bmap_iextent_xref(
 	struct xfs_inode	*ip,
-	struct xfs_btree_cur	*cur,
+	struct xchk_bmap_info	*info,
 	struct xfs_bmbt_irec	*irec)
 {
 	struct xfs_mount	*mp = info->sc->mp;
@@ -240,9 +236,6 @@ xchk_bmap_extent_xref(
 	xfs_extlen_t		len;
 	int			error;
 
-	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
-		return;
-
 	agno = XFS_FSB_TO_AGNO(mp, irec->br_startblock);
 	agbno = XFS_FSB_TO_AGBNO(mp, irec->br_startblock);
 	len = irec->br_blockcount;
@@ -300,20 +293,15 @@ xchk_bmap_dirattr_extent(
 
 /* Scrub a single extent record. */
 STATIC int
-xchk_bmap_extent(
+xchk_bmap_iextent(
 	struct xfs_inode	*ip,
-	struct xfs_btree_cur	*cur,
 	struct xchk_bmap_info	*info,
 	struct xfs_bmbt_irec	*irec)
 {
 	struct xfs_mount	*mp = info->sc->mp;
-	struct xfs_buf		*bp = NULL;
 	xfs_filblks_t		end;
 	int			error = 0;
 
-	if (cur)
-		xfs_btree_get_block(cur, 0, &bp);
-
 	/*
 	 * Check for out-of-order extents.  This record could have come
 	 * from the incore list, for which there is no ordering check.
@@ -364,10 +352,13 @@ xchk_bmap_extent(
 		xchk_fblock_set_corrupt(info->sc, info->whichfork,
 				irec->br_startoff);
 
+	if (info->sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
+		return 0;
+
 	if (info->is_rt)
-		xchk_bmap_rt_extent_xref(info, ip, cur, irec);
+		xchk_bmap_rt_iextent_xref(ip, info, irec);
 	else
-		xchk_bmap_extent_xref(info, ip, cur, irec);
+		xchk_bmap_iextent_xref(ip, info, irec);
 
 	info->lastoff = irec->br_startoff + irec->br_blockcount;
 	return error;
@@ -380,10 +371,13 @@ xchk_bmapbt_rec(
 	union xfs_btree_rec	*rec)
 {
 	struct xfs_bmbt_irec	irec;
+	struct xfs_bmbt_irec	iext_irec;
+	struct xfs_iext_cursor	icur;
 	struct xchk_bmap_info	*info = bs->private;
 	struct xfs_inode	*ip = bs->cur->bc_private.b.ip;
 	struct xfs_buf		*bp = NULL;
 	struct xfs_btree_block	*block;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(ip, info->whichfork);
 	uint64_t		owner;
 	int			i;
 
@@ -402,9 +396,25 @@ xchk_bmapbt_rec(
 		}
 	}
 
-	/* Set up the in-core record and scrub it. */
+	/*
+	 * Check that the incore extent tree contains an extent that matches
+	 * this one exactly.  We validate those cached bmaps later, so we don't
+	 * need to check them here.  If the extent tree was freshly loaded by
+	 * the scrubber then we skip the check entirely.
+	 */
+	if (info->was_loaded)
+		return 0;
+
 	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
-	return xchk_bmap_extent(ip, bs->cur, info, &irec);
+	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
+				&iext_irec) ||
+	    irec.br_startoff != iext_irec.br_startoff ||
+	    irec.br_startblock != iext_irec.br_startblock ||
+	    irec.br_blockcount != iext_irec.br_blockcount ||
+	    irec.br_state != iext_irec.br_state)
+		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
+				irec.br_startoff);
+	return 0;
 }
 
 /* Scan the btree records. */
@@ -415,15 +425,26 @@ xchk_bmap_btree(
 	struct xchk_bmap_info	*info)
 {
 	struct xfs_owner_info	oinfo;
+	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
 	struct xfs_mount	*mp = sc->mp;
 	struct xfs_inode	*ip = sc->ip;
 	struct xfs_btree_cur	*cur;
 	int			error;
 
+	/* Load the incore bmap cache if it's not loaded. */
+	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
+	if (!info->was_loaded) {
+		error = xfs_iread_extents(sc->tp, ip, whichfork);
+		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
+			goto out;
+	}
+
+	/* Check the btree structure. */
 	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
 	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
 	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
 	xfs_btree_del_cursor(cur, error);
+out:
 	return error;
 }
 
@@ -671,13 +692,6 @@ xchk_bmap(
 	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
 		goto out;
 
-	/* Now try to scrub the in-memory extent list. */
-        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
-		error = xfs_iread_extents(sc->tp, ip, whichfork);
-		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
-			goto out;
-	}
-
 	/* Find the offset of the last extent in the mapping. */
 	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
 	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
@@ -689,7 +703,7 @@ xchk_bmap(
 	for_each_xfs_iext(ifp, &icur, &irec) {
 		if (xchk_should_terminate(sc, &error) ||
 		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
-			break;
+			goto out;
 		if (isnullstartblock(irec.br_startblock))
 			continue;
 		if (irec.br_startoff >= endoff) {
@@ -697,7 +711,7 @@ xchk_bmap(
 					irec.br_startoff);
 			goto out;
 		}
-		error = xchk_bmap_extent(ip, NULL, &info, &irec);
+		error = xchk_bmap_iextent(ip, &info, &irec);
 		if (error)
 			goto out;
 	}

[PATCH 2/4] xfs: fix maxicount division by zero error

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In xfs_ialloc_setup_geometry, it's possible for a malicious/corrupt fs
image to set an unreasonably large value for sb_inopblog which will
cause ialloc_blks to be zero.  If sb_imax_pct is also set, this results
in a division by zero error in the second do_div call.  Therefore, force
maxicount to zero if ialloc_blks is zero.

Note that the kernel metadata verifiers will catch the garbage inopblog
value and abort the fs mount long before it tries to set up the inode
geometry; this is needed to avoid a crash in xfs_db while setting up the
xfs_mount structure.

Found by fuzzing sb_inopblog to 122 in xfs/350.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_ialloc.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 04377ab75863..aa190a502326 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -2788,7 +2788,7 @@ xfs_ialloc_setup_geometry(
 			inodes);
 
 	/* Set the maximum inode count for this filesystem. */
-	if (sbp->sb_imax_pct) {
+	if (sbp->sb_imax_pct && igeo->ialloc_blks) {
 		/*
 		 * Make sure the maximum inode count is a multiple
 		 * of the units we allocate inodes in.

[PATCH 3/4] xfs: don't return _QUERY_ABORT from xfs_rmap_has_other_keys

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The xfs_rmap_has_other_keys helper aborts the iteration as soon as it
has an answer.  Don't let this abort leak out to callers.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_rmap.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)


diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
index e6aeb390b2fb..819693ef852c 100644
--- a/fs/xfs/libxfs/xfs_rmap.c
+++ b/fs/xfs/libxfs/xfs_rmap.c
@@ -2540,8 +2540,11 @@ xfs_rmap_has_other_keys(
 
 	error = xfs_rmap_query_range(cur, &low, &high,
 			xfs_rmap_has_other_keys_helper, &rks);
+	if (error < 0)
+		return error;
+
 	*has_rmap = rks.has_rmap;
-	return error;
+	return 0;
 }
 
 const struct xfs_owner_info XFS_RMAP_OINFO_SKIP_UPDATE = {

[PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
two unsigned 64-bit quantities.  If the second quantity is actually the
"maximum" key (all ones) as used in _query_all, the subtraction
effectively becomes addition of two positive numbers and the function
returns incorrect results.  Fix this with explicit comparisons of the
unsigned values.  Nobody needs this now, but the online repair patches
will need this to work properly.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)


diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
index fbb18ba5d905..3c1a805b3775 100644
--- a/fs/xfs/libxfs/xfs_bmap_btree.c
+++ b/fs/xfs/libxfs/xfs_bmap_btree.c
@@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
 	union xfs_btree_key	*k1,
 	union xfs_btree_key	*k2)
 {
-	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
-			  be64_to_cpu(k2->bmbt.br_startoff);
+	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
+	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
+
+	/*
+	 * Note: This routine previously casted a and b to int64 and subtracted
+	 * them to generate a result.  This lead to problems if b was the
+	 * "maximum" key value (all ones) being signed incorrectly, hence this
+	 * somewhat less efficient version.
+	 */
+	if (a > b)
+		return 1;
+	else if (b > a)
+		return -1;
+	return 0;
 }
 
 static xfs_failaddr_t

Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

[Dave Chinner]

On Mon, Aug 26, 2019 at 02:48:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The inode block mapping scrub function does more work for btree format
> extent maps than is absolutely necessary -- first it will walk the bmbt
> and check all the entries, and then it will load the incore tree and
> check every entry in that tree, possibly for a second time.
> 
> Simplify the code and decrease check runtime by separating the two
> responsibilities.  The bmbt walk will make sure the incore extent
> mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> and check that every bmbt record has a corresponding incore extent map;
> and the incore extent map walk takes all the responsibility for checking
> the mapping records and cross referencing them with other AG metadata.
> 
> This enables us to clean up some messy parameter handling and reduce
> redundant code.  Rename a few functions to make the split of
> responsibilities clearer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks fine.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 2/4] xfs: fix maxicount division by zero error

[Dave Chinner]

On Mon, Aug 26, 2019 at 02:48:56PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_ialloc_setup_geometry, it's possible for a malicious/corrupt fs
> image to set an unreasonably large value for sb_inopblog which will
> cause ialloc_blks to be zero.  If sb_imax_pct is also set, this results
> in a division by zero error in the second do_div call.  Therefore, force
> maxicount to zero if ialloc_blks is zero.
> 
> Note that the kernel metadata verifiers will catch the garbage inopblog
> value and abort the fs mount long before it tries to set up the inode
> geometry; this is needed to avoid a crash in xfs_db while setting up the
> xfs_mount structure.
> 
> Found by fuzzing sb_inopblog to 122 in xfs/350.

Harmless for the kernel, makes sense for shared code.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/4] xfs: don't return _QUERY_ABORT from xfs_rmap_has_other_keys

[Dave Chinner]

On Mon, Aug 26, 2019 at 02:49:02PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The xfs_rmap_has_other_keys helper aborts the iteration as soon as it
> has an answer.  Don't let this abort leak out to callers.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_rmap.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_rmap.c b/fs/xfs/libxfs/xfs_rmap.c
> index e6aeb390b2fb..819693ef852c 100644
> --- a/fs/xfs/libxfs/xfs_rmap.c
> +++ b/fs/xfs/libxfs/xfs_rmap.c
> @@ -2540,8 +2540,11 @@ xfs_rmap_has_other_keys(
>  
>  	error = xfs_rmap_query_range(cur, &low, &high,
>  			xfs_rmap_has_other_keys_helper, &rks);
> +	if (error < 0)
> +		return error;
> +
>  	*has_rmap = rks.has_rmap;
> -	return error;
> +	return 0;
>  }

Yup, makes sense - XFS_BTREE_QUERY_RANGE_ABORT has a positive value,
so this makes success have a return value of 0 and that fixes
the spurious error detection in xrep_reap_block().

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

[Dave Chinner]

On Mon, Aug 26, 2019 at 02:49:09PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
> two unsigned 64-bit quantities.  If the second quantity is actually the
> "maximum" key (all ones) as used in _query_all, the subtraction
> effectively becomes addition of two positive numbers and the function
> returns incorrect results.  Fix this with explicit comparisons of the
> unsigned values.  Nobody needs this now, but the online repair patches
> will need this to work properly.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> 
> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> index fbb18ba5d905..3c1a805b3775 100644
> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> @@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
>  	union xfs_btree_key	*k1,
>  	union xfs_btree_key	*k2)
>  {
> -	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
> -			  be64_to_cpu(k2->bmbt.br_startoff);
> +	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
> +	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
> +
> +	/*
> +	 * Note: This routine previously casted a and b to int64 and subtracted
> +	 * them to generate a result.  This lead to problems if b was the
> +	 * "maximum" key value (all ones) being signed incorrectly, hence this
> +	 * somewhat less efficient version.
> +	 */
> +	if (a > b)
> +		return 1;
> +	else if (b > a)
> +		return -1;

No need for an else here, but otherwise OK.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

[Eric Sandeen]

On 8/26/19 6:15 PM, Dave Chinner wrote:
> On Mon, Aug 26, 2019 at 02:49:09PM -0700, Darrick J. Wong wrote:
>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
>> two unsigned 64-bit quantities.  If the second quantity is actually the
>> "maximum" key (all ones) as used in _query_all, the subtraction
>> effectively becomes addition of two positive numbers and the function
>> returns incorrect results.  Fix this with explicit comparisons of the
>> unsigned values.  Nobody needs this now, but the online repair patches
>> will need this to work properly.
>>
>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>> ---
>>  fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>
>>
>> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
>> index fbb18ba5d905..3c1a805b3775 100644
>> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
>> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
>> @@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
>>  	union xfs_btree_key	*k1,
>>  	union xfs_btree_key	*k2)
>>  {
>> -	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
>> -			  be64_to_cpu(k2->bmbt.br_startoff);
>> +	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
>> +	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
>> +
>> +	/*
>> +	 * Note: This routine previously casted a and b to int64 and subtracted
>> +	 * them to generate a result.  This lead to problems if b was the
>> +	 * "maximum" key value (all ones) being signed incorrectly, hence this
>> +	 * somewhat less efficient version.
>> +	 */
>> +	if (a > b)
>> +		return 1;
>> +	else if (b > a)
>> +		return -1;
> 
> No need for an else here, but otherwise OK.
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

In fact having the else means the a == b case isn't handled, even if it
should never happen, so might a static checker eventually complain about
reaching the end of a non-void function?

-Eric

Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

[Brian Foster]

On Mon, Aug 26, 2019 at 02:48:49PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The inode block mapping scrub function does more work for btree format
> extent maps than is absolutely necessary -- first it will walk the bmbt
> and check all the entries, and then it will load the incore tree and
> check every entry in that tree, possibly for a second time.
> 
> Simplify the code and decrease check runtime by separating the two
> responsibilities.  The bmbt walk will make sure the incore extent
> mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> and check that every bmbt record has a corresponding incore extent map;
> and the incore extent map walk takes all the responsibility for checking
> the mapping records and cross referencing them with other AG metadata.
> 
> This enables us to clean up some messy parameter handling and reduce
> redundant code.  Rename a few functions to make the split of
> responsibilities clearer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
>  1 file changed, 45 insertions(+), 31 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> index 1bd29fdc2ab5..f6ed6eb133a6 100644
> --- a/fs/xfs/scrub/bmap.c
> +++ b/fs/xfs/scrub/bmap.c
...
> @@ -402,9 +396,25 @@ xchk_bmapbt_rec(
>  		}
>  	}
>  
> -	/* Set up the in-core record and scrub it. */
> +	/*
> +	 * Check that the incore extent tree contains an extent that matches
> +	 * this one exactly.  We validate those cached bmaps later, so we don't
> +	 * need to check them here.  If the extent tree was freshly loaded by
> +	 * the scrubber then we skip the check entirely.
> +	 */
> +	if (info->was_loaded)
> +		return 0;
> +

This all looks fine to me except that I don't follow the reasoning for
skipping the lookup from the comment. Are we just saying that if we
loaded the extent tree, then we can assume it reflects what is on disk?
If so, can we fix up the last sentence in the comment to explain?

Brian

>  	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
> -	return xchk_bmap_extent(ip, bs->cur, info, &irec);
> +	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
> +				&iext_irec) ||
> +	    irec.br_startoff != iext_irec.br_startoff ||
> +	    irec.br_startblock != iext_irec.br_startblock ||
> +	    irec.br_blockcount != iext_irec.br_blockcount ||
> +	    irec.br_state != iext_irec.br_state)
> +		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
> +				irec.br_startoff);
> +	return 0;
>  }
>  
>  /* Scan the btree records. */
> @@ -415,15 +425,26 @@ xchk_bmap_btree(
>  	struct xchk_bmap_info	*info)
>  {
>  	struct xfs_owner_info	oinfo;
> +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
>  	struct xfs_mount	*mp = sc->mp;
>  	struct xfs_inode	*ip = sc->ip;
>  	struct xfs_btree_cur	*cur;
>  	int			error;
>  
> +	/* Load the incore bmap cache if it's not loaded. */
> +	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
> +	if (!info->was_loaded) {
> +		error = xfs_iread_extents(sc->tp, ip, whichfork);
> +		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> +			goto out;
> +	}
> +
> +	/* Check the btree structure. */
>  	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
>  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
>  	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
>  	xfs_btree_del_cursor(cur, error);
> +out:
>  	return error;
>  }
>  
> @@ -671,13 +692,6 @@ xchk_bmap(
>  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
>  		goto out;
>  
> -	/* Now try to scrub the in-memory extent list. */
> -        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> -		error = xfs_iread_extents(sc->tp, ip, whichfork);
> -		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> -			goto out;
> -	}
> -
>  	/* Find the offset of the last extent in the mapping. */
>  	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
>  	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> @@ -689,7 +703,7 @@ xchk_bmap(
>  	for_each_xfs_iext(ifp, &icur, &irec) {
>  		if (xchk_should_terminate(sc, &error) ||
>  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> -			break;
> +			goto out;
>  		if (isnullstartblock(irec.br_startblock))
>  			continue;
>  		if (irec.br_startoff >= endoff) {
> @@ -697,7 +711,7 @@ xchk_bmap(
>  					irec.br_startoff);
>  			goto out;
>  		}
> -		error = xchk_bmap_extent(ip, NULL, &info, &irec);
> +		error = xchk_bmap_iextent(ip, &info, &irec);
>  		if (error)
>  			goto out;
>  	}
> 

Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

[Darrick J. Wong]

On Tue, Aug 27, 2019 at 09:14:03AM -0400, Brian Foster wrote:
> On Mon, Aug 26, 2019 at 02:48:49PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The inode block mapping scrub function does more work for btree format
> > extent maps than is absolutely necessary -- first it will walk the bmbt
> > and check all the entries, and then it will load the incore tree and
> > check every entry in that tree, possibly for a second time.
> > 
> > Simplify the code and decrease check runtime by separating the two
> > responsibilities.  The bmbt walk will make sure the incore extent
> > mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> > and check that every bmbt record has a corresponding incore extent map;
> > and the incore extent map walk takes all the responsibility for checking
> > the mapping records and cross referencing them with other AG metadata.
> > 
> > This enables us to clean up some messy parameter handling and reduce
> > redundant code.  Rename a few functions to make the split of
> > responsibilities clearer.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
> >  1 file changed, 45 insertions(+), 31 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > index 1bd29fdc2ab5..f6ed6eb133a6 100644
> > --- a/fs/xfs/scrub/bmap.c
> > +++ b/fs/xfs/scrub/bmap.c
> ...
> > @@ -402,9 +396,25 @@ xchk_bmapbt_rec(
> >  		}
> >  	}
> >  
> > -	/* Set up the in-core record and scrub it. */
> > +	/*
> > +	 * Check that the incore extent tree contains an extent that matches
> > +	 * this one exactly.  We validate those cached bmaps later, so we don't
> > +	 * need to check them here.  If the extent tree was freshly loaded by
> > +	 * the scrubber then we skip the check entirely.
> > +	 */
> > +	if (info->was_loaded)
> > +		return 0;
> > +
> 
> This all looks fine to me except that I don't follow the reasoning for
> skipping the lookup from the comment. Are we just saying that if we
> loaded the extent tree, then we can assume it reflects what is on disk?
> If so, can we fix up the last sentence in the comment to explain?

Yes.

"If the incore extent tree was just loaded from disk by the scrubber,
we assume that its contents match what's on disk and skip the
equivalence check." ?

--D

> Brian
> 
> >  	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
> > -	return xchk_bmap_extent(ip, bs->cur, info, &irec);
> > +	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
> > +				&iext_irec) ||
> > +	    irec.br_startoff != iext_irec.br_startoff ||
> > +	    irec.br_startblock != iext_irec.br_startblock ||
> > +	    irec.br_blockcount != iext_irec.br_blockcount ||
> > +	    irec.br_state != iext_irec.br_state)
> > +		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
> > +				irec.br_startoff);
> > +	return 0;
> >  }
> >  
> >  /* Scan the btree records. */
> > @@ -415,15 +425,26 @@ xchk_bmap_btree(
> >  	struct xchk_bmap_info	*info)
> >  {
> >  	struct xfs_owner_info	oinfo;
> > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
> >  	struct xfs_mount	*mp = sc->mp;
> >  	struct xfs_inode	*ip = sc->ip;
> >  	struct xfs_btree_cur	*cur;
> >  	int			error;
> >  
> > +	/* Load the incore bmap cache if it's not loaded. */
> > +	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
> > +	if (!info->was_loaded) {
> > +		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > +		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > +			goto out;
> > +	}
> > +
> > +	/* Check the btree structure. */
> >  	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
> >  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
> >  	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
> >  	xfs_btree_del_cursor(cur, error);
> > +out:
> >  	return error;
> >  }
> >  
> > @@ -671,13 +692,6 @@ xchk_bmap(
> >  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> >  		goto out;
> >  
> > -	/* Now try to scrub the in-memory extent list. */
> > -        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > -		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > -		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > -			goto out;
> > -	}
> > -
> >  	/* Find the offset of the last extent in the mapping. */
> >  	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
> >  	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > @@ -689,7 +703,7 @@ xchk_bmap(
> >  	for_each_xfs_iext(ifp, &icur, &irec) {
> >  		if (xchk_should_terminate(sc, &error) ||
> >  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> > -			break;
> > +			goto out;
> >  		if (isnullstartblock(irec.br_startblock))
> >  			continue;
> >  		if (irec.br_startoff >= endoff) {
> > @@ -697,7 +711,7 @@ xchk_bmap(
> >  					irec.br_startoff);
> >  			goto out;
> >  		}
> > -		error = xchk_bmap_extent(ip, NULL, &info, &irec);
> > +		error = xchk_bmap_iextent(ip, &info, &irec);
> >  		if (error)
> >  			goto out;
> >  	}
> > 

Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

[Darrick J. Wong]

On Tue, Aug 27, 2019 at 08:01:16AM -0500, Eric Sandeen wrote:
> On 8/26/19 6:15 PM, Dave Chinner wrote:
> > On Mon, Aug 26, 2019 at 02:49:09PM -0700, Darrick J. Wong wrote:
> >> From: Darrick J. Wong <darrick.wong@oracle.com>
> >>
> >> In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
> >> two unsigned 64-bit quantities.  If the second quantity is actually the
> >> "maximum" key (all ones) as used in _query_all, the subtraction
> >> effectively becomes addition of two positive numbers and the function
> >> returns incorrect results.  Fix this with explicit comparisons of the
> >> unsigned values.  Nobody needs this now, but the online repair patches
> >> will need this to work properly.
> >>
> >> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> >> ---
> >>  fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
> >>  1 file changed, 14 insertions(+), 2 deletions(-)
> >>
> >>
> >> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
> >> index fbb18ba5d905..3c1a805b3775 100644
> >> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
> >> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
> >> @@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
> >>  	union xfs_btree_key	*k1,
> >>  	union xfs_btree_key	*k2)
> >>  {
> >> -	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
> >> -			  be64_to_cpu(k2->bmbt.br_startoff);
> >> +	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
> >> +	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
> >> +
> >> +	/*
> >> +	 * Note: This routine previously casted a and b to int64 and subtracted
> >> +	 * them to generate a result.  This lead to problems if b was the
> >> +	 * "maximum" key value (all ones) being signed incorrectly, hence this
> >> +	 * somewhat less efficient version.
> >> +	 */
> >> +	if (a > b)
> >> +		return 1;
> >> +	else if (b > a)
> >> +		return -1;
> > 
> > No need for an else here, but otherwise OK.
> > 
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> In fact having the else means the a == b case isn't handled, even if it
> should never happen, so might a static checker eventually complain about
> reaching the end of a non-void function?

Hmm?  There's a return 0 after that which Dave's reply clipped.

--D

> -Eric
> 

Re: [PATCH 4/4] xfs: fix sign handling problem in xfs_bmbt_diff_two_keys

[Eric Sandeen]

On 8/27/19 10:19 AM, Darrick J. Wong wrote:
> On Tue, Aug 27, 2019 at 08:01:16AM -0500, Eric Sandeen wrote:
>> On 8/26/19 6:15 PM, Dave Chinner wrote:
>>> On Mon, Aug 26, 2019 at 02:49:09PM -0700, Darrick J. Wong wrote:
>>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>>
>>>> In xfs_bmbt_diff_two_keys, we perform a signed int64_t subtraction with
>>>> two unsigned 64-bit quantities.  If the second quantity is actually the
>>>> "maximum" key (all ones) as used in _query_all, the subtraction
>>>> effectively becomes addition of two positive numbers and the function
>>>> returns incorrect results.  Fix this with explicit comparisons of the
>>>> unsigned values.  Nobody needs this now, but the online repair patches
>>>> will need this to work properly.
>>>>
>>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>>> ---
>>>>  fs/xfs/libxfs/xfs_bmap_btree.c |   16 ++++++++++++++--
>>>>  1 file changed, 14 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>> diff --git a/fs/xfs/libxfs/xfs_bmap_btree.c b/fs/xfs/libxfs/xfs_bmap_btree.c
>>>> index fbb18ba5d905..3c1a805b3775 100644
>>>> --- a/fs/xfs/libxfs/xfs_bmap_btree.c
>>>> +++ b/fs/xfs/libxfs/xfs_bmap_btree.c
>>>> @@ -400,8 +400,20 @@ xfs_bmbt_diff_two_keys(
>>>>  	union xfs_btree_key	*k1,
>>>>  	union xfs_btree_key	*k2)
>>>>  {
>>>> -	return (int64_t)be64_to_cpu(k1->bmbt.br_startoff) -
>>>> -			  be64_to_cpu(k2->bmbt.br_startoff);
>>>> +	uint64_t		a = be64_to_cpu(k1->bmbt.br_startoff);
>>>> +	uint64_t		b = be64_to_cpu(k2->bmbt.br_startoff);
>>>> +
>>>> +	/*
>>>> +	 * Note: This routine previously casted a and b to int64 and subtracted
>>>> +	 * them to generate a result.  This lead to problems if b was the
>>>> +	 * "maximum" key value (all ones) being signed incorrectly, hence this
>>>> +	 * somewhat less efficient version.
>>>> +	 */
>>>> +	if (a > b)
>>>> +		return 1;
>>>> +	else if (b > a)
>>>> +		return -1;
>>>
>>> No need for an else here, but otherwise OK.
>>>
>>> Reviewed-by: Dave Chinner <dchinner@redhat.com>
>>
>> In fact having the else means the a == b case isn't handled, even if it
>> should never happen, so might a static checker eventually complain about
>> reaching the end of a non-void function?
> 
> Hmm?  There's a return 0 after that which Dave's reply clipped.

Oh sorry, patch/thread reading skills lacking, too early in the AM.

-Eric

Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

[Brian Foster]

On Tue, Aug 27, 2019 at 08:18:12AM -0700, Darrick J. Wong wrote:
> On Tue, Aug 27, 2019 at 09:14:03AM -0400, Brian Foster wrote:
> > On Mon, Aug 26, 2019 at 02:48:49PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > The inode block mapping scrub function does more work for btree format
> > > extent maps than is absolutely necessary -- first it will walk the bmbt
> > > and check all the entries, and then it will load the incore tree and
> > > check every entry in that tree, possibly for a second time.
> > > 
> > > Simplify the code and decrease check runtime by separating the two
> > > responsibilities.  The bmbt walk will make sure the incore extent
> > > mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> > > and check that every bmbt record has a corresponding incore extent map;
> > > and the incore extent map walk takes all the responsibility for checking
> > > the mapping records and cross referencing them with other AG metadata.
> > > 
> > > This enables us to clean up some messy parameter handling and reduce
> > > redundant code.  Rename a few functions to make the split of
> > > responsibilities clearer.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
> > >  1 file changed, 45 insertions(+), 31 deletions(-)
> > > 
> > > 
> > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > > index 1bd29fdc2ab5..f6ed6eb133a6 100644
> > > --- a/fs/xfs/scrub/bmap.c
> > > +++ b/fs/xfs/scrub/bmap.c
> > ...
> > > @@ -402,9 +396,25 @@ xchk_bmapbt_rec(
> > >  		}
> > >  	}
> > >  
> > > -	/* Set up the in-core record and scrub it. */
> > > +	/*
> > > +	 * Check that the incore extent tree contains an extent that matches
> > > +	 * this one exactly.  We validate those cached bmaps later, so we don't
> > > +	 * need to check them here.  If the extent tree was freshly loaded by
> > > +	 * the scrubber then we skip the check entirely.
> > > +	 */
> > > +	if (info->was_loaded)
> > > +		return 0;
> > > +
> > 
> > This all looks fine to me except that I don't follow the reasoning for
> > skipping the lookup from the comment. Are we just saying that if we
> > loaded the extent tree, then we can assume it reflects what is on disk?
> > If so, can we fix up the last sentence in the comment to explain?
> 
> Yes.
> 

Ok.

> "If the incore extent tree was just loaded from disk by the scrubber,
> we assume that its contents match what's on disk and skip the
> equivalence check." ?
> 

Sounds good. With that tweak:

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

> --D
> 
> > Brian
> > 
> > >  	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
> > > -	return xchk_bmap_extent(ip, bs->cur, info, &irec);
> > > +	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
> > > +				&iext_irec) ||
> > > +	    irec.br_startoff != iext_irec.br_startoff ||
> > > +	    irec.br_startblock != iext_irec.br_startblock ||
> > > +	    irec.br_blockcount != iext_irec.br_blockcount ||
> > > +	    irec.br_state != iext_irec.br_state)
> > > +		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
> > > +				irec.br_startoff);
> > > +	return 0;
> > >  }
> > >  
> > >  /* Scan the btree records. */
> > > @@ -415,15 +425,26 @@ xchk_bmap_btree(
> > >  	struct xchk_bmap_info	*info)
> > >  {
> > >  	struct xfs_owner_info	oinfo;
> > > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
> > >  	struct xfs_mount	*mp = sc->mp;
> > >  	struct xfs_inode	*ip = sc->ip;
> > >  	struct xfs_btree_cur	*cur;
> > >  	int			error;
> > >  
> > > +	/* Load the incore bmap cache if it's not loaded. */
> > > +	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
> > > +	if (!info->was_loaded) {
> > > +		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > > +		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > +			goto out;
> > > +	}
> > > +
> > > +	/* Check the btree structure. */
> > >  	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
> > >  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
> > >  	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
> > >  	xfs_btree_del_cursor(cur, error);
> > > +out:
> > >  	return error;
> > >  }
> > >  
> > > @@ -671,13 +692,6 @@ xchk_bmap(
> > >  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> > >  		goto out;
> > >  
> > > -	/* Now try to scrub the in-memory extent list. */
> > > -        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > > -		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > > -		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > -			goto out;
> > > -	}
> > > -
> > >  	/* Find the offset of the last extent in the mapping. */
> > >  	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
> > >  	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > @@ -689,7 +703,7 @@ xchk_bmap(
> > >  	for_each_xfs_iext(ifp, &icur, &irec) {
> > >  		if (xchk_should_terminate(sc, &error) ||
> > >  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> > > -			break;
> > > +			goto out;
> > >  		if (isnullstartblock(irec.br_startblock))
> > >  			continue;
> > >  		if (irec.br_startoff >= endoff) {
> > > @@ -697,7 +711,7 @@ xchk_bmap(
> > >  					irec.br_startoff);
> > >  			goto out;
> > >  		}
> > > -		error = xchk_bmap_extent(ip, NULL, &info, &irec);
> > > +		error = xchk_bmap_iextent(ip, &info, &irec);
> > >  		if (error)
> > >  			goto out;
> > >  	}
> > > 

Re: [PATCH 1/4] xfs: bmap scrub should only scrub records once

[Darrick J. Wong]

On Tue, Aug 27, 2019 at 11:21:47AM -0400, Brian Foster wrote:
> On Tue, Aug 27, 2019 at 08:18:12AM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 27, 2019 at 09:14:03AM -0400, Brian Foster wrote:
> > > On Mon, Aug 26, 2019 at 02:48:49PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > The inode block mapping scrub function does more work for btree format
> > > > extent maps than is absolutely necessary -- first it will walk the bmbt
> > > > and check all the entries, and then it will load the incore tree and
> > > > check every entry in that tree, possibly for a second time.
> > > > 
> > > > Simplify the code and decrease check runtime by separating the two
> > > > responsibilities.  The bmbt walk will make sure the incore extent
> > > > mappings are loaded, check the shape of the bmap btree (via xchk_btree)
> > > > and check that every bmbt record has a corresponding incore extent map;
> > > > and the incore extent map walk takes all the responsibility for checking
> > > > the mapping records and cross referencing them with other AG metadata.
> > > > 
> > > > This enables us to clean up some messy parameter handling and reduce
> > > > redundant code.  Rename a few functions to make the split of
> > > > responsibilities clearer.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  fs/xfs/scrub/bmap.c |   76 ++++++++++++++++++++++++++++++---------------------
> > > >  1 file changed, 45 insertions(+), 31 deletions(-)
> > > > 
> > > > 
> > > > diff --git a/fs/xfs/scrub/bmap.c b/fs/xfs/scrub/bmap.c
> > > > index 1bd29fdc2ab5..f6ed6eb133a6 100644
> > > > --- a/fs/xfs/scrub/bmap.c
> > > > +++ b/fs/xfs/scrub/bmap.c
> > > ...
> > > > @@ -402,9 +396,25 @@ xchk_bmapbt_rec(
> > > >  		}
> > > >  	}
> > > >  
> > > > -	/* Set up the in-core record and scrub it. */
> > > > +	/*
> > > > +	 * Check that the incore extent tree contains an extent that matches
> > > > +	 * this one exactly.  We validate those cached bmaps later, so we don't
> > > > +	 * need to check them here.  If the extent tree was freshly loaded by
> > > > +	 * the scrubber then we skip the check entirely.
> > > > +	 */
> > > > +	if (info->was_loaded)

Heh, I didn't even spot the backwards logic here.

If the bmap scrub just loaded the extent tree then was_loaded will be
false, so this needs to be "if (!info->was_loaded) return 0;".

I fixed it before I sent it to for-next.

--D

> > > > +		return 0;
> > > > +
> > > 
> > > This all looks fine to me except that I don't follow the reasoning for
> > > skipping the lookup from the comment. Are we just saying that if we
> > > loaded the extent tree, then we can assume it reflects what is on disk?
> > > If so, can we fix up the last sentence in the comment to explain?
> > 
> > Yes.
> > 
> 
> Ok.
> 
> > "If the incore extent tree was just loaded from disk by the scrubber,
> > we assume that its contents match what's on disk and skip the
> > equivalence check." ?
> > 
> 
> Sounds good. With that tweak:
> 
> Reviewed-by: Brian Foster <bfoster@redhat.com>
> 
> > --D
> > 
> > > Brian
> > > 
> > > >  	xfs_bmbt_disk_get_all(&rec->bmbt, &irec);
> > > > -	return xchk_bmap_extent(ip, bs->cur, info, &irec);
> > > > +	if (!xfs_iext_lookup_extent(ip, ifp, irec.br_startoff, &icur,
> > > > +				&iext_irec) ||
> > > > +	    irec.br_startoff != iext_irec.br_startoff ||
> > > > +	    irec.br_startblock != iext_irec.br_startblock ||
> > > > +	    irec.br_blockcount != iext_irec.br_blockcount ||
> > > > +	    irec.br_state != iext_irec.br_state)
> > > > +		xchk_fblock_set_corrupt(bs->sc, info->whichfork,
> > > > +				irec.br_startoff);
> > > > +	return 0;
> > > >  }
> > > >  
> > > >  /* Scan the btree records. */
> > > > @@ -415,15 +425,26 @@ xchk_bmap_btree(
> > > >  	struct xchk_bmap_info	*info)
> > > >  {
> > > >  	struct xfs_owner_info	oinfo;
> > > > +	struct xfs_ifork	*ifp = XFS_IFORK_PTR(sc->ip, whichfork);
> > > >  	struct xfs_mount	*mp = sc->mp;
> > > >  	struct xfs_inode	*ip = sc->ip;
> > > >  	struct xfs_btree_cur	*cur;
> > > >  	int			error;
> > > >  
> > > > +	/* Load the incore bmap cache if it's not loaded. */
> > > > +	info->was_loaded = ifp->if_flags & XFS_IFEXTENTS;
> > > > +	if (!info->was_loaded) {
> > > > +		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > > > +		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > > +			goto out;
> > > > +	}
> > > > +
> > > > +	/* Check the btree structure. */
> > > >  	cur = xfs_bmbt_init_cursor(mp, sc->tp, ip, whichfork);
> > > >  	xfs_rmap_ino_bmbt_owner(&oinfo, ip->i_ino, whichfork);
> > > >  	error = xchk_btree(sc, cur, xchk_bmapbt_rec, &oinfo, info);
> > > >  	xfs_btree_del_cursor(cur, error);
> > > > +out:
> > > >  	return error;
> > > >  }
> > > >  
> > > > @@ -671,13 +692,6 @@ xchk_bmap(
> > > >  	if (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT)
> > > >  		goto out;
> > > >  
> > > > -	/* Now try to scrub the in-memory extent list. */
> > > > -        if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > > > -		error = xfs_iread_extents(sc->tp, ip, whichfork);
> > > > -		if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > > -			goto out;
> > > > -	}
> > > > -
> > > >  	/* Find the offset of the last extent in the mapping. */
> > > >  	error = xfs_bmap_last_offset(ip, &endoff, whichfork);
> > > >  	if (!xchk_fblock_process_error(sc, whichfork, 0, &error))
> > > > @@ -689,7 +703,7 @@ xchk_bmap(
> > > >  	for_each_xfs_iext(ifp, &icur, &irec) {
> > > >  		if (xchk_should_terminate(sc, &error) ||
> > > >  		    (sc->sm->sm_flags & XFS_SCRUB_OFLAG_CORRUPT))
> > > > -			break;
> > > > +			goto out;
> > > >  		if (isnullstartblock(irec.br_startblock))
> > > >  			continue;
> > > >  		if (irec.br_startoff >= endoff) {
> > > > @@ -697,7 +711,7 @@ xchk_bmap(
> > > >  					irec.br_startoff);
> > > >  			goto out;
> > > >  		}
> > > > -		error = xchk_bmap_extent(ip, NULL, &info, &irec);
> > > > +		error = xchk_bmap_iextent(ip, &info, &irec);
> > > >  		if (error)
> > > >  			goto out;
> > > >  	}
> > > >