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

[Dave Chinner <david@fromorbit.com>]

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