xfs
[Top] [All Lists]

Re: [PATCH 1/2] xfs: reclaim inodes under a write lock

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH 1/2] xfs: reclaim inodes under a write lock
From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Fri, 8 Jan 2010 05:20:42 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <1262819125-27083-2-git-send-email-david@xxxxxxxxxxxxx>
References: <1262819125-27083-1-git-send-email-david@xxxxxxxxxxxxx> <1262819125-27083-2-git-send-email-david@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.19 (2009-01-05)
On Thu, Jan 07, 2010 at 10:05:24AM +1100, Dave Chinner wrote:
> Make the inode tree reclaim walk exclusive to avoid races with
> concurrent sync walkers and lookups. This is a version of
> a patch posted by Christoph Hellwig that avoids all the code
> duplication.

I don't like the write_lock flag very much, but given that the other
option is duplication we might have to live it.

> -     /*
> -      * If we can't get a reference on the inode, it must be in reclaim.
> -      * Leave it for the reclaim code to flush. Also avoid inodes that
> -      * haven't been fully initialised.
> -      */
> +     /* avoid new or reclaimable inodes. Leave for reclaim code to flush */
> +     if (xfs_iflags_test(ip, XFS_INEW | XFS_IRECLAIMABLE | XFS_IRECLAIM)) {
> +             read_unlock(&pag->pag_ici_lock);
> +             return ENOENT;
> +     }
> +
> +     /* If we can't get a reference on the inode, it must be in reclaim. */
>       if (!igrab(inode)) {
>               read_unlock(&pag->pag_ici_lock);
>               return ENOENT;
>       }
>       read_unlock(&pag->pag_ici_lock);
>  
> -     if (is_bad_inode(inode) || xfs_iflags_test(ip, XFS_INEW)) {
> +     if (is_bad_inode(inode)) {
>               IRELE(ip);
>               return ENOENT;

That's an unrelated change and should be a separate patch.

> @@ -791,12 +779,22 @@ xfs_reclaim_inode_now(
>       struct xfs_perag        *pag,
>       int                     flags)
>  {
> +     /*
> +      * The radix tree lock here protects a thread in xfs_iget from racing
> +      * with us starting reclaim on the inode.  Once we have the
> +      * XFS_IRECLAIM flag set it will not touch us.
> +      */
> +     spin_lock(&ip->i_flags_lock);
> +     ASSERT_ALWAYS(__xfs_iflags_test(ip, XFS_IRECLAIMABLE));
> +     if (__xfs_iflags_test(ip, XFS_IRECLAIM)) {
> +             /* ignore as it is already under reclaim */
> +             spin_unlock(&ip->i_flags_lock);
> +             write_unlock(&pag->pag_ici_lock);
>               return 0;
>       }
> +     __xfs_iflags_set(ip, XFS_IRECLAIM);
> +     spin_unlock(&ip->i_flags_lock);
> +     write_unlock(&pag->pag_ici_lock);
>  
>       return xfs_reclaim_inode(ip, flags);

Once you move things around please merge xfs_reclaim_inode_now and
xfs_reclaim_inode into a single function.

And yes, all this currently doesn't apply against the XFS tree or
mainline, but you know that already.

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