Re: [patch] Fix xfs_iunpin() sets I_DIRTY_SYNC after clear_inode().

David Chinner
2006-10-18T09:08:08Z

On Wed, Oct 18, 2006 at 12:33:25PM +1000, David Chinner wrote:
> On Tue, Oct 17, 2006 at 12:02:18PM +1000, David Chinner wrote:
> > So, here's another patch that doesn't have the performance problems,
> > but removes the iput/igrab while still (I think) fixing the use
> > after free problem. Can you try this one out, Takenori? I've
> > run it through some stress tests and haven't been able to trigger
> > problems.
>
> I just hit the BUG_ON(vp == NULL) that I put in xfs_iunpin()
> in this patch. The xfs inode had no link to the bhv_vnode, nor
> did it have either XFS_IRECLAIM* flag set, and hence it triggered
> the BUG.

And again. The xfs_iget_core change is valid - there's still a
race in xfs_iunpin (how many of them can we find?):

xfs_iunpin xfs_iget_core
if(atomic_dec_and_test(pincount))
if (vp == NULL)
if(IRECLAIMABLE)
if(pincount)
force+restart
.....
clear IRECLAIMABLE

spin_lock(i_flags_lock)
If (IRECLAIMABLE)
BUG_ON(vp == NULL)

So the solution is this:

---
fs/xfs/xfs_inode.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.c 2006-10-18 11:27:04.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_inode.c 2006-10-18 16:45:12.658102093 +1000
@@ -2738,7 +2738,7 @@ xfs_iunpin(
{
ASSERT(atomic_read(&ip->i_pincount) > 0);

- if (atomic_dec_and_test(&ip->i_pincount)) {
+ if (atomic_dec_and_lock(&ip->i_pincount, &ip->i_flags_lock)) {

/*
* If the inode is currently being reclaimed, the link between
@@ -2757,7 +2757,6 @@ xfs_iunpin(
* unpinned.
*/

- spin_lock(&ip->i_flags_lock);
if (!__xfs_iflags_test(ip, XFS_IRECLAIM|XFS_IRECLAIMABLE)) {
bhv_vnode_t *vp = XFS_ITOV_NULL(ip);
struct inode *inode = NULL;

I'm running stress tests on this now - it it survives until morning
I'll send out a new set of patches for testing...

Cheers,