Re: [PATCH] Re: another problem with latest code drops

[Dave Chinner <david@fromorbit.com>]

On Mon, Oct 20, 2008 at 04:29:29PM +1100, Dave Chinner wrote:
> On Mon, Oct 20, 2008 at 02:37:42PM +1000, Lachlan McIlroy wrote:
> > Dave Chinner wrote:
> >> On Mon, Oct 20, 2008 at 12:37:13PM +1000, Lachlan McIlroy wrote:
> >>> I also hit this panic where we have taken a reference on an inode
> >>> that has I_CLEAR set.  I suspect we've made it into xfs_iget_cache_hit()
> >>
> >> I don't think there is an iput() in that path.  The only iput() call
> >> should be the IRELE() I added to xfs_iget_cache_miss(). Can you make
> >> sure the compiler is not inlining functions so we can pin-point
> >> where the iput() call is coming from? (i.e. static > STATIC on the
> >> hit/miss functions)
> > Just disassembled xfs_iget() and xfs_iget_cache_miss() has been inlined
> > and we're calling the IRELE() at the end of that function.
> 
> Ok, that makes more sense.
> 
> >>> and found an inode with XFS_IRECLAIMABLE set and since we don't call
> >>> igrab() we don't do the I_CLEAR check.
> >>
> >> In that case, we call inode_init_always() instead which sets the
> >> state to I_NEW and the reference count to 1. In the error case,
> >> the inode will have already been freed and we make
> > We don't set inode->i_state to i_NEW.  We're stuffing XFS_INEW into
> > the XFS inode's i_flags field and not removing the I_CLEAR from the
> > linux inode.  Note that inode_init_always() doesn't touch i_state.
> 
> Yeah, xfs_setup_inode() is what is doing:
> 
> 	inode->i_state = I_NEW|I_LOCK;
> 
> which happens after the cache miss has been processed.
> 
> Ok, so the initialisation code needs to clear іnode->i_state
> during allocation so that iput() does not complain. The i_state
> field is not accessed by anything until the inode is inserted
> into the superblock lists, so it should be safe to zero it
> in xfs_inode_alloc(). I missed that new_inode() returns an
> inode with a zeroed i_state....

On second thoughts, the inode has not been set up properly
by this stage, so we can't really even expect to be able to
call iput() on it. Hmmmm - maybe the only thing we can do
here is call security_inode_free() before xfs_idestroy.

Christoph - any opinions on what the best thing to do is?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com