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

["Darrick J. Wong" <darrick.wong@oracle.com>]

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);
> >  }
> >  
> >  /*
> >