From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marcelo Tosatti <marcelo.tosatti@cyclades.com>
Subject: Re: FW: oops in 2.4.25 prune_icache() called from kswapd
Date: Mon, 8 Aug 2005 23:55:09 -0300
Message-ID: <20050809025509.GO9569@dmt.cnet>
References: <717252EC3E37AE4392E2614EA24E9F2B0D7D0736@txnexc01.americas.cpqcorp.net>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: petrides@redhat.com, lwoodman@redhat.com,
	linux-fsdevel@vger.kernel.org
Return-path: <linux-fsdevel-owner@vger.kernel.org>
Received: from parcelfarce.linux.theplanet.co.uk ([195.92.249.252]:390 "EHLO
	parcelfarce.linux.theplanet.co.uk") by vger.kernel.org with ESMTP
	id S932427AbVHIDAR (ORCPT <rfc822;linux-fsdevel@vger.kernel.org>);
	Mon, 8 Aug 2005 23:00:17 -0400
To: "Srivastava, Rahul" <rahul.srivastava@hp.com>
Content-Disposition: inline
In-Reply-To: <717252EC3E37AE4392E2614EA24E9F2B0D7D0736@txnexc01.americas.cpqcorp.net>
Sender: linux-fsdevel-owner@vger.kernel.org
List-Id: linux-fsdevel.vger.kernel.org

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.