In-reply-to: <20100301113031.GA13217@xxxxxxxxxxxxx>
I've temporarily lost my ability to *read* my e-mail, but I
am able to blindly send this off...
> From: Christoph Hellwig <hch@xxxxxxxxxxxxx>
>
> 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@xxxxxxx>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> Reported-by: Patrick Schreurs <patrick@xxxxxxxxxxxxxxxx>
> Tested-by: Patrick Schreurs <patrick@xxxxxxxxxxxxxxxx>
. . .
> 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);
. . .
|