diff mbox series

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

Message ID 156685612978.2853532.15764464511279169366.stgit@magnolia (mailing list archive)
State Accepted
Headers show
Series xfs: fixes for 5.4 | expand

Commit Message

Darrick J. Wong Aug. 26, 2019, 9:48 p.m. UTC 
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(-)

Comments

Dave Chinner Aug. 26, 2019, 11:08 p.m. UTC | #1 
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>
Brian Foster Aug. 27, 2019, 1:14 p.m. UTC | #2 
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;
>  	}
>
Darrick J. Wong Aug. 27, 2019, 3:18 p.m. UTC | #3 
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;
> >  	}
> >
Brian Foster Aug. 27, 2019, 3:21 p.m. UTC | #4 
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;
> > >  	}
> > >
Darrick J. Wong Aug. 28, 2019, 4:01 p.m. UTC | #5 
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;
> > > >  	}
> > > >
diff mbox series

Patch

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;
 	}