xfs: get rid of unnecessary xfs_perag_{get,put} pairs
diff mbox series

Message ID 20200602145238.1512-1-hsiangkao@redhat.com
State Superseded
Headers show
Series
  • xfs: get rid of unnecessary xfs_perag_{get,put} pairs
Related show

Commit Message

Gao Xiang June 2, 2020, 2:52 p.m. UTC
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>
---
Not sure addressing all the cases, but seems mostly.
Kindly correct me if something wrong somewhere...

 fs/xfs/libxfs/xfs_ag.c             |  4 ++--
 fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
 fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
 fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
 fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
 fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
 fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
 7 files changed, 35 insertions(+), 77 deletions(-)

Comments

Darrick J. Wong June 3, 2020, 12:22 a.m. UTC | #1
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>
> ---
> Not sure addressing all the cases, but seems mostly.
> Kindly correct me if something wrong somewhere...
> 
>  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
>  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
>  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
>  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
>  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
>  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
>  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
>  7 files changed, 35 insertions(+), 77 deletions(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9d84007a5c65..8cf73fe4338e 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -563,7 +563,8 @@ xfs_ag_get_geometry(
>  	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agf_bp);
>  	if (error)
>  		goto out_agi;
> -	pag = xfs_perag_get(mp, agno);
> +
> +	pag = agi_bp->b_pag;
>  
>  	/* Fill out form. */
>  	memset(ageo, 0, sizeof(*ageo));
> @@ -583,7 +584,6 @@ xfs_ag_get_geometry(
>  	xfs_ag_geom_health(pag, ageo);
>  
>  	/* Release resources. */
> -	xfs_perag_put(pag);

Looks like a reaosnable pattern throughout the codebase.  Did fstests
cough up any new errors?

>  	xfs_buf_relse(agf_bp);
>  out_agi:
>  	xfs_buf_relse(agi_bp);
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 203e74fa64aa..bf4d07e5c73f 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -710,13 +710,12 @@ xfs_alloc_read_agfl(
>  STATIC int
>  xfs_alloc_update_counters(
>  	struct xfs_trans	*tp,
> -	struct xfs_perag	*pag,
>  	struct xfs_buf		*agbp,
>  	long			len)
>  {
>  	struct xfs_agf		*agf = agbp->b_addr;
>  
> -	pag->pagf_freeblks += len;
> +	agbp->b_pag->pagf_freeblks += len;
>  	be32_add_cpu(&agf->agf_freeblks, len);
>  
>  	xfs_trans_agblocks_delta(tp, len);
> @@ -1175,8 +1174,7 @@ xfs_alloc_ag_vextent(
>  	}
>  
>  	if (!args->wasfromfl) {
> -		error = xfs_alloc_update_counters(args->tp, args->pag,
> -						  args->agbp,
> +		error = xfs_alloc_update_counters(args->tp, args->agbp,
>  						  -((long)(args->len)));
>  		if (error)
>  			return error;
> @@ -1887,7 +1885,6 @@ xfs_free_ag_extent(
>  	enum xfs_ag_resv_type		type)
>  {
>  	struct xfs_mount		*mp;
> -	struct xfs_perag		*pag;
>  	struct xfs_btree_cur		*bno_cur;
>  	struct xfs_btree_cur		*cnt_cur;
>  	xfs_agblock_t			gtbno; /* start of right neighbor */
> @@ -2167,10 +2164,8 @@ xfs_free_ag_extent(
>  	/*
>  	 * Update the freespace totals in the ag and superblock.
>  	 */
> -	pag = xfs_perag_get(mp, agno);
> -	error = xfs_alloc_update_counters(tp, pag, agbp, len);
> -	xfs_ag_resv_free_extent(pag, type, tp, len);
> -	xfs_perag_put(pag);
> +	error = xfs_alloc_update_counters(tp, agbp, len);
> +	xfs_ag_resv_free_extent(agbp->b_pag, type, tp, len);
>  	if (error)
>  		goto error0;
>  
> @@ -2689,7 +2684,7 @@ xfs_alloc_get_freelist(
>  	if (be32_to_cpu(agf->agf_flfirst) == xfs_agfl_size(mp))
>  		agf->agf_flfirst = 0;
>  
> -	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
> +	pag = agbp->b_pag;
>  	ASSERT(!pag->pagf_agflreset);
>  	be32_add_cpu(&agf->agf_flcount, -1);
>  	xfs_trans_agflist_delta(tp, -1);
> @@ -2701,7 +2696,6 @@ xfs_alloc_get_freelist(
>  		pag->pagf_btreeblks++;
>  		logflags |= XFS_AGF_BTREEBLKS;
>  	}
> -	xfs_perag_put(pag);
>  
>  	xfs_alloc_log_agf(tp, agbp, logflags);
>  	*bnop = bno;
> @@ -2797,7 +2791,7 @@ xfs_alloc_put_freelist(
>  	if (be32_to_cpu(agf->agf_fllast) == xfs_agfl_size(mp))
>  		agf->agf_fllast = 0;
>  
> -	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
> +	pag = agbp->b_pag;
>  	ASSERT(!pag->pagf_agflreset);
>  	be32_add_cpu(&agf->agf_flcount, 1);
>  	xfs_trans_agflist_delta(tp, 1);
> @@ -2809,7 +2803,6 @@ xfs_alloc_put_freelist(
>  		pag->pagf_btreeblks--;
>  		logflags |= XFS_AGF_BTREEBLKS;
>  	}
> -	xfs_perag_put(pag);
>  
>  	xfs_alloc_log_agf(tp, agbp, logflags);
>  
> @@ -3006,7 +2999,7 @@ xfs_alloc_read_agf(
>  	ASSERT(!(*bpp)->b_error);
>  
>  	agf = (*bpp)->b_addr;
> -	pag = xfs_perag_get(mp, agno);
> +	pag = (*bpp)->b_pag;
>  	if (!pag->pagf_init) {
>  		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
>  		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
> @@ -3034,7 +3027,6 @@ xfs_alloc_read_agf(
>  		       be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]));
>  	}
>  #endif
> -	xfs_perag_put(pag);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
> index 60c453cb3ee3..c1d276f791ea 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -38,16 +38,15 @@ xfs_allocbt_set_root(
>  {
>  	struct xfs_buf		*agbp = cur->bc_ag.agbp;
>  	struct xfs_agf		*agf = agbp->b_addr;
> -	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
>  	int			btnum = cur->bc_btnum;
> -	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
> +	struct xfs_perag	*pag = agbp->b_pag;
>  
>  	ASSERT(ptr->s != 0);
> +	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
>  
>  	agf->agf_roots[btnum] = ptr->s;
>  	be32_add_cpu(&agf->agf_levels[btnum], inc);
>  	pag->pagf_levels[btnum] += inc;
> -	xfs_perag_put(pag);
>  
>  	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_ROOTS | XFS_AGF_LEVELS);
>  }
> @@ -115,7 +114,6 @@ xfs_allocbt_update_lastrec(
>  	int			reason)
>  {
>  	struct xfs_agf		*agf = cur->bc_ag.agbp->b_addr;
> -	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
>  	struct xfs_perag	*pag;
>  	__be32			len;
>  	int			numrecs;
> @@ -160,9 +158,9 @@ xfs_allocbt_update_lastrec(
>  	}
>  
>  	agf->agf_longest = len;
> -	pag = xfs_perag_get(cur->bc_mp, seqno);
> +	pag = cur->bc_ag.agbp->b_pag;
> +	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
>  	pag->pagf_longest = be32_to_cpu(len);
> -	xfs_perag_put(pag);
>  	xfs_alloc_log_agf(cur->bc_tp, cur->bc_ag.agbp, XFS_AGF_LONGEST);
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index 7fcf62b324b0..f742a96a2fe1 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -888,10 +888,9 @@ xfs_ialloc_ag_alloc(
>  	 */
>  	be32_add_cpu(&agi->agi_count, newlen);
>  	be32_add_cpu(&agi->agi_freecount, newlen);
> -	pag = xfs_perag_get(args.mp, agno);
> +	pag = agbp->b_pag;
>  	pag->pagi_freecount += newlen;
>  	pag->pagi_count += newlen;
> -	xfs_perag_put(pag);
>  	agi->agi_newino = cpu_to_be32(newino);
>  
>  	/*
> @@ -1134,7 +1133,7 @@ xfs_dialloc_ag_inobt(
>  	xfs_agnumber_t		agno = be32_to_cpu(agi->agi_seqno);
>  	xfs_agnumber_t		pagno = XFS_INO_TO_AGNO(mp, parent);
>  	xfs_agino_t		pagino = XFS_INO_TO_AGINO(mp, parent);
> -	struct xfs_perag	*pag;
> +	struct xfs_perag	*pag = agbp->b_pag;
>  	struct xfs_btree_cur	*cur, *tcur;
>  	struct xfs_inobt_rec_incore rec, trec;
>  	xfs_ino_t		ino;
> @@ -1143,8 +1142,6 @@ xfs_dialloc_ag_inobt(
>  	int			i, j;
>  	int			searchdistance = 10;
>  
> -	pag = xfs_perag_get(mp, agno);
> -
>  	ASSERT(pag->pagi_init);
>  	ASSERT(pag->pagi_inodeok);
>  	ASSERT(pag->pagi_freecount > 0);
> @@ -1384,14 +1381,12 @@ xfs_dialloc_ag_inobt(
>  
>  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
> -	xfs_perag_put(pag);
>  	*inop = ino;
>  	return 0;
>  error1:
>  	xfs_btree_del_cursor(tcur, XFS_BTREE_ERROR);
>  error0:
>  	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> -	xfs_perag_put(pag);
>  	return error;
>  }
>  
> @@ -1587,7 +1582,6 @@ xfs_dialloc_ag(
>  	xfs_agnumber_t			agno = be32_to_cpu(agi->agi_seqno);
>  	xfs_agnumber_t			pagno = XFS_INO_TO_AGNO(mp, parent);
>  	xfs_agino_t			pagino = XFS_INO_TO_AGINO(mp, parent);
> -	struct xfs_perag		*pag;
>  	struct xfs_btree_cur		*cur;	/* finobt cursor */
>  	struct xfs_btree_cur		*icur;	/* inobt cursor */
>  	struct xfs_inobt_rec_incore	rec;
> @@ -1599,8 +1593,6 @@ xfs_dialloc_ag(
>  	if (!xfs_sb_version_hasfinobt(&mp->m_sb))
>  		return xfs_dialloc_ag_inobt(tp, agbp, parent, inop);
>  
> -	pag = xfs_perag_get(mp, agno);
> -
>  	/*
>  	 * If pagino is 0 (this is the root inode allocation) use newino.
>  	 * This must work because we've just allocated some.
> @@ -1667,7 +1659,7 @@ xfs_dialloc_ag(
>  	 */
>  	be32_add_cpu(&agi->agi_freecount, -1);
>  	xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
> -	pag->pagi_freecount--;
> +	agbp->b_pag->pagi_freecount--;
>  
>  	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
>  
> @@ -1680,7 +1672,6 @@ xfs_dialloc_ag(
>  
>  	xfs_btree_del_cursor(icur, XFS_BTREE_NOERROR);
>  	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> -	xfs_perag_put(pag);
>  	*inop = ino;
>  	return 0;
>  
> @@ -1688,7 +1679,6 @@ xfs_dialloc_ag(
>  	xfs_btree_del_cursor(icur, XFS_BTREE_ERROR);
>  error_cur:
>  	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
> -	xfs_perag_put(pag);
>  	return error;
>  }
>  
> @@ -1945,7 +1935,6 @@ xfs_difree_inobt(
>  {
>  	struct xfs_agi			*agi = agbp->b_addr;
>  	xfs_agnumber_t			agno = be32_to_cpu(agi->agi_seqno);
> -	struct xfs_perag		*pag;
>  	struct xfs_btree_cur		*cur;
>  	struct xfs_inobt_rec_incore	rec;
>  	int				ilen;
> @@ -2007,6 +1996,8 @@ xfs_difree_inobt(
>  	if (!(mp->m_flags & XFS_MOUNT_IKEEP) &&
>  	    rec.ir_free == XFS_INOBT_ALL_FREE &&
>  	    mp->m_sb.sb_inopblock <= XFS_INODES_PER_CHUNK) {
> +		struct xfs_perag	*pag = agbp->b_pag;
> +
>  		xic->deleted = true;
>  		xic->first_ino = XFS_AGINO_TO_INO(mp, agno, rec.ir_startino);
>  		xic->alloc = xfs_inobt_irec_to_allocmask(&rec);
> @@ -2020,10 +2011,8 @@ xfs_difree_inobt(
>  		be32_add_cpu(&agi->agi_count, -ilen);
>  		be32_add_cpu(&agi->agi_freecount, -(ilen - 1));
>  		xfs_ialloc_log_agi(tp, agbp, XFS_AGI_COUNT | XFS_AGI_FREECOUNT);
> -		pag = xfs_perag_get(mp, agno);
>  		pag->pagi_freecount -= ilen - 1;
>  		pag->pagi_count -= ilen;
> -		xfs_perag_put(pag);
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, -ilen);
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -(ilen - 1));
>  
> @@ -2049,9 +2038,7 @@ xfs_difree_inobt(
>  		 */
>  		be32_add_cpu(&agi->agi_freecount, 1);
>  		xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
> -		pag = xfs_perag_get(mp, agno);
> -		pag->pagi_freecount++;
> -		xfs_perag_put(pag);
> +		agbp->b_pag->pagi_freecount++;
>  		xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, 1);
>  	}
>  
> @@ -2661,7 +2648,7 @@ xfs_ialloc_read_agi(
>  		return error;
>  
>  	agi = (*bpp)->b_addr;
> -	pag = xfs_perag_get(mp, agno);
> +	pag = (*bpp)->b_pag;
>  	if (!pag->pagi_init) {
>  		pag->pagi_freecount = be32_to_cpu(agi->agi_freecount);
>  		pag->pagi_count = be32_to_cpu(agi->agi_count);
> @@ -2674,7 +2661,6 @@ xfs_ialloc_read_agi(
>  	 */
>  	ASSERT(pag->pagi_freecount == be32_to_cpu(agi->agi_freecount) ||
>  		XFS_FORCED_SHUTDOWN(mp));
> -	xfs_perag_put(pag);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
> index 7fd6044a4f78..f445a8e2d04e 100644
> --- a/fs/xfs/libxfs/xfs_refcount_btree.c
> +++ b/fs/xfs/libxfs/xfs_refcount_btree.c
> @@ -37,15 +37,14 @@ xfs_refcountbt_set_root(
>  {
>  	struct xfs_buf		*agbp = cur->bc_ag.agbp;
>  	struct xfs_agf		*agf = agbp->b_addr;
> -	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
> -	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
> +	struct xfs_perag	*pag = agbp->b_pag;
>  
>  	ASSERT(ptr->s != 0);
> +	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
>  
>  	agf->agf_refcount_root = ptr->s;
>  	be32_add_cpu(&agf->agf_refcount_level, inc);
>  	pag->pagf_refcount_level += inc;
> -	xfs_perag_put(pag);
>  
>  	xfs_alloc_log_agf(cur->bc_tp, agbp,
>  			XFS_AGF_REFCOUNT_ROOT | XFS_AGF_REFCOUNT_LEVEL);
> diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
> index b7c05314d07c..959d9df1964e 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -63,16 +63,15 @@ xfs_rmapbt_set_root(
>  {
>  	struct xfs_buf		*agbp = cur->bc_ag.agbp;
>  	struct xfs_agf		*agf = agbp->b_addr;
> -	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
>  	int			btnum = cur->bc_btnum;
> -	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
> +	struct xfs_perag	*pag = agbp->b_pag;
>  
>  	ASSERT(ptr->s != 0);
> +	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
>  
>  	agf->agf_roots[btnum] = ptr->s;
>  	be32_add_cpu(&agf->agf_levels[btnum], inc);
>  	pag->pagf_levels[btnum] += inc;
> -	xfs_perag_put(pag);
>  
>  	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_ROOTS | XFS_AGF_LEVELS);
>  }
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 64f5f9a440ae..d0e7db7a2c32 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2248,7 +2248,6 @@ xfs_iunlink(
>  	}
>  
>  	if (next_agino != NULLAGINO) {
> -		struct xfs_perag	*pag;
>  		xfs_agino_t		old_agino;
>  
>  		/*
> @@ -2265,9 +2264,7 @@ xfs_iunlink(
>  		 * agino has been unlinked, add a backref from the next inode
>  		 * back to agino.
>  		 */
> -		pag = xfs_perag_get(mp, agno);
> -		error = xfs_iunlink_add_backref(pag, agino, next_agino);
> -		xfs_perag_put(pag);
> +		error = xfs_iunlink_add_backref(agibp->b_pag, agino, next_agino);
>  		if (error)
>  			return error;
>  	}
> @@ -2403,7 +2400,6 @@ xfs_iunlink_remove(
>  	struct xfs_buf		*agibp;
>  	struct xfs_buf		*last_ibp;
>  	struct xfs_dinode	*last_dip = NULL;
> -	struct xfs_perag	*pag = NULL;
>  	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
>  	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
>  	xfs_agino_t		next_agino;
> @@ -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.

--D

>  }
>  
>  /*
> -- 
> 2.18.1
>
Dave Chinner June 3, 2020, 12:44 a.m. UTC | #2
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.
Darrick J. Wong June 3, 2020, 12:48 a.m. UTC | #3
On Wed, Jun 03, 2020 at 10:44:29AM +1000, Dave Chinner wrote:
> 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...

Aha!  I wondered.  Ok, let's just leave this part alone then?  No harm
nor foul letting it stay in the meantime. :)

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
Gao Xiang June 3, 2020, 12:49 a.m. UTC | #4
Hi Darrick,

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>
> > ---
> > Not sure addressing all the cases, but seems mostly.
> > Kindly correct me if something wrong somewhere...
> > 
> >  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
> >  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
> >  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
> >  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
> >  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
> >  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
> >  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
> >  7 files changed, 35 insertions(+), 77 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 9d84007a5c65..8cf73fe4338e 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -563,7 +563,8 @@ xfs_ag_get_geometry(
> >  	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agf_bp);
> >  	if (error)
> >  		goto out_agi;
> > -	pag = xfs_perag_get(mp, agno);
> > +
> > +	pag = agi_bp->b_pag;
> >  
> >  	/* Fill out form. */
> >  	memset(ageo, 0, sizeof(*ageo));
> > @@ -583,7 +584,6 @@ xfs_ag_get_geometry(
> >  	xfs_ag_geom_health(pag, ageo);
> >  
> >  	/* Release resources. */
> > -	xfs_perag_put(pag);
> 
> Looks like a reaosnable pattern throughout the codebase.  Did fstests
> cough up any new errors?

Actually I add more extra ASSERTs (to check pag_agno vs agno) around
all the cases (many of them aren't shown in this patch), and have been
running fstests and fsstress with ASSERT version for a while. It seems
no visible crash. But I'm not sure how many exist failures are (at least
no panic yells out)...

> 
> >  	xfs_buf_relse(agf_bp);

...

> > -		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.

Okay, thanks for pointing out... I think there are many indirect
potential cleanups indeed.... Will fix the above cases in the
next version...

Thanks,
Gao Xiang

> 
> --D
> 
> >  }
> >  
> >  /*
> > -- 
> > 2.18.1
> > 
>
Dave Chinner June 3, 2020, 1:27 a.m. UTC | #5
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>

Hi Xiang,

One of the quirks of XFS is that we tend towards commit messages
that explain the reason for the change than the actual change being
made in the commit message. That means we'll put iinformation about
how to reproduce bugs, the problem that needed to be solved,
assumptions that are being made, etc into the commit message rather
than describe what the change being made is. We can see what the
change is from the code, but we may not be able to understand why
the change is being made from reading the code.

Hence we try to put the "why?" into the commit message so that
everyone reviewing the code knows this information without having to
ask. This also means that we capture the reasons/thinking/issues
that the commit address in the code repository and hence when we
look up a change (e.g. when considering if we need to back port it
to another kernel), we have a good idea of what problem that change
is addressing. It also means that in a few months/years time when
we've forgotten exactly why a specific change was made, the commit
message should contain enough detail to remind us.

Perhaps something like this?

	In the course of some operations, we look up the perag from
	the mount multiple times to get or change perag information.
	These are often very short pieces of code, so while the
	lookup cost is generally low, the cost of the lookup is far
	higher than the cost of the operation we are doing on the
	perag.

	Since we changed buffers to hold references to the perag
	they are cached in, many modification contexts already hold
	active references to the perag that are held across these
	operations. This is especially true for any operation that
	is serialised by an allocation group header buffer.

	In these cases, we can just use the buffer's reference to
	the perag to avoid needing to do lookups to access the
	perag. This means that many operations don't need to do
	perag lookups at all to access the perag because they've
	already looked up objects that own persistent references
	and hence can use that reference instead.

The first paragraph explains the problem. The second paragraph
explains the underlying assumption the change depends on. And the
third paragraph defines the scope we can apply the general pattern
to.

It takes a while to get used to doing this - for any major change I
tend to write the series description first (the requirements and
design doc), then for each patch I write the commit message before
I start modifying the code (detailed design). Treating the commit
messages as design documentation really helps other people
understand the changes being made....

> ---
> Not sure addressing all the cases, but seems mostly.
> Kindly correct me if something wrong somewhere...
> 
>  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
>  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
>  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
>  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
>  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
>  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
>  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
>  7 files changed, 35 insertions(+), 77 deletions(-)

There were more places using this pattern than I thought. :)

With an updated commit message,

Reviewed-by: Dave Chinner <dchinner@redhat.com>
Gao Xiang June 3, 2020, 1:40 a.m. UTC | #6
On Wed, Jun 03, 2020 at 11:27:34AM +1000, Dave Chinner 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>
> 
> Hi Xiang,

Hi Dave,

> 
> One of the quirks of XFS is that we tend towards commit messages
> that explain the reason for the change than the actual change being
> made in the commit message. That means we'll put iinformation about
> how to reproduce bugs, the problem that needed to be solved,
> assumptions that are being made, etc into the commit message rather
> than describe what the change being made is. We can see what the
> change is from the code, but we may not be able to understand why
> the change is being made from reading the code.
> 
> Hence we try to put the "why?" into the commit message so that
> everyone reviewing the code knows this information without having to
> ask. This also means that we capture the reasons/thinking/issues
> that the commit address in the code repository and hence when we
> look up a change (e.g. when considering if we need to back port it
> to another kernel), we have a good idea of what problem that change
> is addressing. It also means that in a few months/years time when
> we've forgotten exactly why a specific change was made, the commit
> message should contain enough detail to remind us.

Okay, I understood. I'm a newbie to XFS here. I'll try to add more
reasons/thoughts from now on with my current limited knowledage
about XFS.

> 
> Perhaps something like this?
> 
> 	In the course of some operations, we look up the perag from
> 	the mount multiple times to get or change perag information.
> 	These are often very short pieces of code, so while the
> 	lookup cost is generally low, the cost of the lookup is far
> 	higher than the cost of the operation we are doing on the
> 	perag.
> 
> 	Since we changed buffers to hold references to the perag
> 	they are cached in, many modification contexts already hold
> 	active references to the perag that are held across these
> 	operations. This is especially true for any operation that
> 	is serialised by an allocation group header buffer.
> 
> 	In these cases, we can just use the buffer's reference to
> 	the perag to avoid needing to do lookups to access the
> 	perag. This means that many operations don't need to do
> 	perag lookups at all to access the perag because they've
> 	already looked up objects that own persistent references
> 	and hence can use that reference instead.
> 
> The first paragraph explains the problem. The second paragraph
> explains the underlying assumption the change depends on. And the
> third paragraph defines the scope we can apply the general pattern
> to.
> 
> It takes a while to get used to doing this - for any major change I
> tend to write the series description first (the requirements and
> design doc), then for each patch I write the commit message before
> I start modifying the code (detailed design). Treating the commit
> messages as design documentation really helps other people
> understand the changes being made....

Yeah, I saw many patchsets of you.. Partially due to my limited
knowledge and somewhat limited English skill though... But I will
write more as much as possible to get myself better...

> 
> > ---
> > Not sure addressing all the cases, but seems mostly.
> > Kindly correct me if something wrong somewhere...
> > 
> >  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
> >  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
> >  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
> >  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
> >  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
> >  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
> >  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
> >  7 files changed, 35 insertions(+), 77 deletions(-)
> 
> There were more places using this pattern than I thought. :)
> 
> With an updated commit message,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>

Thanks for your review. b.t.w, would you tend to drop all extra ASSERTs
or leave these ASSERTs for a while to catch potential issues on this
patch?... And in addition I will try to find more potential cases, if
not, I will just send out with updated commit messages (maybe without
iunlink orphan inode related part, just to confirm?).

Thanks,
Gao Xiang

> 
> -- 
> Dave Chinner
> david@fromorbit.com
>
Dave Chinner June 3, 2020, 3:02 a.m. UTC | #7
On Wed, Jun 03, 2020 at 09:40:39AM +0800, Gao Xiang wrote:
> On Wed, Jun 03, 2020 at 11:27:34AM +1000, Dave Chinner wrote:
> > On Tue, Jun 02, 2020 at 10:52:38PM +0800, Gao Xiang wrote:
> > >  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
> > >  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
> > >  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
> > >  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
> > >  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
> > >  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
> > >  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
> > >  7 files changed, 35 insertions(+), 77 deletions(-)
> > 
> > There were more places using this pattern than I thought. :)
> > 
> > With an updated commit message,
> > 
> > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Thanks for your review. b.t.w, would you tend to drop all extra ASSERTs
> or leave these ASSERTs for a while to catch potential issues on this
> patch?...

We typically use ASSERT() statements to document assumptions the
function implementation makes. e.g. if we expect that the inode is
locked on entry to a function, rather than adding that as a comment
we'll do:

	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));

That way our debug builds validate that all the callers of the
function are doing the right thing.

I frequently add ASSERT()s when debugging my code, but then remove
once I've found the issue. Typically I'm adding asserts to cover
conditions I know shouldn't occur, but could be caused by a bug and
I try to place the asserts to catch the issue earlier than what I'm
currently seeing. Depending on which debug assert fires first, I'll
change/add/remove asserts to further narrow down the problem.

Hence the ASSERTs I tend to leave in the code are either documenting
assumptions or were the ones that were most helpful in debugging the
changes I was making.

I did think about the asserts you added, wondering if they were
necessary. But then I noticed they were replicating a pattern in
other parts of the code so they seemed like a reasonable addition.

> And in addition I will try to find more potential cases, if
> not, I will just send out with updated commit messages (maybe without
> iunlink orphan inode related part, just to confirm?).

Your original patch is fine including those iunlink bits. I was was
simply pointing out that spending more time cleaning up the iunlink
code wasn't worth spending time on because I've got much more
substantial changes that address those issues already...

Cheers,

Dave.
Gao Xiang June 3, 2020, 3:19 a.m. UTC | #8
On Wed, Jun 03, 2020 at 01:02:41PM +1000, Dave Chinner wrote:
> On Wed, Jun 03, 2020 at 09:40:39AM +0800, Gao Xiang wrote:
> > On Wed, Jun 03, 2020 at 11:27:34AM +1000, Dave Chinner wrote:
> > > On Tue, Jun 02, 2020 at 10:52:38PM +0800, Gao Xiang wrote:
> > > >  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
> > > >  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
> > > >  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
> > > >  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
> > > >  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
> > > >  fs/xfs/libxfs/xfs_rmap_btree.c     |  5 ++--
> > > >  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
> > > >  7 files changed, 35 insertions(+), 77 deletions(-)
> > > 
> > > There were more places using this pattern than I thought. :)
> > > 
> > > With an updated commit message,
> > > 
> > > Reviewed-by: Dave Chinner <dchinner@redhat.com>
> > 
> > Thanks for your review. b.t.w, would you tend to drop all extra ASSERTs
> > or leave these ASSERTs for a while to catch potential issues on this
> > patch?...
> 
> We typically use ASSERT() statements to document assumptions the
> function implementation makes. e.g. if we expect that the inode is
> locked on entry to a function, rather than adding that as a comment
> we'll do:
> 
> 	ASSERT(xfs_isilocked(ip, XFS_ILOCK_EXCL));

Yes, that's the typical use for most filesystems.

> 
> That way our debug builds validate that all the callers of the
> function are doing the right thing.
> 
> I frequently add ASSERT()s when debugging my code, but then remove
> once I've found the issue. Typically I'm adding asserts to cover
> conditions I know shouldn't occur, but could be caused by a bug and
> I try to place the asserts to catch the issue earlier than what I'm
> currently seeing. Depending on which debug assert fires first, I'll
> change/add/remove asserts to further narrow down the problem.
> 
> Hence the ASSERTs I tend to leave in the code are either documenting
> assumptions or were the ones that were most helpful in debugging the
> changes I was making.
> 
> I did think about the asserts you added, wondering if they were
> necessary. But then I noticed they were replicating a pattern in
> other parts of the code so they seemed like a reasonable addition.

Okay... I will follow your suggestion and fold in all remaining
ASSERTs (was not in this version) about this pattern. Will sort
out the next version later...

> 
> > And in addition I will try to find more potential cases, if
> > not, I will just send out with updated commit messages (maybe without
> > iunlink orphan inode related part, just to confirm?).
> 
> Your original patch is fine including those iunlink bits. I was was
> simply pointing out that spending more time cleaning up the iunlink
> code wasn't worth spending time on because I've got much more
> substantial changes that address those issues already...

Okay...

Thanks,
Gao Xiang

> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>

Patch
diff mbox series

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9d84007a5c65..8cf73fe4338e 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -563,7 +563,8 @@  xfs_ag_get_geometry(
 	error = xfs_alloc_read_agf(mp, NULL, agno, 0, &agf_bp);
 	if (error)
 		goto out_agi;
-	pag = xfs_perag_get(mp, agno);
+
+	pag = agi_bp->b_pag;
 
 	/* Fill out form. */
 	memset(ageo, 0, sizeof(*ageo));
@@ -583,7 +584,6 @@  xfs_ag_get_geometry(
 	xfs_ag_geom_health(pag, ageo);
 
 	/* Release resources. */
-	xfs_perag_put(pag);
 	xfs_buf_relse(agf_bp);
 out_agi:
 	xfs_buf_relse(agi_bp);
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 203e74fa64aa..bf4d07e5c73f 100644
--- a/fs/xfs/libxfs/xfs_alloc.c
+++ b/fs/xfs/libxfs/xfs_alloc.c
@@ -710,13 +710,12 @@  xfs_alloc_read_agfl(
 STATIC int
 xfs_alloc_update_counters(
 	struct xfs_trans	*tp,
-	struct xfs_perag	*pag,
 	struct xfs_buf		*agbp,
 	long			len)
 {
 	struct xfs_agf		*agf = agbp->b_addr;
 
-	pag->pagf_freeblks += len;
+	agbp->b_pag->pagf_freeblks += len;
 	be32_add_cpu(&agf->agf_freeblks, len);
 
 	xfs_trans_agblocks_delta(tp, len);
@@ -1175,8 +1174,7 @@  xfs_alloc_ag_vextent(
 	}
 
 	if (!args->wasfromfl) {
-		error = xfs_alloc_update_counters(args->tp, args->pag,
-						  args->agbp,
+		error = xfs_alloc_update_counters(args->tp, args->agbp,
 						  -((long)(args->len)));
 		if (error)
 			return error;
@@ -1887,7 +1885,6 @@  xfs_free_ag_extent(
 	enum xfs_ag_resv_type		type)
 {
 	struct xfs_mount		*mp;
-	struct xfs_perag		*pag;
 	struct xfs_btree_cur		*bno_cur;
 	struct xfs_btree_cur		*cnt_cur;
 	xfs_agblock_t			gtbno; /* start of right neighbor */
@@ -2167,10 +2164,8 @@  xfs_free_ag_extent(
 	/*
 	 * Update the freespace totals in the ag and superblock.
 	 */
-	pag = xfs_perag_get(mp, agno);
-	error = xfs_alloc_update_counters(tp, pag, agbp, len);
-	xfs_ag_resv_free_extent(pag, type, tp, len);
-	xfs_perag_put(pag);
+	error = xfs_alloc_update_counters(tp, agbp, len);
+	xfs_ag_resv_free_extent(agbp->b_pag, type, tp, len);
 	if (error)
 		goto error0;
 
@@ -2689,7 +2684,7 @@  xfs_alloc_get_freelist(
 	if (be32_to_cpu(agf->agf_flfirst) == xfs_agfl_size(mp))
 		agf->agf_flfirst = 0;
 
-	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
+	pag = agbp->b_pag;
 	ASSERT(!pag->pagf_agflreset);
 	be32_add_cpu(&agf->agf_flcount, -1);
 	xfs_trans_agflist_delta(tp, -1);
@@ -2701,7 +2696,6 @@  xfs_alloc_get_freelist(
 		pag->pagf_btreeblks++;
 		logflags |= XFS_AGF_BTREEBLKS;
 	}
-	xfs_perag_put(pag);
 
 	xfs_alloc_log_agf(tp, agbp, logflags);
 	*bnop = bno;
@@ -2797,7 +2791,7 @@  xfs_alloc_put_freelist(
 	if (be32_to_cpu(agf->agf_fllast) == xfs_agfl_size(mp))
 		agf->agf_fllast = 0;
 
-	pag = xfs_perag_get(mp, be32_to_cpu(agf->agf_seqno));
+	pag = agbp->b_pag;
 	ASSERT(!pag->pagf_agflreset);
 	be32_add_cpu(&agf->agf_flcount, 1);
 	xfs_trans_agflist_delta(tp, 1);
@@ -2809,7 +2803,6 @@  xfs_alloc_put_freelist(
 		pag->pagf_btreeblks--;
 		logflags |= XFS_AGF_BTREEBLKS;
 	}
-	xfs_perag_put(pag);
 
 	xfs_alloc_log_agf(tp, agbp, logflags);
 
@@ -3006,7 +2999,7 @@  xfs_alloc_read_agf(
 	ASSERT(!(*bpp)->b_error);
 
 	agf = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
 	if (!pag->pagf_init) {
 		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
 		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
@@ -3034,7 +3027,6 @@  xfs_alloc_read_agf(
 		       be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNTi]));
 	}
 #endif
-	xfs_perag_put(pag);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_alloc_btree.c b/fs/xfs/libxfs/xfs_alloc_btree.c
index 60c453cb3ee3..c1d276f791ea 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -38,16 +38,15 @@  xfs_allocbt_set_root(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
-	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
 	int			btnum = cur->bc_btnum;
-	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
+	struct xfs_perag	*pag = agbp->b_pag;
 
 	ASSERT(ptr->s != 0);
+	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
 
 	agf->agf_roots[btnum] = ptr->s;
 	be32_add_cpu(&agf->agf_levels[btnum], inc);
 	pag->pagf_levels[btnum] += inc;
-	xfs_perag_put(pag);
 
 	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_ROOTS | XFS_AGF_LEVELS);
 }
@@ -115,7 +114,6 @@  xfs_allocbt_update_lastrec(
 	int			reason)
 {
 	struct xfs_agf		*agf = cur->bc_ag.agbp->b_addr;
-	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
 	struct xfs_perag	*pag;
 	__be32			len;
 	int			numrecs;
@@ -160,9 +158,9 @@  xfs_allocbt_update_lastrec(
 	}
 
 	agf->agf_longest = len;
-	pag = xfs_perag_get(cur->bc_mp, seqno);
+	pag = cur->bc_ag.agbp->b_pag;
+	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
 	pag->pagf_longest = be32_to_cpu(len);
-	xfs_perag_put(pag);
 	xfs_alloc_log_agf(cur->bc_tp, cur->bc_ag.agbp, XFS_AGF_LONGEST);
 }
 
diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
index 7fcf62b324b0..f742a96a2fe1 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -888,10 +888,9 @@  xfs_ialloc_ag_alloc(
 	 */
 	be32_add_cpu(&agi->agi_count, newlen);
 	be32_add_cpu(&agi->agi_freecount, newlen);
-	pag = xfs_perag_get(args.mp, agno);
+	pag = agbp->b_pag;
 	pag->pagi_freecount += newlen;
 	pag->pagi_count += newlen;
-	xfs_perag_put(pag);
 	agi->agi_newino = cpu_to_be32(newino);
 
 	/*
@@ -1134,7 +1133,7 @@  xfs_dialloc_ag_inobt(
 	xfs_agnumber_t		agno = be32_to_cpu(agi->agi_seqno);
 	xfs_agnumber_t		pagno = XFS_INO_TO_AGNO(mp, parent);
 	xfs_agino_t		pagino = XFS_INO_TO_AGINO(mp, parent);
-	struct xfs_perag	*pag;
+	struct xfs_perag	*pag = agbp->b_pag;
 	struct xfs_btree_cur	*cur, *tcur;
 	struct xfs_inobt_rec_incore rec, trec;
 	xfs_ino_t		ino;
@@ -1143,8 +1142,6 @@  xfs_dialloc_ag_inobt(
 	int			i, j;
 	int			searchdistance = 10;
 
-	pag = xfs_perag_get(mp, agno);
-
 	ASSERT(pag->pagi_init);
 	ASSERT(pag->pagi_inodeok);
 	ASSERT(pag->pagi_freecount > 0);
@@ -1384,14 +1381,12 @@  xfs_dialloc_ag_inobt(
 
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
-	xfs_perag_put(pag);
 	*inop = ino;
 	return 0;
 error1:
 	xfs_btree_del_cursor(tcur, XFS_BTREE_ERROR);
 error0:
 	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-	xfs_perag_put(pag);
 	return error;
 }
 
@@ -1587,7 +1582,6 @@  xfs_dialloc_ag(
 	xfs_agnumber_t			agno = be32_to_cpu(agi->agi_seqno);
 	xfs_agnumber_t			pagno = XFS_INO_TO_AGNO(mp, parent);
 	xfs_agino_t			pagino = XFS_INO_TO_AGINO(mp, parent);
-	struct xfs_perag		*pag;
 	struct xfs_btree_cur		*cur;	/* finobt cursor */
 	struct xfs_btree_cur		*icur;	/* inobt cursor */
 	struct xfs_inobt_rec_incore	rec;
@@ -1599,8 +1593,6 @@  xfs_dialloc_ag(
 	if (!xfs_sb_version_hasfinobt(&mp->m_sb))
 		return xfs_dialloc_ag_inobt(tp, agbp, parent, inop);
 
-	pag = xfs_perag_get(mp, agno);
-
 	/*
 	 * If pagino is 0 (this is the root inode allocation) use newino.
 	 * This must work because we've just allocated some.
@@ -1667,7 +1659,7 @@  xfs_dialloc_ag(
 	 */
 	be32_add_cpu(&agi->agi_freecount, -1);
 	xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
-	pag->pagi_freecount--;
+	agbp->b_pag->pagi_freecount--;
 
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
 
@@ -1680,7 +1672,6 @@  xfs_dialloc_ag(
 
 	xfs_btree_del_cursor(icur, XFS_BTREE_NOERROR);
 	xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
-	xfs_perag_put(pag);
 	*inop = ino;
 	return 0;
 
@@ -1688,7 +1679,6 @@  xfs_dialloc_ag(
 	xfs_btree_del_cursor(icur, XFS_BTREE_ERROR);
 error_cur:
 	xfs_btree_del_cursor(cur, XFS_BTREE_ERROR);
-	xfs_perag_put(pag);
 	return error;
 }
 
@@ -1945,7 +1935,6 @@  xfs_difree_inobt(
 {
 	struct xfs_agi			*agi = agbp->b_addr;
 	xfs_agnumber_t			agno = be32_to_cpu(agi->agi_seqno);
-	struct xfs_perag		*pag;
 	struct xfs_btree_cur		*cur;
 	struct xfs_inobt_rec_incore	rec;
 	int				ilen;
@@ -2007,6 +1996,8 @@  xfs_difree_inobt(
 	if (!(mp->m_flags & XFS_MOUNT_IKEEP) &&
 	    rec.ir_free == XFS_INOBT_ALL_FREE &&
 	    mp->m_sb.sb_inopblock <= XFS_INODES_PER_CHUNK) {
+		struct xfs_perag	*pag = agbp->b_pag;
+
 		xic->deleted = true;
 		xic->first_ino = XFS_AGINO_TO_INO(mp, agno, rec.ir_startino);
 		xic->alloc = xfs_inobt_irec_to_allocmask(&rec);
@@ -2020,10 +2011,8 @@  xfs_difree_inobt(
 		be32_add_cpu(&agi->agi_count, -ilen);
 		be32_add_cpu(&agi->agi_freecount, -(ilen - 1));
 		xfs_ialloc_log_agi(tp, agbp, XFS_AGI_COUNT | XFS_AGI_FREECOUNT);
-		pag = xfs_perag_get(mp, agno);
 		pag->pagi_freecount -= ilen - 1;
 		pag->pagi_count -= ilen;
-		xfs_perag_put(pag);
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_ICOUNT, -ilen);
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -(ilen - 1));
 
@@ -2049,9 +2038,7 @@  xfs_difree_inobt(
 		 */
 		be32_add_cpu(&agi->agi_freecount, 1);
 		xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
-		pag = xfs_perag_get(mp, agno);
-		pag->pagi_freecount++;
-		xfs_perag_put(pag);
+		agbp->b_pag->pagi_freecount++;
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, 1);
 	}
 
@@ -2661,7 +2648,7 @@  xfs_ialloc_read_agi(
 		return error;
 
 	agi = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
 	if (!pag->pagi_init) {
 		pag->pagi_freecount = be32_to_cpu(agi->agi_freecount);
 		pag->pagi_count = be32_to_cpu(agi->agi_count);
@@ -2674,7 +2661,6 @@  xfs_ialloc_read_agi(
 	 */
 	ASSERT(pag->pagi_freecount == be32_to_cpu(agi->agi_freecount) ||
 		XFS_FORCED_SHUTDOWN(mp));
-	xfs_perag_put(pag);
 	return 0;
 }
 
diff --git a/fs/xfs/libxfs/xfs_refcount_btree.c b/fs/xfs/libxfs/xfs_refcount_btree.c
index 7fd6044a4f78..f445a8e2d04e 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -37,15 +37,14 @@  xfs_refcountbt_set_root(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
-	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
-	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
+	struct xfs_perag	*pag = agbp->b_pag;
 
 	ASSERT(ptr->s != 0);
+	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
 
 	agf->agf_refcount_root = ptr->s;
 	be32_add_cpu(&agf->agf_refcount_level, inc);
 	pag->pagf_refcount_level += inc;
-	xfs_perag_put(pag);
 
 	xfs_alloc_log_agf(cur->bc_tp, agbp,
 			XFS_AGF_REFCOUNT_ROOT | XFS_AGF_REFCOUNT_LEVEL);
diff --git a/fs/xfs/libxfs/xfs_rmap_btree.c b/fs/xfs/libxfs/xfs_rmap_btree.c
index b7c05314d07c..959d9df1964e 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -63,16 +63,15 @@  xfs_rmapbt_set_root(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
-	xfs_agnumber_t		seqno = be32_to_cpu(agf->agf_seqno);
 	int			btnum = cur->bc_btnum;
-	struct xfs_perag	*pag = xfs_perag_get(cur->bc_mp, seqno);
+	struct xfs_perag	*pag = agbp->b_pag;
 
 	ASSERT(ptr->s != 0);
+	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
 
 	agf->agf_roots[btnum] = ptr->s;
 	be32_add_cpu(&agf->agf_levels[btnum], inc);
 	pag->pagf_levels[btnum] += inc;
-	xfs_perag_put(pag);
 
 	xfs_alloc_log_agf(cur->bc_tp, agbp, XFS_AGF_ROOTS | XFS_AGF_LEVELS);
 }
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 64f5f9a440ae..d0e7db7a2c32 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2248,7 +2248,6 @@  xfs_iunlink(
 	}
 
 	if (next_agino != NULLAGINO) {
-		struct xfs_perag	*pag;
 		xfs_agino_t		old_agino;
 
 		/*
@@ -2265,9 +2264,7 @@  xfs_iunlink(
 		 * agino has been unlinked, add a backref from the next inode
 		 * back to agino.
 		 */
-		pag = xfs_perag_get(mp, agno);
-		error = xfs_iunlink_add_backref(pag, agino, next_agino);
-		xfs_perag_put(pag);
+		error = xfs_iunlink_add_backref(agibp->b_pag, agino, next_agino);
 		if (error)
 			return error;
 	}
@@ -2403,7 +2400,6 @@  xfs_iunlink_remove(
 	struct xfs_buf		*agibp;
 	struct xfs_buf		*last_ibp;
 	struct xfs_dinode	*last_dip = NULL;
-	struct xfs_perag	*pag = NULL;
 	xfs_agnumber_t		agno = XFS_INO_TO_AGNO(mp, ip->i_ino);
 	xfs_agino_t		agino = XFS_INO_TO_AGINO(mp, ip->i_ino);
 	xfs_agino_t		next_agino;
@@ -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);
 }
 
 /*