RE: [PATCH 6/6] XFS: Reference count per-ag structures

Alex Elder
2009-12-23T22:15:50Z

Dave Chinner wrote:
> Reference count the per-ag structures to ensure that we keep get/put
> pairs balanced. Assert that the reference counts are zero at unmount
> time to catch leaks. In future, reference counts will enable us to
> safely remove perag structures by allowing us to detect when they
> are no longer in use.

Looks good. Two comments below; I'll fix up if you want.

> Signed-off-by: Dave Chinner <david@fromorbit.com>

Reviewed-by: Alex Elder <aelder@sgi.com>

> ---
. . .
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index 73d61d4..1d00f7f 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -215,6 +215,7 @@ xfs_free_perag(
> for (agno = 0; agno < mp->m_sb.sb_agcount; agno++) {
> spin_lock(&mp->m_perag_lock);
> pag = radix_tree_delete(&mp->m_perag_tree, agno);
> + ASSERT(atomic_read(&pag->pag_ref) == 0);
> spin_unlock(&mp->m_perag_lock);
> ASSERT(pag);

This ASSERT(pag) should be moved up before ASSERT(atomic_read(&pag->pag_ref == 0)

> kmem_free(pag->pagb_list);
. . .
> diff --git a/fs/xfs/xfs_mount.h b/fs/xfs/xfs_mount.h
> index cfa7a5d..16b2212 100644
> --- a/fs/xfs/xfs_mount.h
> +++ b/fs/xfs/xfs_mount.h
. . .
> @@ -393,6 +393,12 @@ xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
>
> spin_lock(&mp->m_perag_lock);
> pag = radix_tree_lookup(&mp->m_perag_tree, agno);
> + if (pag) {
> + ASSERT(atomic_read(&pag->pag_ref) >= 0);
> + /* catch leaks in the positive direction during testing */
> + ASSERT(atomic_read(&pag->pag_ref) < 1000);

This is good; what makes 1000 a reasonable number (why not
100 or 10000?)

> + atomic_inc(&pag->pag_ref);
> + }
> spin_unlock(&mp->m_perag_lock);
> return pag;
> }
. . .