Re: FW: oops in 2.4.25 prune_icache() called from kswapd

[Marcelo Tosatti <marcelo.tosatti@cyclades.com>]

On Mon, Aug 08, 2005 at 11:45:28AM -0500, Srivastava, Rahul wrote:
> Hi All,
> 
> I was just wondering if any of you guys had a chance to validate the
> hypothesis and the proposed fix. 
> 
> Thanks,
> Rahul
> 
> 
> -----Original Message-----
> From: Srivastava, Rahul 
> Sent: Tuesday, August 02, 2005 8:32 AM
> To: 'Marcelo Tosatti'; 'Ernie Petrides'; 'Larry Woodman'
> Subject: RE: oops in 2.4.25 prune_icache() called from kswapd
> 
> 
> Hi,
> 
> Thanks for reviewing the mail. I was thinking whether below changes in
> clear_inode() will close the race window:
> 
> in clear_inode(), change line:
> 
> inode->i_state = I_CLEAR;
> 
> with below piece of code:
> 
> *****
> spin_lock(&inode_lock);
> while (inode->i_state & I_LOCK) {
>  spin_unlock(&inode_lock);
>  __wait_on_inode(inode);
>  spin_lock(&inode_lock);
> }
> inode->i_state = I_CLEAR;
> spin_unlock(&inode_lock);
> *****
> 
> I feel the race is between "__sync_one()" and "iput()/clear_inode()"
> (also suggested by Albert) which is as follows:
> 
> **************** race
> condition*******************************************
> 
> 
> engine 0:
> | 
> calls iput() and lock inode_lock. iput removes the inode from the i_list
> and unlocks              |
> inode_lock
> |
>  
> |   
>  
> |  engine 1:
> 
>  
> |  grab inode_lock and calls __sync_one()
>  
> |
> engine 0:
> |
> calls clear_inode(), get past the call to "wait_on_inode()" which looks
> if I_LOCK is set.         |
> /* From this point onwards clear_inode() and the remainder of iput()
> does not care about       |
> I_LOCK or inode_lock. */
> |    
>  
> |
>  
> | engine 1:
> 
>  
> | Sets I_LOCK.
>  
> |
> engine 0:
> |
> sets i_state = I_CLEAR
> |         
> iput() calls destroy_inode()
> |
> kmem_cache_free() returns the inode to free list of inode cache.
> |
>  
> |  
>  
> | engine 1: 
>  
> | Goes ahead and inserts the freed inode into one of the three possible
> lists.

As stated in private, Larry's fix should catch that in __refile_inode() and
ignore the I_CLEAR inode.

> And we endup in having a corrupted inode on the inode list.
> 
> Your thoughts please.