* [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs @ 2020-06-02 14:52 Gao Xiang 2020-06-03 0:22 ` Darrick J. Wong ` (2 more replies) 0 siblings, 3 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-02 14:52 UTC (permalink / raw) To: linux-xfs; +Cc: 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-02 14:52 [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs Gao Xiang @ 2020-06-03 0:22 ` Darrick J. Wong 2020-06-03 0:44 ` Dave Chinner 2020-06-03 0:49 ` Gao Xiang 2020-06-03 1:27 ` Dave Chinner 2020-06-03 12:11 ` [PATCH v2] " Gao Xiang 2 siblings, 2 replies; 19+ messages in thread From: Darrick J. Wong @ 2020-06-03 0:22 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 0:22 ` Darrick J. Wong @ 2020-06-03 0:44 ` Dave Chinner 2020-06-03 0:48 ` Darrick J. Wong 2020-06-03 0:49 ` Gao Xiang 1 sibling, 1 reply; 19+ messages in thread From: Dave Chinner @ 2020-06-03 0:44 UTC (permalink / raw) To: Darrick J. Wong; +Cc: Gao Xiang, linux-xfs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 0:44 ` Dave Chinner @ 2020-06-03 0:48 ` Darrick J. Wong 0 siblings, 0 replies; 19+ messages in thread From: Darrick J. Wong @ 2020-06-03 0:48 UTC (permalink / raw) To: Dave Chinner; +Cc: Gao Xiang, linux-xfs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 0:22 ` Darrick J. Wong 2020-06-03 0:44 ` Dave Chinner @ 2020-06-03 0:49 ` Gao Xiang 1 sibling, 0 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-03 0:49 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs 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 > > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-02 14:52 [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs Gao Xiang 2020-06-03 0:22 ` Darrick J. Wong @ 2020-06-03 1:27 ` Dave Chinner 2020-06-03 1:40 ` Gao Xiang 2020-06-03 12:11 ` [PATCH v2] " Gao Xiang 2 siblings, 1 reply; 19+ messages in thread From: Dave Chinner @ 2020-06-03 1:27 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 1:27 ` Dave Chinner @ 2020-06-03 1:40 ` Gao Xiang 2020-06-03 3:02 ` Dave Chinner 0 siblings, 1 reply; 19+ messages in thread From: Gao Xiang @ 2020-06-03 1:40 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 1:40 ` Gao Xiang @ 2020-06-03 3:02 ` Dave Chinner 2020-06-03 3:19 ` Gao Xiang 0 siblings, 1 reply; 19+ messages in thread From: Dave Chinner @ 2020-06-03 3:02 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 3:02 ` Dave Chinner @ 2020-06-03 3:19 ` Gao Xiang 0 siblings, 0 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-03 3:19 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-02 14:52 [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs Gao Xiang 2020-06-03 0:22 ` Darrick J. Wong 2020-06-03 1:27 ` Dave Chinner @ 2020-06-03 12:11 ` Gao Xiang 2020-06-04 21:59 ` Dave Chinner 2020-06-05 8:52 ` [PATCH v3] " Gao Xiang 2 siblings, 2 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-03 12:11 UTC (permalink / raw) To: linux-xfs; +Cc: Gao Xiang, Dave Chinner, Darrick J. Wong 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 12:11 ` [PATCH v2] " Gao Xiang @ 2020-06-04 21:59 ` Dave Chinner 2020-06-05 1:44 ` Gao Xiang 2020-06-05 8:52 ` [PATCH v3] " Gao Xiang 1 sibling, 1 reply; 19+ messages in thread From: Dave Chinner @ 2020-06-04 21:59 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Darrick J. Wong 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-04 21:59 ` Dave Chinner @ 2020-06-05 1:44 ` Gao Xiang 0 siblings, 0 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-05 1:44 UTC (permalink / raw) To: Dave Chinner; +Cc: linux-xfs, Dave Chinner, Darrick J. Wong 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-03 12:11 ` [PATCH v2] " Gao Xiang 2020-06-04 21:59 ` Dave Chinner @ 2020-06-05 8:52 ` Gao Xiang 2020-06-05 15:56 ` Darrick J. Wong 2020-07-13 8:53 ` [PATCH v4] " Gao Xiang 1 sibling, 2 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-05 8:52 UTC (permalink / raw) To: linux-xfs; +Cc: Dave Chinner, Darrick J. Wong, 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v3] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-05 8:52 ` [PATCH v3] " Gao Xiang @ 2020-06-05 15:56 ` Darrick J. Wong 2020-06-05 18:30 ` Gao Xiang 2020-07-13 8:53 ` [PATCH v4] " Gao Xiang 1 sibling, 1 reply; 19+ messages in thread From: Darrick J. Wong @ 2020-06-05 15:56 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs, Dave Chinner 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-05 15:56 ` Darrick J. Wong @ 2020-06-05 18:30 ` Gao Xiang 2020-06-05 18:47 ` Gao Xiang 0 siblings, 1 reply; 19+ messages in thread From: Gao Xiang @ 2020-06-05 18:30 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Dave Chinner 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-05 18:30 ` Gao Xiang @ 2020-06-05 18:47 ` Gao Xiang 2020-06-23 10:08 ` Gao Xiang 0 siblings, 1 reply; 19+ messages in thread From: Gao Xiang @ 2020-06-05 18:47 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Dave Chinner 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v3] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-05 18:47 ` Gao Xiang @ 2020-06-23 10:08 ` Gao Xiang 0 siblings, 0 replies; 19+ messages in thread From: Gao Xiang @ 2020-06-23 10:08 UTC (permalink / raw) To: Darrick J. Wong; +Cc: linux-xfs, Dave Chinner 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 > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v4] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-06-05 8:52 ` [PATCH v3] " Gao Xiang 2020-06-05 15:56 ` Darrick J. Wong @ 2020-07-13 8:53 ` Gao Xiang 2020-07-13 16:12 ` Darrick J. Wong 1 sibling, 1 reply; 19+ messages in thread From: Gao Xiang @ 2020-07-13 8:53 UTC (permalink / raw) To: linux-xfs Cc: Dave Chinner, Darrick J. Wong, Brian Foster, Gao Xiang, Dave Chinner 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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v4] xfs: get rid of unnecessary xfs_perag_{get,put} pairs 2020-07-13 8:53 ` [PATCH v4] " Gao Xiang @ 2020-07-13 16:12 ` Darrick J. Wong 0 siblings, 0 replies; 19+ messages in thread From: Darrick J. Wong @ 2020-07-13 16:12 UTC (permalink / raw) To: Gao Xiang; +Cc: linux-xfs, Dave Chinner, Brian Foster, Dave Chinner 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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-07-13 16:15 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-06-02 14:52 [PATCH] xfs: get rid of unnecessary xfs_perag_{get,put} pairs Gao Xiang 2020-06-03 0:22 ` Darrick J. Wong 2020-06-03 0:44 ` Dave Chinner 2020-06-03 0:48 ` Darrick J. Wong 2020-06-03 0:49 ` Gao Xiang 2020-06-03 1:27 ` Dave Chinner 2020-06-03 1:40 ` Gao Xiang 2020-06-03 3:02 ` Dave Chinner 2020-06-03 3:19 ` Gao Xiang 2020-06-03 12:11 ` [PATCH v2] " Gao Xiang 2020-06-04 21:59 ` Dave Chinner 2020-06-05 1:44 ` Gao Xiang 2020-06-05 8:52 ` [PATCH v3] " Gao Xiang 2020-06-05 15:56 ` Darrick J. Wong 2020-06-05 18:30 ` Gao Xiang 2020-06-05 18:47 ` Gao Xiang 2020-06-23 10:08 ` Gao Xiang 2020-07-13 8:53 ` [PATCH v4] " Gao Xiang 2020-07-13 16:12 ` Darrick J. Wong
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.