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

[David Chinner <dgc@sgi.com>]

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,

Dave.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group