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

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

[Gao Xiang]

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

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

[Darrick J. Wong]

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
> 

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

[Dave Chinner]

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

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

[Darrick J. Wong]

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

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

[Gao Xiang]

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

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

[Dave Chinner]

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>

-- 
Dave Chinner
david@fromorbit.com

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

[Gao Xiang]

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
> 

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

[Dave Chinner]

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.
-- 
Dave Chinner
david@fromorbit.com

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

[Gao Xiang]

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 v2] xfs: get rid of unnecessary xfs_perag_{get,put} pairs

[Gao Xiang]

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.

Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v1:
 - update the commit message suggested by Dave;
 - fold in all corresponding ASSERTs I made;
 - apply another trivial case xfs_ag_resv_rmapbt_free as well.
   the counterpart xfs_ag_resv_rmapbt_alloc also needed by
   scrub, which seems some way indirect though (I didn't
   trace into that path).

 fs/xfs/libxfs/xfs_ag.c             |  5 ++--
 fs/xfs/libxfs/xfs_ag_resv.h        | 12 ---------
 fs/xfs/libxfs/xfs_alloc.c          | 27 +++++++++-----------
 fs/xfs/libxfs/xfs_alloc_btree.c    | 10 +++-----
 fs/xfs/libxfs/xfs_ialloc.c         | 34 ++++++++++---------------
 fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
 fs/xfs/libxfs/xfs_rmap_btree.c     | 11 ++++----
 fs/xfs/xfs_inode.c                 | 41 ++++++++++--------------------
 8 files changed, 54 insertions(+), 91 deletions(-)

diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
index 9d84007a5c65..4b8c7cb87b84 100644
--- a/fs/xfs/libxfs/xfs_ag.c
+++ b/fs/xfs/libxfs/xfs_ag.c
@@ -563,7 +563,9 @@ 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;
+	ASSERT(pag->pag_agno == agno);
 
 	/* Fill out form. */
 	memset(ageo, 0, sizeof(*ageo));
@@ -583,7 +585,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_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
index f3fd0ee9a7f7..8a8eb4bc48bb 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.h
+++ b/fs/xfs/libxfs/xfs_ag_resv.h
@@ -37,16 +37,4 @@ xfs_ag_resv_rmapbt_alloc(
 	xfs_perag_put(pag);
 }
 
-static inline void
-xfs_ag_resv_rmapbt_free(
-	struct xfs_mount	*mp,
-	xfs_agnumber_t		agno)
-{
-	struct xfs_perag	*pag;
-
-	pag = xfs_perag_get(mp, agno);
-	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
-	xfs_perag_put(pag);
-}
-
 #endif	/* __XFS_AG_RESV_H__ */
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 203e74fa64aa..df89279bbe1b 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,8 @@ xfs_alloc_ag_vextent(
 	}
 
 	if (!args->wasfromfl) {
-		error = xfs_alloc_update_counters(args->tp, args->pag,
-						  args->agbp,
+		ASSERT(args->pag == args->agbp->b_pag);
+		error = xfs_alloc_update_counters(args->tp, args->agbp,
 						  -((long)(args->len)));
 		if (error)
 			return error;
@@ -1887,7 +1886,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 +2165,9 @@ 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);
+	ASSERT(agno == agbp->b_pag->pag_agno);
+	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 +2686,8 @@ 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->pag_agno == be32_to_cpu(agf->agf_seqno));
 	ASSERT(!pag->pagf_agflreset);
 	be32_add_cpu(&agf->agf_flcount, -1);
 	xfs_trans_agflist_delta(tp, -1);
@@ -2701,7 +2699,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 +2794,8 @@ 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->pag_agno == be32_to_cpu(agf->agf_seqno));
 	ASSERT(!pag->pagf_agflreset);
 	be32_add_cpu(&agf->agf_flcount, 1);
 	xfs_trans_agflist_delta(tp, 1);
@@ -2809,7 +2807,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 +3003,8 @@ xfs_alloc_read_agf(
 	ASSERT(!(*bpp)->b_error);
 
 	agf = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
+	ASSERT(pag->pag_agno == agno);
 	if (!pag->pagf_init) {
 		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
 		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
@@ -3034,7 +3032,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..53b156c9ec84 100644
--- a/fs/xfs/libxfs/xfs_ialloc.c
+++ b/fs/xfs/libxfs/xfs_ialloc.c
@@ -888,10 +888,10 @@ 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;
+	ASSERT(pag->pag_agno == agno);
 	pag->pagi_freecount += newlen;
 	pag->pagi_count += newlen;
-	xfs_perag_put(pag);
 	agi->agi_newino = cpu_to_be32(newino);
 
 	/*
@@ -1134,7 +1134,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 +1143,7 @@ xfs_dialloc_ag_inobt(
 	int			i, j;
 	int			searchdistance = 10;
 
-	pag = xfs_perag_get(mp, agno);
-
+	ASSERT(pag->pag_agno == agno);
 	ASSERT(pag->pagi_init);
 	ASSERT(pag->pagi_inodeok);
 	ASSERT(pag->pagi_freecount > 0);
@@ -1384,14 +1383,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 +1584,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 +1595,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 +1661,8 @@ xfs_dialloc_ag(
 	 */
 	be32_add_cpu(&agi->agi_freecount, -1);
 	xfs_ialloc_log_agi(tp, agbp, XFS_AGI_FREECOUNT);
-	pag->pagi_freecount--;
+	ASSERT(agbp->b_pag->pag_agno == agno);
+	agbp->b_pag->pagi_freecount--;
 
 	xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, -1);
 
@@ -1680,7 +1675,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 +1682,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 +1938,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 +1999,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 +2014,9 @@ 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);
+		ASSERT(pag->pag_agno == 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 +2042,8 @@ 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);
+		ASSERT(agbp->b_pag->pag_agno == agno);
+		agbp->b_pag->pagi_freecount++;
 		xfs_trans_mod_sb(tp, XFS_TRANS_SB_IFREE, 1);
 	}
 
@@ -2661,7 +2653,8 @@ xfs_ialloc_read_agi(
 		return error;
 
 	agi = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
+	ASSERT(pag->pag_agno == agno);
 	if (!pag->pagi_init) {
 		pag->pagi_freecount = be32_to_cpu(agi->agi_freecount);
 		pag->pagi_count = be32_to_cpu(agi->agi_count);
@@ -2674,7 +2667,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..be660b2fd422 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);
 }
@@ -123,6 +122,7 @@ xfs_rmapbt_free_block(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
+	struct xfs_perag	*pag;
 	xfs_agblock_t		bno;
 	int			error;
 
@@ -139,8 +139,9 @@ xfs_rmapbt_free_block(
 			      XFS_EXTENT_BUSY_SKIP_DISCARD);
 	xfs_trans_agbtree_delta(cur->bc_tp, -1);
 
-	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_ag.agno);
-
+	pag = cur->bc_ag.agbp->b_pag;
+	ASSERT(pag->pag_agno == cur->bc_ag.agno);
+	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 64f5f9a440ae..bcc5a6eb6a50 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,8 @@ 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);
+		ASSERT(agibp->b_pag->pag_agno == agno);
+		error = xfs_iunlink_add_backref(agibp->b_pag, agino, next_agino);
 		if (error)
 			return error;
 	}
@@ -2403,7 +2401,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 +2444,24 @@ 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,
+		ASSERT(agibp->b_pag->pag_agno == agno);
+		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);
-
+		ASSERT(agibp->b_pag->pag_agno == 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 +2475,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);
 }
 
 /*
-- 
2.18.1

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

[Dave Chinner]

On Wed, Jun 03, 2020 at 08:11:56PM +0800, Gao Xiang wrote:
> 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.
> 
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v1:
>  - update the commit message suggested by Dave;
>  - fold in all corresponding ASSERTs I made;

Ok, I think we had a small misunderstanding there. I was trying to
say the asserts that were in the first patch were fine, but we
didn't really need any more because the new asserts mostly matched
an existing pattern.

I wasn't suggesting that we do this everywhere:

> diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> index 9d84007a5c65..4b8c7cb87b84 100644
> --- a/fs/xfs/libxfs/xfs_ag.c
> +++ b/fs/xfs/libxfs/xfs_ag.c
> @@ -563,7 +563,9 @@ 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;
> +	ASSERT(pag->pag_agno == agno);

.... because we've already checked this in xfs_ialloc_read_agi() a
few lines of code back up the function.

That's the pattern I was refering to - we tend to check
relationships when they are first brought into a context, then we
don't need to check them again in that context.  Hence the asserts
in xfs_ialloc_read_agi() and xfs_alloc_read_agf() effectively cover
all the places where we pull the pag from those buffers, and so
there's no need to validate the correct perag is attached to the
buffer every time we access it....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[Gao Xiang]

On Fri, Jun 05, 2020 at 07:59:17AM +1000, Dave Chinner wrote:
> On Wed, Jun 03, 2020 at 08:11:56PM +0800, Gao Xiang wrote:

...

> 
> Ok, I think we had a small misunderstanding there. I was trying to
> say the asserts that were in the first patch were fine, but we
> didn't really need any more because the new asserts mostly matched
> an existing pattern.
> 
> I wasn't suggesting that we do this everywhere:
> 
> > diff --git a/fs/xfs/libxfs/xfs_ag.c b/fs/xfs/libxfs/xfs_ag.c
> > index 9d84007a5c65..4b8c7cb87b84 100644
> > --- a/fs/xfs/libxfs/xfs_ag.c
> > +++ b/fs/xfs/libxfs/xfs_ag.c
> > @@ -563,7 +563,9 @@ 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;
> > +	ASSERT(pag->pag_agno == agno);
> 
> .... because we've already checked this in xfs_ialloc_read_agi() a
> few lines of code back up the function.
> 
> That's the pattern I was refering to - we tend to check
> relationships when they are first brought into a context, then we
> don't need to check them again in that context.  Hence the asserts
> in xfs_ialloc_read_agi() and xfs_alloc_read_agf() effectively cover
> all the places where we pull the pag from those buffers, and so
> there's no need to validate the correct perag is attached to the
> buffer every time we access it....

Sorry about that, I folded in ASSERTs of my debugging code at that time.
Because that is the straight way to check if somewhere has strange due
to my modification, but some are unnecessary really, I didn't check that,
sorry about that. I will check again and remove unneeded ASSERTs
in the next version.

Thanks,
Gao Xiang

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

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

[Gao Xiang]

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.

Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v2:
  kill unneeded ASSERTs, leaving which first brought
  into a context pointed out by Dave (including callback
  entrances).

 fs/xfs/libxfs/xfs_ag.c             |  4 ++--
 fs/xfs/libxfs/xfs_ag_resv.h        | 12 ----------
 fs/xfs/libxfs/xfs_alloc.c          | 23 +++++++-----------
 fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
 fs/xfs/libxfs/xfs_ialloc.c         | 29 +++++++----------------
 fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
 fs/xfs/libxfs/xfs_rmap_btree.c     | 11 +++++----
 fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
 8 files changed, 41 insertions(+), 91 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);
 	xfs_buf_relse(agf_bp);
 out_agi:
 	xfs_buf_relse(agi_bp);
diff --git a/fs/xfs/libxfs/xfs_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
index f3fd0ee9a7f7..8a8eb4bc48bb 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.h
+++ b/fs/xfs/libxfs/xfs_ag_resv.h
@@ -37,16 +37,4 @@ xfs_ag_resv_rmapbt_alloc(
 	xfs_perag_put(pag);
 }
 
-static inline void
-xfs_ag_resv_rmapbt_free(
-	struct xfs_mount	*mp,
-	xfs_agnumber_t		agno)
-{
-	struct xfs_perag	*pag;
-
-	pag = xfs_perag_get(mp, agno);
-	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
-	xfs_perag_put(pag);
-}
-
 #endif	/* __XFS_AG_RESV_H__ */
diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
index 203e74fa64aa..6c40b0cb4fcc 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,8 @@ xfs_alloc_read_agf(
 	ASSERT(!(*bpp)->b_error);
 
 	agf = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
+	ASSERT(pag->pag_agno == agno);
 	if (!pag->pagf_init) {
 		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
 		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
@@ -3034,7 +3028,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..14aa1faaa081 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,8 @@ xfs_ialloc_read_agi(
 		return error;
 
 	agi = (*bpp)->b_addr;
-	pag = xfs_perag_get(mp, agno);
+	pag = (*bpp)->b_pag;
+	ASSERT(pag->pag_agno == agno);
 	if (!pag->pagi_init) {
 		pag->pagi_freecount = be32_to_cpu(agi->agi_freecount);
 		pag->pagi_count = be32_to_cpu(agi->agi_count);
@@ -2674,7 +2662,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..be660b2fd422 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);
 }
@@ -123,6 +122,7 @@ xfs_rmapbt_free_block(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
+	struct xfs_perag	*pag;
 	xfs_agblock_t		bno;
 	int			error;
 
@@ -139,8 +139,9 @@ xfs_rmapbt_free_block(
 			      XFS_EXTENT_BUSY_SKIP_DISCARD);
 	xfs_trans_agbtree_delta(cur->bc_tp, -1);
 
-	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_ag.agno);
-
+	pag = cur->bc_ag.agbp->b_pag;
+	ASSERT(pag->pag_agno == cur->bc_ag.agno);
+	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
 	return 0;
 }
 
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);
 }
 
 /*
-- 
2.18.1

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

[Darrick J. Wong]

On Fri, Jun 05, 2020 at 04:52:00PM +0800, Gao Xiang wrote:
> 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.
> 
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
> ---
> changes since v2:
>   kill unneeded ASSERTs, leaving which first brought
>   into a context pointed out by Dave (including callback
>   entrances).
> 
>  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
>  fs/xfs/libxfs/xfs_ag_resv.h        | 12 ----------
>  fs/xfs/libxfs/xfs_alloc.c          | 23 +++++++-----------
>  fs/xfs/libxfs/xfs_alloc_btree.c    | 10 ++++----
>  fs/xfs/libxfs/xfs_ialloc.c         | 29 +++++++----------------
>  fs/xfs/libxfs/xfs_refcount_btree.c |  5 ++--
>  fs/xfs/libxfs/xfs_rmap_btree.c     | 11 +++++----
>  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
>  8 files changed, 41 insertions(+), 91 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);
>  	xfs_buf_relse(agf_bp);
>  out_agi:
>  	xfs_buf_relse(agi_bp);
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
> index f3fd0ee9a7f7..8a8eb4bc48bb 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.h
> +++ b/fs/xfs/libxfs/xfs_ag_resv.h
> @@ -37,16 +37,4 @@ xfs_ag_resv_rmapbt_alloc(
>  	xfs_perag_put(pag);
>  }
>  
> -static inline void
> -xfs_ag_resv_rmapbt_free(
> -	struct xfs_mount	*mp,
> -	xfs_agnumber_t		agno)
> -{
> -	struct xfs_perag	*pag;
> -
> -	pag = xfs_perag_get(mp, agno);
> -	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
> -	xfs_perag_put(pag);
> -}
> -
>  #endif	/* __XFS_AG_RESV_H__ */
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index 203e74fa64aa..6c40b0cb4fcc 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,8 @@ xfs_alloc_read_agf(
>  	ASSERT(!(*bpp)->b_error);
>  
>  	agf = (*bpp)->b_addr;
> -	pag = xfs_perag_get(mp, agno);
> +	pag = (*bpp)->b_pag;
> +	ASSERT(pag->pag_agno == agno);

I thought these assertions were all dropped in v3?

Alternately-- if you want to sanity check that b_pag and the buffer
belong to the same ag, why not do that in xfs_buf_find for all the
buffers?

>  	if (!pag->pagf_init) {
>  		pag->pagf_freeblks = be32_to_cpu(agf->agf_freeblks);
>  		pag->pagf_btreeblks = be32_to_cpu(agf->agf_btreeblks);
> @@ -3034,7 +3028,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));

I still see a few of these after-the-fact agno checks throughout the patch.

--D

>  
>  	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..14aa1faaa081 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,8 @@ xfs_ialloc_read_agi(
>  		return error;
>  
>  	agi = (*bpp)->b_addr;
> -	pag = xfs_perag_get(mp, agno);
> +	pag = (*bpp)->b_pag;
> +	ASSERT(pag->pag_agno == agno);
>  	if (!pag->pagi_init) {
>  		pag->pagi_freecount = be32_to_cpu(agi->agi_freecount);
>  		pag->pagi_count = be32_to_cpu(agi->agi_count);
> @@ -2674,7 +2662,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..be660b2fd422 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);
>  }
> @@ -123,6 +122,7 @@ xfs_rmapbt_free_block(
>  {
>  	struct xfs_buf		*agbp = cur->bc_ag.agbp;
>  	struct xfs_agf		*agf = agbp->b_addr;
> +	struct xfs_perag	*pag;
>  	xfs_agblock_t		bno;
>  	int			error;
>  
> @@ -139,8 +139,9 @@ xfs_rmapbt_free_block(
>  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
>  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
>  
> -	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_ag.agno);
> -
> +	pag = cur->bc_ag.agbp->b_pag;
> +	ASSERT(pag->pag_agno == cur->bc_ag.agno);
> +	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
>  	return 0;
>  }
>  
> 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);
>  }
>  
>  /*
> -- 
> 2.18.1
> 

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

[Gao Xiang]

On Fri, Jun 05, 2020 at 08:56:04AM -0700, Darrick J. Wong wrote:
> On Fri, Jun 05, 2020 at 04:52:00PM +0800, Gao Xiang wrote:

...

> > ---
> > changes since v2:
> >   kill unneeded ASSERTs, leaving which first brought
> >   into a context pointed out by Dave (including callback
> >   entrances).

...

> >  
> > @@ -3006,7 +2999,8 @@ xfs_alloc_read_agf(
> >  	ASSERT(!(*bpp)->b_error);
> >  
> >  	agf = (*bpp)->b_addr;
> > -	pag = xfs_perag_get(mp, agno);
> > +	pag = (*bpp)->b_pag;
> > +	ASSERT(pag->pag_agno == agno);
> 
> I thought these assertions were all dropped in v3?

The ASSERT above is in xfs_alloc_read_agf. If I didn't misread what Dave
said, I think this is necessary at least.

> 
> Alternately-- if you want to sanity check that b_pag and the buffer
> belong to the same ag, why not do that in xfs_buf_find for all the
> buffers?

Since that modification doesn't relate to this patch though (since
the purpose of this patch is not add ASSERT to xfs_buf_find).

If in that way, I think we can just kill all these ASSERTs.

> 
> >  	ASSERT(ptr->s != 0);
> > +	ASSERT(pag->pag_agno == be32_to_cpu(agf->agf_seqno));
> 
> I still see a few of these after-the-fact agno checks throughout the patch.

what I wrote above is:

> >   into a context pointed out by Dave (including callback
> >   entrances).

I can leave these xfs_alloc_read_agf, xfs_ialloc_read_agi 2 assertions only
if you want.

Thanks,
Gao Xiang

> 
> --D
> 

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

[Gao Xiang]

On Sat, Jun 06, 2020 at 02:30:37AM +0800, Gao Xiang wrote:

...

> > 
> > Alternately-- if you want to sanity check that b_pag and the buffer
> > belong to the same ag, why not do that in xfs_buf_find for all the
> > buffers?
> 
> Since that modification doesn't relate to this patch though (since
> the purpose of this patch is not add ASSERT to xfs_buf_find).
> 
> If in that way, I think we can just kill all these ASSERTs.

Add some to the previous words:

What I really concern is to avoid introduce some regression from
this trivial patch. That is the original reason why I added these
assertions and tested with stress.

If some of these seems useful to the codebase, I could leave them
in this patch (since these are related to the modification of this
patch). Otherwise, I'd suggest kill them all though as I mentioned
earilier in the reply of v1.

Thanks,
Gao Xiang

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

[Gao Xiang]

Hi,

Friendly ping (just to keep up). What should I do for this patch?
Should I kill all ASSERTs instead (ASSERTs v3 is just v1 + read_{agf,agi}?
I could resend again quickly if needed..

Thanks,
Gao Xiang

On Sat, Jun 06, 2020 at 02:47:32AM +0800, Gao Xiang wrote:
> On Sat, Jun 06, 2020 at 02:30:37AM +0800, Gao Xiang wrote:
> 
> ...
> 
> > > 
> > > Alternately-- if you want to sanity check that b_pag and the buffer
> > > belong to the same ag, why not do that in xfs_buf_find for all the
> > > buffers?
> > 
> > Since that modification doesn't relate to this patch though (since
> > the purpose of this patch is not add ASSERT to xfs_buf_find).
> > 
> > If in that way, I think we can just kill all these ASSERTs.
> 
> Add some to the previous words:
> 
> What I really concern is to avoid introduce some regression from
> this trivial patch. That is the original reason why I added these
> assertions and tested with stress.
> 
> If some of these seems useful to the codebase, I could leave them
> in this patch (since these are related to the modification of this
> patch). Otherwise, I'd suggest kill them all though as I mentioned
> earilier in the reply of v1.
> 
> Thanks,
> Gao Xiang
> 
> 

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

[Gao Xiang]

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.

Cc: Dave Chinner <dchinner@redhat.com>
Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
Signed-off-by: Gao Xiang <hsiangkao@redhat.com>
---
changes since v3:
 - rebased on xfs-5.9-merge-4;
 - kill all added ASSERTs as another version (I'm confused, yet this
   patch is part of my work. So I try to resend it in this way again
   in order to close this task. Hope some feedback since I don't find
   something wrong from this trivial stuff but it seems a good cleanup
   from the diffstat. thanks!)

Thanks,
Gao Xiang

 fs/xfs/libxfs/xfs_ag.c             |  4 ++--
 fs/xfs/libxfs/xfs_ag_resv.h        | 12 ----------
 fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
 fs/xfs/libxfs/xfs_alloc_btree.c    |  8 ++-----
 fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
 fs/xfs/libxfs/xfs_refcount_btree.c |  4 +---
 fs/xfs/libxfs/xfs_rmap_btree.c     |  9 ++++---
 fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
 8 files changed, 34 insertions(+), 91 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);
 	xfs_buf_relse(agf_bp);
 out_agi:
 	xfs_buf_relse(agi_bp);
diff --git a/fs/xfs/libxfs/xfs_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
index f3fd0ee9a7f7..8a8eb4bc48bb 100644
--- a/fs/xfs/libxfs/xfs_ag_resv.h
+++ b/fs/xfs/libxfs/xfs_ag_resv.h
@@ -37,16 +37,4 @@ xfs_ag_resv_rmapbt_alloc(
 	xfs_perag_put(pag);
 }
 
-static inline void
-xfs_ag_resv_rmapbt_free(
-	struct xfs_mount	*mp,
-	xfs_agnumber_t		agno)
-{
-	struct xfs_perag	*pag;
-
-	pag = xfs_perag_get(mp, agno);
-	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
-	xfs_perag_put(pag);
-}
-
 #endif	/* __XFS_AG_RESV_H__ */
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..3d1226aa2eb5 100644
--- a/fs/xfs/libxfs/xfs_alloc_btree.c
+++ b/fs/xfs/libxfs/xfs_alloc_btree.c
@@ -38,16 +38,14 @@ 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);
 
 	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 +113,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 +157,8 @@ xfs_allocbt_update_lastrec(
 	}
 
 	agf->agf_longest = len;
-	pag = xfs_perag_get(cur->bc_mp, seqno);
+	pag = cur->bc_ag.agbp->b_pag;
 	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..c5296c124a4c 100644
--- a/fs/xfs/libxfs/xfs_refcount_btree.c
+++ b/fs/xfs/libxfs/xfs_refcount_btree.c
@@ -37,15 +37,13 @@ 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);
 
 	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..94948a53569f 100644
--- a/fs/xfs/libxfs/xfs_rmap_btree.c
+++ b/fs/xfs/libxfs/xfs_rmap_btree.c
@@ -63,16 +63,14 @@ 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);
 
 	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);
 }
@@ -123,6 +121,7 @@ xfs_rmapbt_free_block(
 {
 	struct xfs_buf		*agbp = cur->bc_ag.agbp;
 	struct xfs_agf		*agf = agbp->b_addr;
+	struct xfs_perag	*pag;
 	xfs_agblock_t		bno;
 	int			error;
 
@@ -139,8 +138,8 @@ xfs_rmapbt_free_block(
 			      XFS_EXTENT_BUSY_SKIP_DISCARD);
 	xfs_trans_agbtree_delta(cur->bc_tp, -1);
 
-	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_ag.agno);
-
+	pag = cur->bc_ag.agbp->b_pag;
+	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
 	return 0;
 }
 
diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
index 5c07bf491d9f..407d6299606d 100644
--- a/fs/xfs/xfs_inode.c
+++ b/fs/xfs/xfs_inode.c
@@ -2265,7 +2265,6 @@ xfs_iunlink(
 	}
 
 	if (next_agino != NULLAGINO) {
-		struct xfs_perag	*pag;
 		xfs_agino_t		old_agino;
 
 		/*
@@ -2282,9 +2281,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;
 	}
@@ -2420,7 +2417,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;
@@ -2464,32 +2460,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,
@@ -2503,15 +2489,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);
 }
 
 /*
-- 
2.18.1

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

[Darrick J. Wong]

On Mon, Jul 13, 2020 at 04:53:34PM +0800, Gao Xiang wrote:
> 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.
> 
> Cc: Dave Chinner <dchinner@redhat.com>
> Cc: "Darrick J. Wong" <darrick.wong@oracle.com>
> Signed-off-by: Gao Xiang <hsiangkao@redhat.com>

Looks like a neat cleanup,

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
> changes since v3:
>  - rebased on xfs-5.9-merge-4;
>  - kill all added ASSERTs as another version (I'm confused, yet this
>    patch is part of my work. So I try to resend it in this way again
>    in order to close this task. Hope some feedback since I don't find
>    something wrong from this trivial stuff but it seems a good cleanup
>    from the diffstat. thanks!)
> 
> Thanks,
> Gao Xiang
> 
>  fs/xfs/libxfs/xfs_ag.c             |  4 ++--
>  fs/xfs/libxfs/xfs_ag_resv.h        | 12 ----------
>  fs/xfs/libxfs/xfs_alloc.c          | 22 ++++++-----------
>  fs/xfs/libxfs/xfs_alloc_btree.c    |  8 ++-----
>  fs/xfs/libxfs/xfs_ialloc.c         | 28 ++++++----------------
>  fs/xfs/libxfs/xfs_refcount_btree.c |  4 +---
>  fs/xfs/libxfs/xfs_rmap_btree.c     |  9 ++++---
>  fs/xfs/xfs_inode.c                 | 38 +++++++++---------------------
>  8 files changed, 34 insertions(+), 91 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);
>  	xfs_buf_relse(agf_bp);
>  out_agi:
>  	xfs_buf_relse(agi_bp);
> diff --git a/fs/xfs/libxfs/xfs_ag_resv.h b/fs/xfs/libxfs/xfs_ag_resv.h
> index f3fd0ee9a7f7..8a8eb4bc48bb 100644
> --- a/fs/xfs/libxfs/xfs_ag_resv.h
> +++ b/fs/xfs/libxfs/xfs_ag_resv.h
> @@ -37,16 +37,4 @@ xfs_ag_resv_rmapbt_alloc(
>  	xfs_perag_put(pag);
>  }
>  
> -static inline void
> -xfs_ag_resv_rmapbt_free(
> -	struct xfs_mount	*mp,
> -	xfs_agnumber_t		agno)
> -{
> -	struct xfs_perag	*pag;
> -
> -	pag = xfs_perag_get(mp, agno);
> -	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
> -	xfs_perag_put(pag);
> -}
> -
>  #endif	/* __XFS_AG_RESV_H__ */
> 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..3d1226aa2eb5 100644
> --- a/fs/xfs/libxfs/xfs_alloc_btree.c
> +++ b/fs/xfs/libxfs/xfs_alloc_btree.c
> @@ -38,16 +38,14 @@ 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);
>  
>  	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 +113,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 +157,8 @@ xfs_allocbt_update_lastrec(
>  	}
>  
>  	agf->agf_longest = len;
> -	pag = xfs_perag_get(cur->bc_mp, seqno);
> +	pag = cur->bc_ag.agbp->b_pag;
>  	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..c5296c124a4c 100644
> --- a/fs/xfs/libxfs/xfs_refcount_btree.c
> +++ b/fs/xfs/libxfs/xfs_refcount_btree.c
> @@ -37,15 +37,13 @@ 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);
>  
>  	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..94948a53569f 100644
> --- a/fs/xfs/libxfs/xfs_rmap_btree.c
> +++ b/fs/xfs/libxfs/xfs_rmap_btree.c
> @@ -63,16 +63,14 @@ 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);
>  
>  	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);
>  }
> @@ -123,6 +121,7 @@ xfs_rmapbt_free_block(
>  {
>  	struct xfs_buf		*agbp = cur->bc_ag.agbp;
>  	struct xfs_agf		*agf = agbp->b_addr;
> +	struct xfs_perag	*pag;
>  	xfs_agblock_t		bno;
>  	int			error;
>  
> @@ -139,8 +138,8 @@ xfs_rmapbt_free_block(
>  			      XFS_EXTENT_BUSY_SKIP_DISCARD);
>  	xfs_trans_agbtree_delta(cur->bc_tp, -1);
>  
> -	xfs_ag_resv_rmapbt_free(cur->bc_mp, cur->bc_ag.agno);
> -
> +	pag = cur->bc_ag.agbp->b_pag;
> +	xfs_ag_resv_free_extent(pag, XFS_AG_RESV_RMAPBT, NULL, 1);
>  	return 0;
>  }
>  
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 5c07bf491d9f..407d6299606d 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2265,7 +2265,6 @@ xfs_iunlink(
>  	}
>  
>  	if (next_agino != NULLAGINO) {
> -		struct xfs_perag	*pag;
>  		xfs_agino_t		old_agino;
>  
>  		/*
> @@ -2282,9 +2281,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;
>  	}
> @@ -2420,7 +2417,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;
> @@ -2464,32 +2460,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,
> @@ -2503,15 +2489,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);
>  }
>  
>  /*
> -- 
> 2.18.1
>