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

[Takenori Nagano <t-nagano@ah.jp.nec.com>]

[-- Attachment #1: Type: text/plain, Size: 2645 bytes --]

Hi David,

I'm testing your three patches.
I am not seeing any degradation with your patches.

But I think the patch that I attach to this mail is necessary.
Isn't it?

David Chinner wrote:
> 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.

-- 
+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+
ＮＥＣ コンピュータソフトウェア事業本部
            ＯＳＳ推進センター 技術開発Ｇ

        永野 武則 (Takenori Nagano)

TEL:8-23-57270(MyLine) 042-333-5383(外線)
e-mail:t-nagano@ah.jp.nec.com
+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+

[-- Attachment #2: xfs_idestroy.patch --]
[-- Type: text/plain, Size: 1143 bytes --]

diff -Naru linux-2.6.19-rc1/fs/xfs.orig/xfs_inode.c linux-2.6.19-rc1/fs/xfs/xfs_inode.c
--- linux-2.6.19-rc1/fs/xfs.orig/xfs_inode.c	2006-10-19 01:51:43.020000000 +0900
+++ linux-2.6.19-rc1/fs/xfs/xfs_inode.c	2006-10-19 01:53:47.248000000 +0900
@@ -2713,6 +2713,7 @@
 		       XFS_FORCED_SHUTDOWN(ip->i_mount));
 		xfs_inode_item_destroy(ip);
 	}
+	xfs_iunpin_wait(ip);
 	kmem_zone_free(xfs_inode_zone, ip);
 }
 
@@ -2784,7 +2785,7 @@
  * be subsequently pinned once someone is waiting for it to be
  * unpinned.
  */
-STATIC void
+void
 xfs_iunpin_wait(
 	xfs_inode_t	*ip)
 {
diff -Naru linux-2.6.19-rc1/fs/xfs.orig/xfs_inode.h linux-2.6.19-rc1/fs/xfs/xfs_inode.h
--- linux-2.6.19-rc1/fs/xfs.orig/xfs_inode.h	2006-10-19 01:51:42.980000000 +0900
+++ linux-2.6.19-rc1/fs/xfs/xfs_inode.h	2006-10-19 01:52:17.980000000 +0900
@@ -498,6 +498,7 @@
 void		xfs_iroot_realloc(xfs_inode_t *, int, int);
 void		xfs_ipin(xfs_inode_t *);
 void		xfs_iunpin(xfs_inode_t *);
+void		xfs_iunpin_wait(xfs_inode_t *);
 int		xfs_iextents_copy(xfs_inode_t *, xfs_bmbt_rec_t *, int);
 int		xfs_iflush(xfs_inode_t *, uint);
 void		xfs_iflush_all(struct xfs_mount *);