xfs
[Top] [All Lists]

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

To: Takenori Nagano <t-nagano@xxxxxxxxxxxxx>
Subject: Re: [patch] Fix xfs_iunpin() sets I_DIRTY_SYNC after clear_inode().
From: David Chinner <dgc@xxxxxxx>
Date: Wed, 18 Oct 2006 19:07:01 +1000
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20061018023325.GL8394166@xxxxxxxxxxxxxxxxx>
References: <45237CCE.4010007@xxxxxxxxxxxxx> <20061006032617.GC11034@xxxxxxxxxxxxxxxxx> <20061011064357.GN19345@xxxxxxxxxxxxxxxxx> <452E32FF.8010109@xxxxxxxxxxxxx> <20061013014651.GC19345@xxxxxxxxxxxxxxxxx> <452F83BD.8050501@xxxxxxxxxxxxx> <20061017020218.GE8394166@xxxxxxxxxxxxxxxxx> <20061018023325.GL8394166@xxxxxxxxxxxxxxxxx>
Sender: xfs-bounce@xxxxxxxxxxx
User-agent: Mutt/1.4.2.1i
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.
-- 
Dave Chinner
Principal Engineer
SGI Australian Software Group


<Prev in Thread] Current Thread [Next in Thread>