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

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

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