xfs: fix locking for inode cache radix tree tag updates

[Alex Elder]

In-reply-to: <20100301113031.GA13217 at infradead.org>

I've temporarily lost my ability to *read* my e-mail, but I
am able to blindly send this off...

> From: Christoph Hellwig <hch at infradead.org>
> 
> The radix-tree code requires it's users to serialize tag updates against
> other updates to the tree.  While XFS protects tag updates against each
> other it does not serialize them against updates of the tree contents,
> which can lead to tag corruption.  Fix the inode cache to always take
> pag_ici_lock in exclusive mode when updating radix tree tags.

I have one minor comment below, and point out a typo, but otherwise
this looks good.  I'll fix the typo myself.

As I understand it, the point here is that the tags associated
with a value stored in a radix tree are just as much "content"
as the value itself.  Therefore you need to use the same locking
protocol.  So we need to get the write lock to modify a tag value.

My comments require no action on your part, so...

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

> Signed-off-by: Christoph Hellwig <hch at lst.de>
> Reported-by: Patrick Schreurs <patrick at news-service.com>
> Tested-by: Patrick Schreurs <patrick at news-service.com>

. . .

> Index: xfs/fs/xfs/xfs_iget.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iget.c	2010-02-04 17:28:35.000000000 +0100
> +++ xfs/fs/xfs/xfs_iget.c	2010-02-10 15:53:55.504284758 +0100
> @@ -190,13 +190,12 @@ xfs_iget_cache_hit(
>  		trace_xfs_iget_reclaim(ip);
>  
>  		/*
> -		 * We need to set XFS_INEW atomically with clearing the
> -		 * reclaimable tag so that we do have an indicator of the
> -		 * inode still being initialized.
> +		 * We need to set XFS_IRECLAIM to prevent xfs_reclaim_inode
> +		 * from stomping over us while we recycle the inode.  We can't
> +		 * clear the radix tree reclaimable tag yet as it requires
> +		 * pag_ici_lock to be helt exclusive.

Your accent is showing.  I will change "helt" -> "held".

>  		 */
> -		ip->i_flags |= XFS_INEW;
> -		ip->i_flags &= ~XFS_IRECLAIMABLE;
> -		__xfs_inode_clear_reclaim_tag(mp, pag, ip);
> +		ip->i_flags |= XFS_IRECLAIM;

This succeeds at getting xfs_reclaim_inode() to skip over
this one.  I think it's overloading the flag though--at least
the name of the flag (which maybe is ambiguous anyway) doesn't
really reflect what's happening here.  (The result is correct,
however.)

>  
>  		spin_unlock(&ip->i_flags_lock);
>  		read_unlock(&pag->pag_ici_lock);
. . .