[PATCH 0/3] xfs: fix online repair block reaping

[PATCH 0/3] xfs: fix online repair block reaping

[Darrick J. Wong]

Hi all,

These patches fix a few problems that I noticed in the code that deals
with old btree blocks after a successful repair. First, we clarify how
the reaping function works w.r.t. bitmap lifetimes.  Next we fix a bug
where we could incorrectly invalidate old btree blocks if they were
crosslinked.  Finally, we convert the reap function to use EFIs so that
we can delete blocks without overloading a transaction.

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=repair-reap-fixes

[PATCH 1/3] xfs: xrep_reap_extents should not destroy the bitmap

[Darrick J. Wong]

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

Remove the xfs_bitmap_destroy call from the end of xrep_reap_extents
because this sort of violates our rule that the function initializing a
structure should destroy it.

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


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 7a1a38b636a9..8fcd43040c96 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -698,7 +698,7 @@ xrep_agfl(
 		goto err;
 
 	/* Dump any AGFL overflow. */
-	return xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
+	error = xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
 			XFS_AG_RESV_AGFL);
 err:
 	xfs_bitmap_destroy(&agfl_extents);
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index b70a88bc975e..3a58788e0bd8 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -613,11 +613,9 @@ xrep_reap_extents(
 
 		error = xrep_reap_block(sc, fsbno, oinfo, type);
 		if (error)
-			goto out;
+			break;
 	}
 
-out:
-	xfs_bitmap_destroy(bitmap);
 	return error;
 }
 

Re: [PATCH 1/3] xfs: xrep_reap_extents should not destroy the bitmap

[Brian Foster]

On Wed, Oct 09, 2019 at 09:48:40AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Remove the xfs_bitmap_destroy call from the end of xrep_reap_extents
> because this sort of violates our rule that the function initializing a
> structure should destroy it.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

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

>  fs/xfs/scrub/agheader_repair.c |    2 +-
>  fs/xfs/scrub/repair.c          |    4 +---
>  2 files changed, 2 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
> index 7a1a38b636a9..8fcd43040c96 100644
> --- a/fs/xfs/scrub/agheader_repair.c
> +++ b/fs/xfs/scrub/agheader_repair.c
> @@ -698,7 +698,7 @@ xrep_agfl(
>  		goto err;
>  
>  	/* Dump any AGFL overflow. */
> -	return xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
> +	error = xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
>  			XFS_AG_RESV_AGFL);
>  err:
>  	xfs_bitmap_destroy(&agfl_extents);
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index b70a88bc975e..3a58788e0bd8 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
> @@ -613,11 +613,9 @@ xrep_reap_extents(
>  
>  		error = xrep_reap_block(sc, fsbno, oinfo, type);
>  		if (error)
> -			goto out;
> +			break;
>  	}
>  
> -out:
> -	xfs_bitmap_destroy(bitmap);
>  	return error;
>  }
>  
> 

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

[Darrick J. Wong]

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
@@ -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);
 

Re: [PATCH 2/3] xfs: only invalidate blocks if we're going to free them

[Brian Foster]

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

Re: [PATCH 2/3] xfs: only invalidate blocks if we're going to free them

[Darrick J. Wong]

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 3/3] xfs: use deferred frees to reap old btree blocks

[Darrick J. Wong]

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

Use deferred frees (EFIs) to reap the blocks of a btree that we just
replaced.  This helps us to shrink the window in which those old blocks
could be lost due to a system crash, though we try to flush the EFIs
every few hundred blocks so that we don't also overflow the transaction
reservations during and after we commit the new btree.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fs/xfs/scrub/repair.c |   29 ++++++++++++++++++++++++-----
 1 file changed, 24 insertions(+), 5 deletions(-)


diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index e21faef6db5a..8349694f985d 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -24,6 +24,7 @@
 #include "xfs_extent_busy.h"
 #include "xfs_ag_resv.h"
 #include "xfs_quota.h"
+#include "xfs_bmap.h"
 #include "scrub/scrub.h"
 #include "scrub/common.h"
 #include "scrub/trace.h"
@@ -512,13 +513,15 @@ xrep_reap_block(
 	struct xfs_scrub		*sc,
 	xfs_fsblock_t			fsbno,
 	const struct xfs_owner_info	*oinfo,
-	enum xfs_ag_resv_type		resv)
+	enum xfs_ag_resv_type		resv,
+	unsigned int			*deferred)
 {
 	struct xfs_btree_cur		*cur;
 	struct xfs_buf			*agf_bp = NULL;
 	xfs_agnumber_t			agno;
 	xfs_agblock_t			agbno;
 	bool				has_other_rmap;
+	bool				need_roll = true;
 	int				error;
 
 	agno = XFS_FSB_TO_AGNO(sc->mp, fsbno);
@@ -565,14 +568,24 @@ xrep_reap_block(
 		xrep_reap_invalidate_block(sc, fsbno);
 		error = xrep_put_freelist(sc, agbno);
 	} else {
+		/*
+		 * Use deferred frees to get rid of the old btree blocks to try
+		 * to minimize the window in which we could crash and lose the
+		 * old blocks.  However, we still need to roll the transaction
+		 * every 100 or so EFIs so that we don't exceed the log
+		 * reservation.
+		 */
 		xrep_reap_invalidate_block(sc, fsbno);
-		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
+		__xfs_bmap_add_free(sc->tp, fsbno, 1, oinfo, true);
+		(*deferred)++;
+		need_roll = *deferred > 100;
 	}
 	if (agf_bp != sc->sa.agf_bp)
 		xfs_trans_brelse(sc->tp, agf_bp);
-	if (error)
+	if (error || !need_roll)
 		return error;
 
+	*deferred = 0;
 	if (sc->ip)
 		return xfs_trans_roll_inode(&sc->tp, sc->ip);
 	return xrep_roll_ag_trans(sc);
@@ -594,6 +607,7 @@ xrep_reap_extents(
 	struct xfs_bitmap_range		*bmr;
 	struct xfs_bitmap_range		*n;
 	xfs_fsblock_t			fsbno;
+	unsigned int			deferred = 0;
 	int				error = 0;
 
 	ASSERT(xfs_sb_version_hasrmapbt(&sc->mp->m_sb));
@@ -605,12 +619,17 @@ xrep_reap_extents(
 				XFS_FSB_TO_AGNO(sc->mp, fsbno),
 				XFS_FSB_TO_AGBNO(sc->mp, fsbno), 1);
 
-		error = xrep_reap_block(sc, fsbno, oinfo, type);
+		error = xrep_reap_block(sc, fsbno, oinfo, type, &deferred);
 		if (error)
 			break;
 	}
 
-	return error;
+	if (error || deferred == 0)
+		return error;
+
+	if (sc->ip)
+		return xfs_trans_roll_inode(&sc->tp, sc->ip);
+	return xrep_roll_ag_trans(sc);
 }
 
 /*

Re: [PATCH 3/3] xfs: use deferred frees to reap old btree blocks

[Brian Foster]

On Wed, Oct 09, 2019 at 09:48:53AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use deferred frees (EFIs) to reap the blocks of a btree that we just
> replaced.  This helps us to shrink the window in which those old blocks
> could be lost due to a system crash, though we try to flush the EFIs
> every few hundred blocks so that we don't also overflow the transaction
> reservations during and after we commit the new btree.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  fs/xfs/scrub/repair.c |   29 ++++++++++++++++++++++++-----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> index e21faef6db5a..8349694f985d 100644
> --- a/fs/xfs/scrub/repair.c
> +++ b/fs/xfs/scrub/repair.c
...
> @@ -565,14 +568,24 @@ xrep_reap_block(
>  		xrep_reap_invalidate_block(sc, fsbno);
>  		error = xrep_put_freelist(sc, agbno);
>  	} else {
> +		/*
> +		 * Use deferred frees to get rid of the old btree blocks to try
> +		 * to minimize the window in which we could crash and lose the
> +		 * old blocks.  However, we still need to roll the transaction
> +		 * every 100 or so EFIs so that we don't exceed the log
> +		 * reservation.
> +		 */
>  		xrep_reap_invalidate_block(sc, fsbno);
> -		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
> +		__xfs_bmap_add_free(sc->tp, fsbno, 1, oinfo, true);

xfs_free_extent() sets skip_discard to false and this changes it to
true. Intentional?

Otherwise the rest looks straightforward.

Brian

> +		(*deferred)++;
> +		need_roll = *deferred > 100;
>  	}
>  	if (agf_bp != sc->sa.agf_bp)
>  		xfs_trans_brelse(sc->tp, agf_bp);
> -	if (error)
> +	if (error || !need_roll)
>  		return error;
>  
> +	*deferred = 0;
>  	if (sc->ip)
>  		return xfs_trans_roll_inode(&sc->tp, sc->ip);
>  	return xrep_roll_ag_trans(sc);
> @@ -594,6 +607,7 @@ xrep_reap_extents(
>  	struct xfs_bitmap_range		*bmr;
>  	struct xfs_bitmap_range		*n;
>  	xfs_fsblock_t			fsbno;
> +	unsigned int			deferred = 0;
>  	int				error = 0;
>  
>  	ASSERT(xfs_sb_version_hasrmapbt(&sc->mp->m_sb));
> @@ -605,12 +619,17 @@ xrep_reap_extents(
>  				XFS_FSB_TO_AGNO(sc->mp, fsbno),
>  				XFS_FSB_TO_AGBNO(sc->mp, fsbno), 1);
>  
> -		error = xrep_reap_block(sc, fsbno, oinfo, type);
> +		error = xrep_reap_block(sc, fsbno, oinfo, type, &deferred);
>  		if (error)
>  			break;
>  	}
>  
> -	return error;
> +	if (error || deferred == 0)
> +		return error;
> +
> +	if (sc->ip)
> +		return xfs_trans_roll_inode(&sc->tp, sc->ip);
> +	return xrep_roll_ag_trans(sc);
>  }
>  
>  /*
> 

Re: [PATCH 3/3] xfs: use deferred frees to reap old btree blocks

[Darrick J. Wong]

On Thu, Oct 17, 2019 at 08:55:37AM -0400, Brian Foster wrote:
> On Wed, Oct 09, 2019 at 09:48:53AM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Use deferred frees (EFIs) to reap the blocks of a btree that we just
> > replaced.  This helps us to shrink the window in which those old blocks
> > could be lost due to a system crash, though we try to flush the EFIs
> > every few hundred blocks so that we don't also overflow the transaction
> > reservations during and after we commit the new btree.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  fs/xfs/scrub/repair.c |   29 ++++++++++++++++++++++++-----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
> > index e21faef6db5a..8349694f985d 100644
> > --- a/fs/xfs/scrub/repair.c
> > +++ b/fs/xfs/scrub/repair.c
> ...
> > @@ -565,14 +568,24 @@ xrep_reap_block(
> >  		xrep_reap_invalidate_block(sc, fsbno);
> >  		error = xrep_put_freelist(sc, agbno);
> >  	} else {
> > +		/*
> > +		 * Use deferred frees to get rid of the old btree blocks to try
> > +		 * to minimize the window in which we could crash and lose the
> > +		 * old blocks.  However, we still need to roll the transaction
> > +		 * every 100 or so EFIs so that we don't exceed the log
> > +		 * reservation.
> > +		 */
> >  		xrep_reap_invalidate_block(sc, fsbno);
> > -		error = xfs_free_extent(sc->tp, fsbno, 1, oinfo, resv);
> > +		__xfs_bmap_add_free(sc->tp, fsbno, 1, oinfo, true);
> 
> xfs_free_extent() sets skip_discard to false and this changes it to
> true. Intentional?

Nope.  Will fix this (and the comment in the previous patch).

--D

> Otherwise the rest looks straightforward.
> 
> Brian
> 
> > +		(*deferred)++;
> > +		need_roll = *deferred > 100;
> >  	}
> >  	if (agf_bp != sc->sa.agf_bp)
> >  		xfs_trans_brelse(sc->tp, agf_bp);
> > -	if (error)
> > +	if (error || !need_roll)
> >  		return error;
> >  
> > +	*deferred = 0;
> >  	if (sc->ip)
> >  		return xfs_trans_roll_inode(&sc->tp, sc->ip);
> >  	return xrep_roll_ag_trans(sc);
> > @@ -594,6 +607,7 @@ xrep_reap_extents(
> >  	struct xfs_bitmap_range		*bmr;
> >  	struct xfs_bitmap_range		*n;
> >  	xfs_fsblock_t			fsbno;
> > +	unsigned int			deferred = 0;
> >  	int				error = 0;
> >  
> >  	ASSERT(xfs_sb_version_hasrmapbt(&sc->mp->m_sb));
> > @@ -605,12 +619,17 @@ xrep_reap_extents(
> >  				XFS_FSB_TO_AGNO(sc->mp, fsbno),
> >  				XFS_FSB_TO_AGBNO(sc->mp, fsbno), 1);
> >  
> > -		error = xrep_reap_block(sc, fsbno, oinfo, type);
> > +		error = xrep_reap_block(sc, fsbno, oinfo, type, &deferred);
> >  		if (error)
> >  			break;
> >  	}
> >  
> > -	return error;
> > +	if (error || deferred == 0)
> > +		return error;
> > +
> > +	if (sc->ip)
> > +		return xfs_trans_roll_inode(&sc->tp, sc->ip);
> > +	return xrep_roll_ag_trans(sc);
> >  }
> >  
> >  /*
> > 

[PATCH v2 0/3] xfs: fix online repair block reaping

[Darrick J. Wong]

Hi all,

These patches fix a few problems that I noticed in the code that deals
with old btree blocks after a successful repair. First, we clarify how
the reaping function works w.r.t. bitmap lifetimes.  Next we fix a bug
where we could incorrectly invalidate old btree blocks if they were
crosslinked.  Finally, we convert the reap function to use EFIs so that
we can delete blocks without overloading a transaction.

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=repair-reap-fixes

[PATCH 1/3] xfs: xrep_reap_extents should not destroy the bitmap

[Darrick J. Wong]

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

Remove the xfs_bitmap_destroy call from the end of xrep_reap_extents
because this sort of violates our rule that the function initializing a
structure should destroy it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/scrub/agheader_repair.c |    2 +-
 fs/xfs/scrub/repair.c          |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 7a1a38b636a9..8fcd43040c96 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -698,7 +698,7 @@ xrep_agfl(
 		goto err;
 
 	/* Dump any AGFL overflow. */
-	return xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
+	error = xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
 			XFS_AG_RESV_AGFL);
 err:
 	xfs_bitmap_destroy(&agfl_extents);
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index b70a88bc975e..3a58788e0bd8 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -613,11 +613,9 @@ xrep_reap_extents(
 
 		error = xrep_reap_block(sc, fsbno, oinfo, type);
 		if (error)
-			goto out;
+			break;
 	}
 
-out:
-	xfs_bitmap_destroy(bitmap);
 	return error;
 }
 

[PATCH v2 0/3] xfs: fix online repair block reaping

[Darrick J. Wong]

Hi all,

These patches fix a few problems that I noticed in the code that deals
with old btree blocks after a successful repair. First, we clarify how
the reaping function works w.r.t. bitmap lifetimes.  Next we fix a bug
where we could incorrectly invalidate old btree blocks if they were
crosslinked.  Finally, we convert the reap function to use EFIs so that
we can delete blocks without overloading a transaction.

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=repair-reap-fixes

[PATCH 1/3] xfs: xrep_reap_extents should not destroy the bitmap

[Darrick J. Wong]

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

Remove the xfs_bitmap_destroy call from the end of xrep_reap_extents
because this sort of violates our rule that the function initializing a
structure should destroy it.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Brian Foster <bfoster@redhat.com>
---
 fs/xfs/scrub/agheader_repair.c |    2 +-
 fs/xfs/scrub/repair.c          |    4 +---
 2 files changed, 2 insertions(+), 4 deletions(-)


diff --git a/fs/xfs/scrub/agheader_repair.c b/fs/xfs/scrub/agheader_repair.c
index 7a1a38b636a9..8fcd43040c96 100644
--- a/fs/xfs/scrub/agheader_repair.c
+++ b/fs/xfs/scrub/agheader_repair.c
@@ -698,7 +698,7 @@ xrep_agfl(
 		goto err;
 
 	/* Dump any AGFL overflow. */
-	return xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
+	error = xrep_reap_extents(sc, &agfl_extents, &XFS_RMAP_OINFO_AG,
 			XFS_AG_RESV_AGFL);
 err:
 	xfs_bitmap_destroy(&agfl_extents);
diff --git a/fs/xfs/scrub/repair.c b/fs/xfs/scrub/repair.c
index b70a88bc975e..3a58788e0bd8 100644
--- a/fs/xfs/scrub/repair.c
+++ b/fs/xfs/scrub/repair.c
@@ -613,11 +613,9 @@ xrep_reap_extents(
 
 		error = xrep_reap_block(sc, fsbno, oinfo, type);
 		if (error)
-			goto out;
+			break;
 	}
 
-out:
-	xfs_bitmap_destroy(bitmap);
 	return error;
 }