xfs
[Top] [All Lists]

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

To: David Chinner <dgc@xxxxxxx>
Subject: Re: [patch] Fix xfs_iunpin() sets I_DIRTY_SYNC after clear_inode().
From: Takenori Nagano <t-nagano@xxxxxxxxxxxxx>
Date: Thu, 19 Oct 2006 11:23:02 +0900
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20061018090701.GU11034@melbourne.sgi.com>
References: <45237CCE.4010007@ah.jp.nec.com> <20061006032617.GC11034@melbourne.sgi.com> <20061011064357.GN19345@melbourne.sgi.com> <452E32FF.8010109@ah.jp.nec.com> <20061013014651.GC19345@melbourne.sgi.com> <452F83BD.8050501@ah.jp.nec.com> <20061017020218.GE8394166@melbourne.sgi.com> <20061018023325.GL8394166@melbourne.sgi.com> <20061018090701.GU11034@melbourne.sgi.com>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Thunderbird 1.5.0.7 (Windows/20060909)
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.

-- 
+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+
NEC コンピュータソフトウェア事業本部
            OSS推進センター 技術開発G

        永野 武則 (Takenori Nagano)

TEL:8-23-57270(MyLine) 042-333-5383(外線)
e-mail:t-nagano@xxxxxxxxxxxxx
+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+=-=+
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 *);
<Prev in Thread] Current Thread [Next in Thread>