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: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Thu, 10 Dec 2009 18:48:36 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1259734299-20306-7-git-send-email-david@xxxxxxxxxxxxx>
References: <1259734299-20306-1-git-send-email-david@xxxxxxxxxxxxx> <1259734299-20306-7-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Wed, Dec 02, 2009 at 05:11:39PM +1100, 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.
> 
> Signed-off-by: Dave Chinner <david@xxxxxxxxxxxxx>
> ---
>  fs/xfs/xfs_ag.h    |    4 ++--
>  fs/xfs/xfs_alloc.c |    2 +-
>  fs/xfs/xfs_inode.c |    5 ++++-
>  fs/xfs/xfs_mount.c |    1 +
>  fs/xfs/xfs_mount.h |   11 +++++++++--
>  5 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/xfs/xfs_ag.h b/fs/xfs/xfs_ag.h
> index a5d54bf..b384a3c 100644
> --- a/fs/xfs/xfs_ag.h
> +++ b/fs/xfs/xfs_ag.h

> index 2d076e2..d1349b1 100644
> --- a/fs/xfs/xfs_alloc.c
> +++ b/fs/xfs/xfs_alloc.c
> @@ -2545,7 +2545,6 @@ xfs_alloc_vextent(
>                       }
>                       xfs_perag_put(args->pag);
>               }
> -             xfs_perag_put(args->pag);
>               if (bump_rotor || (type == XFS_ALLOCTYPE_ANY_AG)) {
>                       if (args->agno == sagno)
>                               mp->m_agfrotor = (mp->m_agfrotor + 1) %
> @@ -2571,6 +2570,7 @@ xfs_alloc_vextent(
>                       args->len);
>  #endif
>       }
> +     xfs_perag_put(args->pag);
>       return 0;
>  error0:
>       xfs_perag_put(args->pag);

Should be folded into the earlier patches.
>
> diff --git a/fs/xfs/xfs_inode.c b/fs/xfs/xfs_inode.c
> index 44a1168..e6c178e 100644
> --- a/fs/xfs/xfs_inode.c
> +++ b/fs/xfs/xfs_inode.c
> @@ -2729,7 +2729,7 @@ xfs_iflush_cluster(
>       ilist_size = inodes_per_cluster * sizeof(xfs_inode_t *);
>       ilist = kmem_alloc(ilist_size, KM_MAYFAIL|KM_NOFS);
>       if (!ilist)
> -             return 0;
> +             goto out_put;
>  
>       mask = ~(((XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_inodelog)) - 1);
>       first_index = XFS_INO_TO_AGINO(mp, ip->i_ino) & mask;
> @@ -2798,6 +2798,8 @@ xfs_iflush_cluster(
>  out_free:
>       read_unlock(&pag->pag_ici_lock);
>       kmem_free(ilist);
> +out_put:
> +     xfs_perag_put(pag);
>       return 0;
>  
>  
> @@ -2841,6 +2843,7 @@ cluster_corrupt_out:
>        */
>       xfs_iflush_abort(iq);
>       kmem_free(ilist);
> +     xfs_perag_put(pag);
>       return XFS_ERROR(EFSCORRUPTED);

Same here.

>  static inline xfs_perag_t *
>  xfs_perag_get(struct xfs_mount *mp, xfs_agnumber_t agno)
> @@ -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);

Is there any good reason why we should not be able to hit this number?


Patch looks good, although I'm a bit worried about the performance
overhead.

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