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

[Christoph Hellwig <hch@infradead.org>]

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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs