[2/3] xfs: only invalidate blocks if we're going to free them
diff mbox series

Message ID 157063972723.2913192.12835516373692425243.stgit@magnolia
State New
Headers show
Series
  • xfs: fix online repair block reaping
Related show

Commit Message

Darrick J. Wong Oct. 9, 2019, 4:48 p.m. UTC
From: Darrick J. Wong <darrick.wong@oracle.com>

When we're discarding old btree blocks after a repair, only invalidate
the buffers for the ones that we're freeing -- if the metadata was
crosslinked with another data structure, we don't want to touch it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/repair.c |   76 +++++++++++++++++++++++--------------------------
 fs/xfs/scrub/repair.h |    1 -
 2 files changed, 35 insertions(+), 42 deletions(-)

Comments

Brian Foster Oct. 17, 2019, 12:55 p.m. UTC | #1
On Wed, Oct 09, 2019 at 09:48:47AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're discarding old btree blocks after a repair, only invalidate
> the buffers for the ones that we're freeing -- if the metadata was
> crosslinked with another data structure, we don't want to touch it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   76 +++++++++++++++++++++++--------------------------
>  fs/xfs/scrub/repair.h |    1 -
>  2 files changed, 35 insertions(+), 42 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index 3a58788e0bd8..e21faef6db5a 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
...
> @@ -515,6 +477,35 @@ xrep_put_freelist(
>  	return 0;
>  }
>  
> +/* Try to invalidate the incore buffer for a block that we're about to free. */
> +STATIC void
> +xrep_reap_invalidate_block(
> +	struct xfs_scrub	*sc,
> +	xfs_fsblock_t		fsbno)
> +{
> +	struct xfs_buf		*bp;
> +
> +	/*
> +	 * For each block in each extent, see if there's an incore buffer for
> +	 * exactly that block; if so, invalidate it.  The buffer cache only
> +	 * lets us look for one buffer at a time, so we have to look one block
> +	 * at a time.  Avoid invalidating AG headers and post-EOFS blocks
> +	 * because we never own those; and if we can't TRYLOCK the buffer we
> +	 * assume it's owned by someone else.
> +	 */
> +	/* Skip AG headers and post-EOFS blocks */

The comment doesn't seem to quite go with the implementation any longer.
Also, there's probably no need to have two separate comments here.
Otherwise looks Ok.

Brian

> +	if (!xfs_verify_fsbno(sc->mp, fsbno))
> +		return;
> +	bp = xfs_buf_incore(sc->mp->m_ddev_targp,
> +			XFS_FSB_TO_DADDR(sc->mp, fsbno),
> +			XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK);
> +	if (!bp)
> +		return;
> +
> +	xfs_trans_bjoin(sc->tp, bp);
> +	xfs_trans_binval(sc->tp, bp);
> +}
> +
>  /* Dispose of a single block. */
>  STATIC int
>  xrep_reap_block(
> @@ -568,12 +559,15 @@ xrep_reap_block(
>  	 * blow on writeout, the filesystem will shut down, and the admin gets
>  	 * to run xfs_repair.
>  	 */
> -	if (has_other_rmap)
> +	if (has_other_rmap) {
>  		error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1, oinfo);
> -	else if (resv == XFS_AG_RESV_AGFL)
> +	} else if (resv == XFS_AG_RESV_AGFL) {
> +		xrep_reap_invalidate_block(sc, fsbno);
>  		error = xrep_put_freelist(sc, agbno);
> -	else
> +	} else {
> +		xrep_reap_invalidate_block(sc, fsbno);
>  		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
> +	}
>  	if (agf_bp != sc->sa.agf_bp)
>  		xfs_trans_brelse(sc->tp, agf_bp);
>  	if (error)
> diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> index 60c61d7052a8..eab41928990f 100644
> --- a/fs/xfs/scrub/repair.h
> +++ b/fs/xfs/scrub/repair.h
> @@ -31,7 +31,6 @@ int xrep_init_btblock(struct xfs_scrub *sc, xfs_fsblock_t fsb,
>  struct xfs_bitmap;
>  
>  int xrep_fix_freelist(struct xfs_scrub *sc, bool can_shrink);
> -int xrep_invalidate_blocks(struct xfs_scrub *sc, struct xfs_bitmap *btlist);
>  int xrep_reap_extents(struct xfs_scrub *sc, struct xfs_bitmap *exlist,
>  		const struct xfs_owner_info *oinfo, enum xfs_ag_resv_type type);
>  
>
Darrick J. Wong Oct. 25, 2019, 8:22 p.m. UTC | #2
On Thu, Oct 17, 2019 at 08:55:12AM -0400, Brian Foster wrote:
> On Wed, Oct 09, 2019 at 09:48:47AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we're discarding old btree blocks after a repair, only invalidate
> > the buffers for the ones that we're freeing -- if the metadata was
> > crosslinked with another data structure, we don't want to touch it.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/repair.c |   76 +++++++++++++++++++++++--------------------------
> >  fs/xfs/scrub/repair.h |    1 -
> >  2 files changed, 35 insertions(+), 42 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index 3a58788e0bd8..e21faef6db5a 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> ...
> > @@ -515,6 +477,35 @@ xrep_put_freelist(
> >  	return 0;
> >  }
> >  
> > +/* Try to invalidate the incore buffer for a block that we're about to free. */
> > +STATIC void
> > +xrep_reap_invalidate_block(
> > +	struct xfs_scrub	*sc,
> > +	xfs_fsblock_t		fsbno)
> > +{
> > +	struct xfs_buf		*bp;
> > +
> > +	/*
> > +	 * For each block in each extent, see if there's an incore buffer for
> > +	 * exactly that block; if so, invalidate it.  The buffer cache only
> > +	 * lets us look for one buffer at a time, so we have to look one block
> > +	 * at a time.  Avoid invalidating AG headers and post-EOFS blocks
> > +	 * because we never own those; and if we can't TRYLOCK the buffer we
> > +	 * assume it's owned by someone else.
> > +	 */
> > +	/* Skip AG headers and post-EOFS blocks */
> 
> The comment doesn't seem to quite go with the implementation any longer.
> Also, there's probably no need to have two separate comments here.
> Otherwise looks Ok.

<nod> Fixed.

--D

> Brian
> 
> > +	if (!xfs_verify_fsbno(sc->mp, fsbno))
> > +		return;
> > +	bp = xfs_buf_incore(sc->mp->m_ddev_targp,
> > +			XFS_FSB_TO_DADDR(sc->mp, fsbno),
> > +			XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK);
> > +	if (!bp)
> > +		return;
> > +
> > +	xfs_trans_bjoin(sc->tp, bp);
> > +	xfs_trans_binval(sc->tp, bp);
> > +}
> > +
> >  /* Dispose of a single block. */
> >  STATIC int
> >  xrep_reap_block(
> > @@ -568,12 +559,15 @@ xrep_reap_block(
> >  	 * blow on writeout, the filesystem will shut down, and the admin gets
> >  	 * to run xfs_repair.
> >  	 */
> > -	if (has_other_rmap)
> > +	if (has_other_rmap) {
> >  		error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1, oinfo);
> > -	else if (resv == XFS_AG_RESV_AGFL)
> > +	} else if (resv == XFS_AG_RESV_AGFL) {
> > +		xrep_reap_invalidate_block(sc, fsbno);
> >  		error = xrep_put_freelist(sc, agbno);
> > -	else
> > +	} else {
> > +		xrep_reap_invalidate_block(sc, fsbno);
> >  		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
> > +	}
> >  	if (agf_bp != sc->sa.agf_bp)
> >  		xfs_trans_brelse(sc->tp, agf_bp);
> >  	if (error)
> > diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
> > index 60c61d7052a8..eab41928990f 100644
> > --- a/fs/xfs/scrub/repair.h
> > +++ b/fs/xfs/scrub/repair.h
> > @@ -31,7 +31,6 @@ int xrep_init_btblock(struct xfs_scrub *sc, xfs_fsblock_t fsb,
> >  struct xfs_bitmap;
> >  
> >  int xrep_fix_freelist(struct xfs_scrub *sc, bool can_shrink);
> > -int xrep_invalidate_blocks(struct xfs_scrub *sc, struct xfs_bitmap *btlist);
> >  int xrep_reap_extents(struct xfs_scrub *sc, struct xfs_bitmap *exlist,
> >  		const struct xfs_owner_info *oinfo, enum xfs_ag_resv_type type);
> >  
> >

Patch
diff mbox series

diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index 3a58788e0bd8..e21faef6db5a 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -423,44 +423,6 @@  xrep_init_btblock(
  * buffers associated with @bitmap.
  */
 
-/*
- * Invalidate buffers for per-AG btree blocks we're dumping.  This function
- * is not intended for use with file data repairs; we have bunmapi for that.
- */
-int
-xrep_invalidate_blocks(
-	struct xfs_scrub	*sc,
-	struct xfs_bitmap	*bitmap)
-{
-	struct xfs_bitmap_range	*bmr;
-	struct xfs_bitmap_range	*n;
-	struct xfs_buf		*bp;
-	xfs_fsblock_t		fsbno;
-
-	/*
-	 * For each block in each extent, see if there's an incore buffer for
-	 * exactly that block; if so, invalidate it.  The buffer cache only
-	 * lets us look for one buffer at a time, so we have to look one block
-	 * at a time.  Avoid invalidating AG headers and post-EOFS blocks
-	 * because we never own those; and if we can't TRYLOCK the buffer we
-	 * assume it's owned by someone else.
-	 */
-	for_each_xfs_bitmap_block(fsbno, bmr, n, bitmap) {
-		/* Skip AG headers and post-EOFS blocks */
-		if (!xfs_verify_fsbno(sc->mp, fsbno))
-			continue;
-		bp = xfs_buf_incore(sc->mp->m_ddev_targp,
-				XFS_FSB_TO_DADDR(sc->mp, fsbno),
-				XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK);
-		if (bp) {
-			xfs_trans_bjoin(sc->tp, bp);
-			xfs_trans_binval(sc->tp, bp);
-		}
-	}
-
-	return 0;
-}
-
 /* Ensure the freelist is the correct size. */
 int
 xrep_fix_freelist(
@@ -515,6 +477,35 @@  xrep_put_freelist(
 	return 0;
 }
 
+/* Try to invalidate the incore buffer for a block that we're about to free. */
+STATIC void
+xrep_reap_invalidate_block(
+	struct xfs_scrub	*sc,
+	xfs_fsblock_t		fsbno)
+{
+	struct xfs_buf		*bp;
+
+	/*
+	 * For each block in each extent, see if there's an incore buffer for
+	 * exactly that block; if so, invalidate it.  The buffer cache only
+	 * lets us look for one buffer at a time, so we have to look one block
+	 * at a time.  Avoid invalidating AG headers and post-EOFS blocks
+	 * because we never own those; and if we can't TRYLOCK the buffer we
+	 * assume it's owned by someone else.
+	 */
+	/* Skip AG headers and post-EOFS blocks */
+	if (!xfs_verify_fsbno(sc->mp, fsbno))
+		return;
+	bp = xfs_buf_incore(sc->mp->m_ddev_targp,
+			XFS_FSB_TO_DADDR(sc->mp, fsbno),
+			XFS_FSB_TO_BB(sc->mp, 1), XBF_TRYLOCK);
+	if (!bp)
+		return;
+
+	xfs_trans_bjoin(sc->tp, bp);
+	xfs_trans_binval(sc->tp, bp);
+}
+
 /* Dispose of a single block. */
 STATIC int
 xrep_reap_block(
@@ -568,12 +559,15 @@  xrep_reap_block(
 	 * blow on writeout, the filesystem will shut down, and the admin gets
 	 * to run xfs_repair.
 	 */
-	if (has_other_rmap)
+	if (has_other_rmap) {
 		error = xfs_rmap_free(sc->tp, agf_bp, agno, agbno, 1, oinfo);
-	else if (resv == XFS_AG_RESV_AGFL)
+	} else if (resv == XFS_AG_RESV_AGFL) {
+		xrep_reap_invalidate_block(sc, fsbno);
 		error = xrep_put_freelist(sc, agbno);
-	else
+	} else {
+		xrep_reap_invalidate_block(sc, fsbno);
 		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
+	}
 	if (agf_bp != sc->sa.agf_bp)
 		xfs_trans_brelse(sc->tp, agf_bp);
 	if (error)
diff --git a/fs/xfs/scrub/repair.h b/fs/xfs/scrub/repair.h
index 60c61d7052a8..eab41928990f 100644
--- a/fs/xfs/scrub/repair.h
+++ b/fs/xfs/scrub/repair.h
@@ -31,7 +31,6 @@  int xrep_init_btblock(struct xfs_scrub *sc, xfs_fsblock_t fsb,
 struct xfs_bitmap;
 
 int xrep_fix_freelist(struct xfs_scrub *sc, bool can_shrink);
-int xrep_invalidate_blocks(struct xfs_scrub *sc, struct xfs_bitmap *btlist);
 int xrep_reap_extents(struct xfs_scrub *sc, struct xfs_bitmap *exlist,
 		const struct xfs_owner_info *oinfo, enum xfs_ag_resv_type type);