Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs

[Dave Chinner <david@fromorbit.com>]

On Tue, Jun 02, 2020 at 05:22:22PM -0700, Darrick J. Wong wrote:
> On Tue, Jun 02, 2020 at 10:52:38PM +0800, Gao Xiang wrote:
> > Sometimes no need to play with perag_tree since for many
> > cases perag can also be accessed by agbp reliably.
> > 
> > Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> > ---
....
> > @@ -2447,32 +2443,22 @@ xfs_iunlink_remove(
> >  	 * this inode's backref to point from the next inode.
> >  	 */
> >  	if (next_agino != NULLAGINO) {
> > -		pag = xfs_perag_get(mp, agno);
> > -		error = xfs_iunlink_change_backref(pag, next_agino,
> > +		error = xfs_iunlink_change_backref(agibp->b_pag, next_agino,
> >  				NULLAGINO);
> >  		if (error)
> > -			goto out;
> > +			return error;
> >  	}
> >  
> > -	if (head_agino == agino) {
> > -		/* Point the head of the list to the next unlinked inode. */
> > -		error = xfs_iunlink_update_bucket(tp, agno, agibp, bucket_index,
> > -				next_agino);
> > -		if (error)
> > -			goto out;
> > -	} else {
> > +	if (head_agino != agino) {
> >  		struct xfs_imap	imap;
> >  		xfs_agino_t	prev_agino;
> >  
> > -		if (!pag)
> > -			pag = xfs_perag_get(mp, agno);
> > -
> >  		/* We need to search the list for the inode being freed. */
> >  		error = xfs_iunlink_map_prev(tp, agno, head_agino, agino,
> >  				&prev_agino, &imap, &last_dip, &last_ibp,
> > -				pag);
> > +				agibp->b_pag);
> >  		if (error)
> > -			goto out;
> > +			return error;
> >  
> >  		/* Point the previous inode on the list to the next inode. */
> >  		xfs_iunlink_update_dinode(tp, agno, prev_agino, last_ibp,
> > @@ -2486,15 +2472,13 @@ xfs_iunlink_remove(
> >  		 * change_backref takes care of deleting the backref if
> >  		 * next_agino is NULLAGINO.
> >  		 */
> > -		error = xfs_iunlink_change_backref(pag, agino, next_agino);
> > -		if (error)
> > -			goto out;
> > +		return xfs_iunlink_change_backref(agibp->b_pag, agino,
> > +				next_agino);
> >  	}
> >  
> > -out:
> > -	if (pag)
> > -		xfs_perag_put(pag);
> > -	return error;
> > +	/* Point the head of the list to the next unlinked inode. */
> > +	return xfs_iunlink_update_bucket(tp, agno, agibp, bucket_index,
> > +			next_agino);
> 
> Why not cut out the agno argument here too?  Surely you could obtain it
> from agibp->b_pag->pag_agno.  Ditto for xfs_iunlink_map_prev.

Those functions go away completely in the patchset I'm currently
working on for tracking dirty inodes in ordered buffers. The
in-memory unlinked list code needs to be completely reworked to
acheive this (due to lock order constraints), so I'd much prefer
unnecessary cleanup changes in this area are kept to a minimum
because it will all away real soon.

FWIW, it was the discovery we could use agibp->b_pag instead of
get/put in my iunlink list rework that prompted me to ask Xiang to
look at the rest of the code and see where the same pattern could be
applied...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com