| Message ID | 20200602145238.1512-1-hsiangkao@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: get rid of unnecessary xfs_perag_{get,put} pairs | expand |
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 >
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.
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
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 > > >
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>
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 >
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.
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 >
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); } /*
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(-)