xfs
[Top] [All Lists]

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

To: "Dave Chinner" <david@xxxxxxxxxxxxx>
Subject: RE: [PATCH 6/6] XFS: Reference count per-ag structures
From: "Alex Elder" <aelder@xxxxxxx>
Date: Wed, 23 Dec 2009 16:12:56 -0600
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <1260857477-2368-7-git-send-email-david@xxxxxxxxxxxxx>
Thread-index: Acp9UAaXDWP21wqiSRO4mNohJ/Nl/AGzKtqA
Thread-topic: [PATCH 6/6] XFS: Reference count per-ag structures
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@xxxxxxxxxxxxx>

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> ---
. . .
> 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;
>  }
. . .

<Prev in Thread] Current Thread [Next in Thread>