[PATCH] xfs: i_state of inode is changed after the inode is freed

[PATCH] xfs: i_state of inode is changed after the inode is freed

[Masayuki Saito]

Hi,

I found the case that i_state of the inode was changed when 
the inode was freed in xfs filesystem.  It's as follows to be 
concrete.

(1) xfs_fs_destroy function is called.
(2) after (1), i_state of the inode is changed within
    __mark_inode_dirty function.

In addition to the case of the above, I confirm the case that
i_state of the inode is changed after xfs_inode is reclaimed.
It occurs when xfs_inode reclaim transaction is running with
the transaction mentioned above in parallel. 

My machine has 32way CPUs(IA-64).  The kernel is linux-2.6.17.1.

I think that the cause of the case is that xfs_inode is not
locked.  For this reason, these two(or three) transactions
can be running in parallel.
Here is the typical pattern.  (transaction A runs while transaction
B is running)

generic_delete_inode  xfs_iunpin
---------------------------------------------------------------------------
                      if (!(ip->i_flags & (XFS_IRECLAIM|XFS_IRECLAIMABLE)))
                      +-vnode_t *vp = XFS_ITOV_NULL(ip)
                      ==================(transaction B-Start)==============
                      +-if (vp)
                        +-struct inode    *inode = vn_to_inode(vp)
                        +-if (!(inode->i_state & I_NEW))
                          | +-mark_inode_dirty_sync(inode)
                          |   +-__mark_inode_dirty(inode, I_DIRTY_SYNC)
                                             :



==============(transaction A)=================
(XFS_IRECLAIMABLE or XFS_IRECLAIM set)
(I_CLEAR set)
+-destroy_inode
  +-security_inode_free
    +-inode->i_sb->s_op->destroy_inode
      +-xfs_fs_destroy_inode
        +-kmem_zone_free
==============================================



                                             :
                          |     +-spin_lock(&inode_lock)
                          =================(transaction B-End)=============
                          |     +-inode->i_state |= flags(I_DIRTY_SYNC set)
---------------------------------------------------------------------------

I fixed this problem with adding spinlock for appropriate places.
The patch is against 2.6.17.1.  I confirmed with this patch that the 
inode of i_state is not changed after the inode is freed.

Please comments.

Signed-off-by: Masayuki Saito <m-saito@tnes.nec.co.jp>
---

--- linux-2.6.17.1/fs/xfs/xfs_inode.h.orig	2006-06-29 16:43:21.783055861 +0900
+++ linux-2.6.17.1/fs/xfs/xfs_inode.h	2006-06-29 16:44:44.720968782 +0900
@@ -267,6 +267,7 @@ typedef struct xfs_inode {
 	sema_t			i_flock;	/* inode flush lock */
 	atomic_t		i_pincount;	/* inode pin count */
 	wait_queue_head_t	i_ipin_wait;	/* inode pinning wait queue */
+	spinlock_t              i_vlock;        /* inode lock when inode link/unlink vnode */
 #ifdef HAVE_REFCACHE
 	struct xfs_inode	**i_refcache;	/* ptr to entry in ref cache */
 	struct xfs_inode	*i_release;	/* inode to unref */
--- linux-2.6.17.1/fs/xfs/xfs_inode.c.orig	2006-06-29 16:45:00.297510902 +0900
+++ linux-2.6.17.1/fs/xfs/xfs_inode.c	2006-06-29 17:54:41.929675716 +0900
@@ -861,6 +861,7 @@ xfs_iread(
 	ip = kmem_zone_zalloc(xfs_inode_zone, KM_SLEEP);
 	ip->i_ino = ino;
 	ip->i_mount = mp;
+	spin_lock_init(&ip->i_vlock);
 
 	/*
 	 * Get pointer's to the on-disk inode and the buffer containing it.
@@ -2744,6 +2745,7 @@ xfs_iunpin(
 		 * call as the inode reclaim may be blocked waiting for
 		 * the inode to become unpinned.
 		 */
+		spin_lock(&ip->i_vlock);
 		if (!(ip->i_flags & (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
 			vnode_t	*vp = XFS_ITOV_NULL(ip);
 
@@ -2755,6 +2757,8 @@ xfs_iunpin(
 					mark_inode_dirty_sync(inode);
 			}
 		}
+		spin_unlock(&ip->i_vlock);		
+
 		wake_up(&ip->i_ipin_wait);
 	}
 }
--- linux-2.6.17.1/fs/xfs/linux-2.6/xfs_super.c.orig	2006-06-29 16:49:56.987695062 +0900
+++ linux-2.6.17.1/fs/xfs/linux-2.6/xfs_super.c	2006-06-29 17:55:25.518797628 +0900
@@ -213,11 +213,13 @@ xfs_initialize_vnode(
 	xfs_inode_t		*ip = XFS_BHVTOI(inode_bhv);
 	struct inode		*inode = vn_to_inode(vp);
 
+	spin_lock(&ip->i_vlock);
 	if (!inode_bhv->bd_vobj) {
 		vp->v_vfsp = bhvtovfs(bdp);
 		bhv_desc_init(inode_bhv, ip, vp, &xfs_vnodeops);
 		bhv_insert(VN_BHV_HEAD(vp), inode_bhv);
 	}
+	spin_unlock(&ip->i_vlock);
 
 	/*
 	 * We need to set the ops vectors, and unlock the inode, but if
--- linux-2.6.17.1/fs/xfs/xfs_vnodeops.c.orig	2006-06-29 16:52:06.513256745 +0900
+++ linux-2.6.17.1/fs/xfs/xfs_vnodeops.c	2006-06-29 16:54:56.107495843 +0900
@@ -3834,9 +3834,13 @@ xfs_reclaim(
 
 		/* Protect sync from us */
 		XFS_MOUNT_ILOCK(mp);
-		vn_bhv_remove(VN_BHV_HEAD(vp), XFS_ITOBHV(ip));
-		list_add_tail(&ip->i_reclaim, &mp->m_del_inodes);
-		ip->i_flags |= XFS_IRECLAIMABLE;
+		spin_lock(&ip->i_vlock);
+		if (!(ip->i_flags & XFS_IRECLAIMABLE)) {
+			vn_bhv_remove(VN_BHV_HEAD(vp), XFS_ITOBHV(ip));
+			list_add_tail(&ip->i_reclaim, &mp->m_del_inodes);
+			ip->i_flags |= XFS_IRECLAIMABLE;
+		}
+		spin_unlock(&ip->i_vlock);
 		XFS_MOUNT_IUNLOCK(mp);
 	}
 	return 0;
--- linux-2.6.17.1/fs/xfs/xfs_iget.c.orig	2006-06-29 16:55:45.379720997 +0900
+++ linux-2.6.17.1/fs/xfs/xfs_iget.c	2006-06-29 16:56:51.574275914 +0900
@@ -675,10 +675,12 @@ xfs_ireclaim(xfs_inode_t *ip)
 	/*
 	 * Pull our behavior descriptor from the vnode chain.
 	 */
+	spin_lock(&ip->i_vlock);
 	vp = XFS_ITOV_NULL(ip);
 	if (vp) {
 		vn_bhv_remove(VN_BHV_HEAD(vp), XFS_ITOBHV(ip));
 	}
+	spin_unlock(&ip->i_vlock);
 
 	/*
 	 * Free all memory associated with the inode.
--- linux-2.6.17.1/fs/xfs/xfs_itable.c.orig	2006-06-29 16:58:48.376845205 +0900
+++ linux-2.6.17.1/fs/xfs/xfs_itable.c	2006-06-29 16:59:49.857144001 +0900
@@ -559,6 +559,8 @@ xfs_bulkstat(
 								      KM_SLEEP);
 						ip->i_ino = ino;
 						ip->i_mount = mp;
+						spin_lock_init(&ip->i_vlock);
+
 						if (bp)
 							xfs_buf_relse(bp);
 						error = xfs_itobp(mp, NULL, ip,

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[David Chinner]

On Tue, Jul 04, 2006 at 09:52:56PM +0900, Masayuki Saito wrote:
> Hi,
> 
> I found the case that i_state of the inode was changed when 
> the inode was freed in xfs filesystem.  It's as follows to be 
> concrete.
> 
> (1) xfs_fs_destroy function is called.
> (2) after (1), i_state of the inode is changed within
>     __mark_inode_dirty function.

You'd be talking about xfs_iunpin(), wouldn't you ;)

> In addition to the case of the above, I confirm the case that
> i_state of the inode is changed after xfs_inode is reclaimed.
> It occurs when xfs_inode reclaim transaction is running with
> the transaction mentioned above in parallel. 

Well, it occurs because the xfs_inode_t has a different life cycle
to the linux inode (inherited from Irix). Basically, we can still be
doing transactions on the xfs_inode_t whilst the linux inode is
being freed or has been freed.  Indeed, there was a long standing
use-after-free in this code, as fixed here:

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=58829e490ee805f1c8b3009abc90e2a1a7a0d278

Initially I thought this was all that was necessary to fix the problem.

The problem you are seeing is a fast log transaction completion
(NVRAM in front of your disks, perhaps?), and so the transaction
completion is occurring before the linux inode has been completely
freed. This is the condition the above fix does not handle.....

> I think that the cause of the case is that xfs_inode is not
> locked.  For this reason, these two(or three) transactions
> can be running in parallel.

The inode is locked while the transaction is being built and
unlocked when the transaction is committed to the incore log buffer.
The inode is pinned during the transaction commit, so we can have
multiple _committed_ transactions in flight on the one inode at the
same time, but we only allow an inode to be part of a single
uncommitted transaction at a time.

FYI, see xfs_trans_iget(), xfs_trans_ijoin(), etc for an
explanation of inode transaction locking rules.

> Here is the typical pattern.  (transaction A runs while transaction
> B is running)
> 
> generic_delete_inode  xfs_iunpin
> ---------------------------------------------------------------------------
>                       if (!(ip->i_flags & (XFS_IRECLAIM|XFS_IRECLAIMABLE)))
>                       +-vnode_t *vp = XFS_ITOV_NULL(ip)
>                       ==================(transaction B-Start)==============
>                       +-if (vp)
>                         +-struct inode    *inode = vn_to_inode(vp)
>                         +-if (!(inode->i_state & I_NEW))


This check fails to detect the fact the inode is in the process
of being freed.

http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=714250879ea61cdb1a39bb96fe9d934ee0c669a2

This fixed the reproducable test case I had for the problem.
Can you see if it fixes your problem as well?

Cheers,

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

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Masayuki Saito]

Thank you for comments.

>You'd be talking about xfs_iunpin(), wouldn't you ;)
Yes, of course.

>http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=714250879ea61cdb1a39bb96fe9d934ee0c669a2
>
>This fixed the reproducable test case I had for the problem.
>Can you see if it fixes your problem as well?
We applied the above TAKE to linux-2.6.17.1 and tested it.
However, we confirm the case that i_state of the inode was changed
when the inode was freed in xfs filesystem.

We think that the TAKE reduces the occurrence only.
And we think that our patch fixes the problem.

Could you review our patch again?

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Nathan Scott]

On Fri, Jul 07, 2006 at 09:41:31PM +0900, Masayuki Saito wrote:
> Thank you for comments.
> 
> >You'd be talking about xfs_iunpin(), wouldn't you ;)
> Yes, of course.
> 
> >http://kernel.org/git/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=714250879ea61cdb1a39bb96fe9d934ee0c669a2
> >
> >This fixed the reproducable test case I had for the problem.
> >Can you see if it fixes your problem as well?
> We applied the above TAKE to linux-2.6.17.1 and tested it.
> However, we confirm the case that i_state of the inode was changed
> when the inode was freed in xfs filesystem.
> 
> We think that the TAKE reduces the occurrence only.
> And we think that our patch fixes the problem.
> 
> Could you review our patch again?

I'll leave it to Dave to comment more later (he's travelling at the
moment), since he's had his head deep in this area of the code most
recently - but my first thoughts on your patch are that its solving
the problem incorrectly.  We should not be in the destroy_inode code
if the inode reference counting is correct everywhere - I would have
expected the fix to be a get/put style change, rather than adding an
inode lock and new lock/unlock semantics around an individual field;
... and if that cannot be done to fix this (eh?), then some comments
as to why refcounting didn't solve the problem here.

cheers.

-- 
Nathan

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Masayuki Saito]

Hi, Nathan.

>I'll leave it to Dave to comment more later (he's travelling at the
>moment), since he's had his head deep in this area of the code most
>recently - but my first thoughts on your patch are that its solving
>the problem incorrectly.  We should not be in the destroy_inode code
>if the inode reference counting is correct everywhere - I would have
>expected the fix to be a get/put style change, rather than adding an
>inode lock and new lock/unlock semantics around an individual field;
>... and if that cannot be done to fix this (eh?), then some comments
>as to why refcounting didn't solve the problem here.

On the basis of the above, I consider the get/put style fix which use
i_count.

This problem is that i_state of the inode is changed while the inode
is freed in xfs filesystem.  And the cause is that the inode release
and xfs_iunpun() can run in parallel.

To fix this problem, I added a pair of igrab()/iput() before and behind
mark_inode_dirty_sync() at xfs_iunpin().  I think this can change it as
follows.

(1)The case that the inode release transaction runs after xfs_iunpin()
is called.
While mark_inode_dirty_sync() is running, igrab() promises that the
inode is alive.

(2)The case that xfs_iunpin() is called after iput() in the inode
release transaction is called(i_count is 0).
mark_inode_dirty_sync() is not called because the igrab() can not get
the inode.

I have made the following patch, but it is not yet tested.
I would like to hear your comment, first.

Signed-off-by: Masayuki Saito <m-saito@tnes.nec.co.jp>
---

--- linux-2.6.17.4/fs/xfs/xfs_inode.c.orig	2006-07-14 09:44:44.187844139 +0900
+++ linux-2.6.17.4/fs/xfs/xfs_inode.c	2006-07-14 09:58:26.398486290 +0900
@@ -2751,8 +2751,14 @@ xfs_iunpin(
 			if (vp) {
 				struct inode	*inode = vn_to_inode(vp);
 
-				if (!(inode->i_state & I_NEW))
-					mark_inode_dirty_sync(inode);
+				if (!(inode->i_state & 
+						(I_NEW|I_FREEING|I_CLEAR))) {
+					inode = igrab(inode);
+					if (inode != NULL) {
+						mark_inode_dirty_sync(inode);
+						iput(inode);
+					}
+				}
 			}
 		}
 		wake_up(&ip->i_ipin_wait);

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Nathan Scott]

On Fri, Jul 14, 2006 at 07:25:20PM +0900, Masayuki Saito wrote:
> Hi, Nathan.
> 
> >I'll leave it to Dave to comment more later (he's travelling at the
> >moment), since he's had his head deep in this area of the code most
> >recently - but my first thoughts on your patch are that its solving
> >the problem incorrectly.  We should not be in the destroy_inode code
> >if the inode reference counting is correct everywhere - I would have
> >expected the fix to be a get/put style change, rather than adding an
> >inode lock and new lock/unlock semantics around an individual field;
> >... and if that cannot be done to fix this (eh?), then some comments
> >as to why refcounting didn't solve the problem here.
> 
> On the basis of the above, I consider the get/put style fix which use
> i_count.
> 
> This problem is that i_state of the inode is changed while the inode
> is freed in xfs filesystem.  And the cause is that the inode release
> and xfs_iunpun() can run in parallel.
> 
> To fix this problem, I added a pair of igrab()/iput() before and behind
> mark_inode_dirty_sync() at xfs_iunpin().  I think this can change it as
> follows.
> 
> (1)The case that the inode release transaction runs after xfs_iunpin()
> is called.
> While mark_inode_dirty_sync() is running, igrab() promises that the
> inode is alive.
> 
> (2)The case that xfs_iunpin() is called after iput() in the inode
> release transaction is called(i_count is 0).
> mark_inode_dirty_sync() is not called because the igrab() can not get
> the inode.
> 
> I have made the following patch, but it is not yet tested.
> I would like to hear your comment, first.

If this fixes your test case, then I like the look of it. ;-)

It does seem much simpler and less invasive than the earlier fix
using a spinlock.  I'll run with this in my testing for awhile,
let me know how your own testing goes too, please (I especially
would like to hear if it fixes that reproducible failure case).

thanks!

-- 
Nathan

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[David Chinner]

On Mon, Jul 17, 2006 at 09:05:01PM +1000, Nathan Scott wrote:
> On Fri, Jul 14, 2006 at 07:25:20PM +0900, Masayuki Saito wrote:
> > Hi, Nathan.
> > 
> > >I'll leave it to Dave to comment more later (he's travelling at the
> > >moment), since he's had his head deep in this area of the code most
> > >recently - but my first thoughts on your patch are that its solving
> > >the problem incorrectly.  We should not be in the destroy_inode code
> > >if the inode reference counting is correct everywhere - I would have
> > >expected the fix to be a get/put style change, rather than adding an
> > >inode lock and new lock/unlock semantics around an individual field;
> > >... and if that cannot be done to fix this (eh?), then some comments
> > >as to why refcounting didn't solve the problem here.

Looking at this a bit deeper, I think the reference counting is
correct and the problem is that we are not breaking the link between
the linux inode and the xfs inode atomically w.r.t xfs_iunpin()
usage....

> > On the basis of the above, I consider the get/put style fix which use
> > i_count.
> > 
> > This problem is that i_state of the inode is changed while the inode
> > is freed in xfs filesystem.  And the cause is that the inode release
> > and xfs_iunpun() can run in parallel.
> > 
> > To fix this problem, I added a pair of igrab()/iput() before and behind
> > mark_inode_dirty_sync() at xfs_iunpin().  I think this can change it as
> > follows.
> > 
> > (1)The case that the inode release transaction runs after xfs_iunpin()
> > is called.
> > While mark_inode_dirty_sync() is running, igrab() promises that the
> > inode is alive.
> > 
> > (2)The case that xfs_iunpin() is called after iput() in the inode
> > release transaction is called(i_count is 0).
> > mark_inode_dirty_sync() is not called because the igrab() can not get
> > the inode.
> > 
> > I have made the following patch, but it is not yet tested.
> > I would like to hear your comment, first.
> 
> If this fixes your test case, then I like the look of it. ;-)

I don't think it fixes the problem because igrab() fails to handle
the case we are hitting where I_CLEAR is set on the inode when we
mark it dirty. There's nothing in this patch preventing us from
sleeping after the !(I_NEW|I_FREEING|I_CLEAR) check is done and then
racing with generic_drop_inode() before the igrab() can take a
reference on the inode.

ATM, I think the real problem is the use of the XFS_IRECLAIMABLE
flag and the locking involved. Nathan, the inode hash lock
is used to sync that flag between xfs_iget_core() and
xfs_finish_reclaim(), but no lock is used when setting it or
(now) checking in xfs_iunpin().....

Worse, the i_flags field does not use atomic bitops and
there is no consistent locking protecting i_flags so updates
and reads of this filed can race or even get lost....

Also, I think that xfs_iunpin() must execute atomically w.r.t
xfs_reclaim(), otherwise we cannot ever safely do the checks
we are doing in xfs_iunpin().

> It does seem much simpler and less invasive than the earlier fix
> using a spinlock.  I'll run with this in my testing for awhile,
> let me know how your own testing goes too, please (I especially
> would like to hear if it fixes that reproducible failure case).

I think a fix is going to be much more invasive than just adding
reference as my fixes appear to have only narrowed the race window
and not solved it. The addition of the lock in the original patch
solves the atomic xfs_iunpin()/xfs_reclaim() execution problem,
but it does not solve the problems with the i_flags field. Adding
a new lock may be our only option here.

This will have to wait until after OLS before I get a chance to
look at this further...

Cheers,

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

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Masayuki Saito]

Hi Nathan, David,
Thank you for comments.

>I don't think it fixes the problem because igrab() fails to handle
>the case we are hitting where I_CLEAR is set on the inode when we
>mark it dirty. There's nothing in this patch preventing us from
>sleeping after the !(I_NEW|I_FREEING|I_CLEAR) check is done and then
>racing with generic_drop_inode() before the igrab() can take a
>reference on the inode.

I overlooked the case.  Thank you for your review.


>Worse, the i_flags field does not use atomic bitops and
>there is no consistent locking protecting i_flags so updates
>and reads of this filed can race or even get lost....

I agree it, too.  I think that we should add new spin_lock for
i_flags.


>I think a fix is going to be much more invasive than just adding
>reference as my fixes appear to have only narrowed the race window
>and not solved it. The addition of the lock in the original patch
>solves the atomic xfs_iunpin()/xfs_reclaim() execution problem,
>but it does not solve the problems with the i_flags field. Adding
>a new lock may be our only option here.

I'm considering the solution which fixes two problems([a] i_state of
the inode is changed while the inode is freed in xfs filesystem and
[b] the above i_flags problem)

the solution:
(1)Add new spin_lock(i_flags_lock) for all refernece and change
   places of all i_flags.
(2)Add igrab()/iput() for xfs_iunpin().

It makes sure that mark_inode_dirty_sync() is never called if
xfs_iunpin() runs after I_CLEAR is set.  Because XFS_IRECLAIM
or XFS_IRECLAIMABLE is set/checked within the spin_lock.

And there is the reason that igrab()/iput() is needed even if I add
new spin_lock for xfs_iunpin().  We can prevent the following case
by adding them.
* After passing (I_NEW|I_FREEING|I_CLEAR) check in xfs_iunpin(),
  I_FREEING is set.
* Then mark_inode_dirty_sync() is called and i_state is changed.
* Hit BUG_ON(!(inode->i_state & I_FREEING)) in clear_inode().

If these ideas seem to be correct, I'll make patches for above (1),(2).
Any comment?


(The following is a part of my thinking patch.  Only xfs_iunpin().)

--- linux-2.6.17.6/fs/xfs/xfs_inode.c.orig	2006-07-22 08:07:50.194236144 +0900
+++ linux-2.6.17.6/fs/xfs/xfs_inode.c	2006-07-25 06:07:18.062853045 +0900
@@ -2729,6 +2729,8 @@ void
 xfs_iunpin(
 	xfs_inode_t	*ip)
 {
+	int need_unlock;
+
 	ASSERT(atomic_read(&ip->i_pincount) > 0);
 
 	if (atomic_dec_and_test(&ip->i_pincount)) {
@@ -2744,6 +2746,8 @@ xfs_iunpin(
 		 * call as the inode reclaim may be blocked waiting for
 		 * the inode to become unpinned.
 		 */
+		spin_lock(&ip->i_flags_lock);
+		need_unlock = 1;
 		if (!(ip->i_flags & (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
 			vnode_t	*vp = XFS_ITOV_NULL(ip);
 
@@ -2751,10 +2755,22 @@ xfs_iunpin(
 			if (vp) {
 				struct inode	*inode = vn_to_inode(vp);
 
-				if (!(inode->i_state & I_NEW))
-					mark_inode_dirty_sync(inode);
+				if (!(inode->i_state &
+						(I_NEW|I_FREEING|I_CLEAR))) {
+					inode = igrab(inode);
+					if (inode != NULL) {
+						mark_inode_dirty_sync(inode);
+						spin_unlock(&ip->i_flags_lock);
+						need_unlock = 0;
+						iput(inode);
+					}
+				}
 			}
 		}
+		if (need_unlock) {
+			spin_unlock(&ip->i_flags_lock);
+			need_unlock = 0;
+		}
 		wake_up(&ip->i_ipin_wait);
 	}
 }

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[David Chinner]

Masayuki,

Sorry for taking so long to get back to you - travelling and vacation
left a mountain of email for me to delete :/

On Mon, Jul 24, 2006 at 05:01:33PM +0900, Masayuki Saito wrote:
> >I think a fix is going to be much more invasive than just adding
> >reference as my fixes appear to have only narrowed the race window
> >and not solved it. The addition of the lock in the original patch
> >solves the atomic xfs_iunpin()/xfs_reclaim() execution problem,
> >but it does not solve the problems with the i_flags field. Adding
> >a new lock may be our only option here.
> 
> I'm considering the solution which fixes two problems([a] i_state of
> the inode is changed while the inode is freed in xfs filesystem and
> [b] the above i_flags problem)
> 
> the solution:
> (1)Add new spin_lock(i_flags_lock) for all refernece and change
>    places of all i_flags.
> (2)Add igrab()/iput() for xfs_iunpin().
> 
> It makes sure that mark_inode_dirty_sync() is never called if
> xfs_iunpin() runs after I_CLEAR is set.  Because XFS_IRECLAIM
> or XFS_IRECLAIMABLE is set/checked within the spin_lock.

*nod*

> And there is the reason that igrab()/iput() is needed even if I add
> new spin_lock for xfs_iunpin().  We can prevent the following case
> by adding them.
> * After passing (I_NEW|I_FREEING|I_CLEAR) check in xfs_iunpin(),
>   I_FREEING is set.
> * Then mark_inode_dirty_sync() is called and i_state is changed.
> * Hit BUG_ON(!(inode->i_state & I_FREEING)) in clear_inode().

*nod*

> If these ideas seem to be correct, I'll make patches for above (1),(2).
> Any comment?
> 
> 
> (The following is a part of my thinking patch.  Only xfs_iunpin().)
> 
> --- linux-2.6.17.6/fs/xfs/xfs_inode.c.orig	2006-07-22 08:07:50.194236144 +0900
> +++ linux-2.6.17.6/fs/xfs/xfs_inode.c	2006-07-25 06:07:18.062853045 +0900
> @@ -2729,6 +2729,8 @@ void
>  xfs_iunpin(
>  	xfs_inode_t	*ip)
>  {
> +	int need_unlock;
> +
>  	ASSERT(atomic_read(&ip->i_pincount) > 0);
>  
>  	if (atomic_dec_and_test(&ip->i_pincount)) {
> @@ -2744,6 +2746,8 @@ xfs_iunpin(
>  		 * call as the inode reclaim may be blocked waiting for
>  		 * the inode to become unpinned.
>  		 */
> +		spin_lock(&ip->i_flags_lock);
> +		need_unlock = 1;
>  		if (!(ip->i_flags & (XFS_IRECLAIM|XFS_IRECLAIMABLE))) {
>  			vnode_t	*vp = XFS_ITOV_NULL(ip);
>  
> @@ -2751,10 +2755,22 @@ xfs_iunpin(
>  			if (vp) {
>  				struct inode	*inode = vn_to_inode(vp);
>  
> -				if (!(inode->i_state & I_NEW))
> -					mark_inode_dirty_sync(inode);
> +				if (!(inode->i_state &
> +						(I_NEW|I_FREEING|I_CLEAR))) {
> +					inode = igrab(inode);
> +					if (inode != NULL) {
> +						mark_inode_dirty_sync(inode);
> +						spin_unlock(&ip->i_flags_lock);
> +						need_unlock = 0;
> +						iput(inode);
> +					}
> +				}
>  			}
>  		}
> +		if (need_unlock) {
> +			spin_unlock(&ip->i_flags_lock);
> +			need_unlock = 0;
> +		}
>  		wake_up(&ip->i_ipin_wait);
>  	}
>  }

Hmmm - Idon't think we should iput() before we wake up any pinned waiters.
When we have a waiter on i_ipin_wait (called from xfs_iflush()), we have
a thread sleeping with the inode locked.

If we then call iput() and it drops the last reference, we can call back
into the filesystem and start transactions. Those transactions will need
to lock the inode. Hence I think the above can deadlock when racing against
an inode flush. 

The code should probably read:

	if (dropped last pincount) {
		int need_iput = 0;
		struct inode *inode;

		spin_lock(i_flags_lock)
		if (!reclaimable) {
			if (!vp) {
				if (!(i_state & (NEW|CLEAR))) {
					inode = igrab(inode)
					if (inode) {
						need_iput = 1	
						mark_inode_dirty_sync(inode)
					}
				}
			}
		}
		spin_unlock(i_flags_lock)
		wake_up(&ip->i_ipin_wait)
		if (need_iput)
			iput(inode);
	}

to avoid this possible deadlock.

Cheers,

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

Re: [PATCH] xfs: i_state of inode is changed after the inode is freed

[Masayuki Saito]

Hi Nathan, David,

I had the vacation too.

David Chinner <dgc@sgi.com> wrote:
>Hmmm - Idon't think we should iput() before we wake up any pinned waiters.
>When we have a waiter on i_ipin_wait (called from xfs_iflush()), we have
>a thread sleeping with the inode locked.
>
>If we then call iput() and it drops the last reference, we can call back
>into the filesystem and start transactions. Those transactions will need
>to lock the inode. Hence I think the above can deadlock when racing against
>an inode flush. 
>
>The code should probably read:
>
>	if (dropped last pincount) {
>		int need_iput = 0;
>		struct inode *inode;
>
>		spin_lock(i_flags_lock)
>		if (!reclaimable) {
>			if (!vp) {
>				if (!(i_state & (NEW|CLEAR))) {
>					inode = igrab(inode)
>					if (inode) {
>						need_iput = 1	
>						mark_inode_dirty_sync(inode)
>					}
>				}
>			}
>		}
>		spin_unlock(i_flags_lock)
>		wake_up(&ip->i_ipin_wait)
>		if (need_iput)
>			iput(inode);
>	}
>
>to avoid this possible deadlock.

OK, I see your point.  There is wait_event(in xfs_iunpin_wait) that should
be wake_up'ed(in xfs_iunpin), so deadlock can occur.

I'll update my patch and test it.  Please wait for a few moments.