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

[Gao Xiang <hsiangkao@redhat.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
> > 
>