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

[Masayuki Saito <m-saito@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxx>
---

--- 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);