Messages in this thread Patch in this message | | | Subject | Re: [PATCH] xfs: i_state of inode is changed after the inode is freed | From | Masayuki Saito <> | Date | Fri, 14 Jul 2006 19:25:20 +0900 |
| |
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); - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |