[Top] [All Lists]

Re: xfs: fix locking for inode cache radix tree tag updates

To: hch@xxxxxx
Subject: Re: xfs: fix locking for inode cache radix tree tag updates
From: Alex Elder <aelder@xxxxxxx>
Date: Mon, 01 Mar 2010 19:06:42 -0600
Cc: xfs@xxxxxxxxxxx
User-agent: Heirloom mailx 12.4 7/29/08
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,

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

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