Re: [PATCH] xfs: keep the active perag reference between finish_one calls

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Jul 14, 2021 at 10:31:25AM +1000, Dave Chinner wrote:
> On Tue, Jul 13, 2021 at 05:00:08PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > The refcount and rmap finish_one functions stash a btree cursor when
> > there are multiple ->finish_one calls to be made in a single
> > transaction.  This mechanism is how we maintain the AGF lock between
> > operations of a single intent item.  Since ag btree cursors now need
> > active references to perag structures, we must preserve the perag
> > reference when we save the cursor.
> 
> Hmmm. The cursor already carries it's own internal reference. So
> this:
> 
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/libxfs/xfs_refcount.c |   33 ++++++++++++++++++++-------------
> >  fs/xfs/libxfs/xfs_rmap.c     |    8 +++++++-
> >  2 files changed, 27 insertions(+), 14 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_refcount.c b/fs/xfs/libxfs/xfs_refcount.c
> > index 860a0c9801ba..cfd98958d38c 100644
> > --- a/fs/xfs/libxfs/xfs_refcount.c
> > +++ b/fs/xfs/libxfs/xfs_refcount.c
> > @@ -1113,13 +1113,16 @@ xfs_refcount_finish_one_cleanup(
> >  	int			error)
> >  {
> >  	struct xfs_buf		*agbp;
> > +	struct xfs_perag	*pag;
> >  
> >  	if (rcur == NULL)
> >  		return;
> >  	agbp = rcur->bc_ag.agbp;
> > +	pag = rcur->bc_ag.pag;
> >  	xfs_btree_del_cursor(rcur, error);
> >  	if (error)
> >  		xfs_trans_brelse(tp, agbp);
> > +	xfs_perag_put(pag);
> 
> ... is just duplicating the reference the cursor already carries and
> drops inside xfs_btree_del_cursor().
> 
> What problem is this actually fixing?

LOL, I forgot that the btree cursor increments pag_ref on its own.
Self-NAK for this patch then.

--D

> 
> >  }
> >  
> >  /*
> > @@ -1142,19 +1145,20 @@ xfs_refcount_finish_one(
> >  	struct xfs_mount		*mp = tp->t_mountp;
> >  	struct xfs_btree_cur		*rcur;
> >  	struct xfs_buf			*agbp = NULL;
> > -	int				error = 0;
> > +	struct xfs_perag		*pag;
> > +	unsigned long			nr_ops = 0;
> > +	xfs_agnumber_t			agno;
> >  	xfs_agblock_t			bno;
> >  	xfs_agblock_t			new_agbno;
> > -	unsigned long			nr_ops = 0;
> >  	int				shape_changes = 0;
> > -	struct xfs_perag		*pag;
> > +	int				error = 0;
> >  
> > -	pag = xfs_perag_get(mp, XFS_FSB_TO_AGNO(mp, startblock));
> > +	agno = XFS_FSB_TO_AGNO(mp, startblock);
> > +	pag = xfs_perag_get(mp, agno);
> >  	bno = XFS_FSB_TO_AGBNO(mp, startblock);
> >  
> > -	trace_xfs_refcount_deferred(mp, XFS_FSB_TO_AGNO(mp, startblock),
> > -			type, XFS_FSB_TO_AGBNO(mp, startblock),
> > -			blockcount);
> > +	trace_xfs_refcount_deferred(mp, agno, type,
> > +			 XFS_FSB_TO_AGBNO(mp, startblock), blockcount);
> >  
> >  	if (XFS_TEST_ERROR(false, mp, XFS_ERRTAG_REFCOUNT_FINISH_ONE)) {
> >  		error = -EIO;
> > @@ -1174,14 +1178,16 @@ xfs_refcount_finish_one(
> >  		*pcur = NULL;
> >  	}
> >  	if (rcur == NULL) {
> > -		error = xfs_alloc_read_agf(tp->t_mountp, tp, pag->pag_agno,
> > +		error = xfs_alloc_read_agf(mp, tp, agno,
> >  				XFS_ALLOC_FLAG_FREEING, &agbp);
> 
> Please don't revert these back to using a local variable. The next
> step in cleaning up all these agf/agi read functions is to pass the
> perag into them rather than the mount/agno pair....
> 
> >  		if (error)
> >  			goto out_drop;
> >  
> > +		/* The cursor now owns the AGF buf and perag ref */
> >  		rcur = xfs_refcountbt_init_cursor(mp, tp, agbp, pag);
> >  		rcur->bc_ag.refc.nr_ops = nr_ops;
> >  		rcur->bc_ag.refc.shape_changes = shape_changes;
> > +		pag = NULL;
> 
> The cursor takes it's own reference inside
> xfs_refcountbt_init_cursor() that covers the perag for the life of
> the cursor. THe local get/put covers the perag for this function,
> and guarantees that the init_cursor() function can get it's own
> reference without blocking because the perag already has active
> references.
> 
> Also, the cursor doesn't actually own the agbp at all. The active
> reference to the agbp is actually carried by the transaction, not
> the cursor, and if it is dirty when xfs_trans_brelse() is called,
> then transaction reference is not dropped until
> xfs_trans_commit()...
> 
> Hence I think you're conflating "reference counted object"
> with "cursor contains an object pointer" here, and as such the
> statements about both objects are incorrect for different reasons...
> 
> >  	}
> >  	*pcur = rcur;
> >  
> > @@ -1189,12 +1195,12 @@ xfs_refcount_finish_one(
> >  	case XFS_REFCOUNT_INCREASE:
> >  		error = xfs_refcount_adjust(rcur, bno, blockcount, &new_agbno,
> >  			new_len, XFS_REFCOUNT_ADJUST_INCREASE, NULL);
> > -		*new_fsb = XFS_AGB_TO_FSB(mp, pag->pag_agno, new_agbno);
> > +		*new_fsb = XFS_AGB_TO_FSB(mp, agno, new_agbno);
> >  		break;
> >  	case XFS_REFCOUNT_DECREASE:
> >  		error = xfs_refcount_adjust(rcur, bno, blockcount, &new_agbno,
> >  			new_len, XFS_REFCOUNT_ADJUST_DECREASE, NULL);
> > -		*new_fsb = XFS_AGB_TO_FSB(mp, pag->pag_agno, new_agbno);
> > +		*new_fsb = XFS_AGB_TO_FSB(mp, agno, new_agbno);
> >  		break;
> >  	case XFS_REFCOUNT_ALLOC_COW:
> >  		*new_fsb = startblock + blockcount;
> > @@ -1211,10 +1217,11 @@ xfs_refcount_finish_one(
> >  		error = -EFSCORRUPTED;
> >  	}
> >  	if (!error && *new_len > 0)
> > -		trace_xfs_refcount_finish_one_leftover(mp, pag->pag_agno, type,
> > -				bno, blockcount, new_agbno, *new_len);
> > +		trace_xfs_refcount_finish_one_leftover(mp, agno, type, bno,
> > +				blockcount, new_agbno, *new_len);
> >  out_drop:
> > -	xfs_perag_put(pag);
> > +	if (pag)
> > +		xfs_perag_put(pag);
> >  	return error;
> 
> Yup, this just smells wrong.  The local get/put covers the reference
> for the local function references, the reference gained in
> _init_cursor ensures the perag is referenced for the life of the
> cursor across multiple iterations (including duplicated child
> cursors that also take their own references).
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com