Re: [PATCH] Remove I_WILL_FREE

[Jan Kara <jack@suse.cz>]

On Sat 02-01-16 05:18:11, Al Viro wrote:
> [akpm Cc'd]
> On Fri, Jan 01, 2016 at 10:12:54PM -0500, Zhihui Zhang wrote:
> > You are right, I was thinking from the perspective of I_WILL_FREE.
> > 
> > However, for the examples in fs-writeback.c and a few in
> > ext4/btrfs/inode.c, we can argue that they really should check
> > I_WILL_FREE as well. In theory, bad things can happen if they don't
> > because as soon as I_WILL_FREE is set, the inode is going to be
> > evicted. For example, in fs-writeback.c:
> > 
> >  471         spin_lock(&inode->i_lock);
> >  472         if (inode->i_state & (I_WB_SWITCH | I_FREEING) ||
> > 
> >                                         <-- Assume I_WILL_FREE is set
> > at this point.
> > 
> >  473             inode_to_wb(inode) == isw->new_wb) {
> >  474                 spin_unlock(&inode->i_lock);
> >  475                 goto out_free;
> >  476         }
> >  477         inode->i_state |= I_WB_SWITCH;
> >  478         spin_unlock(&inode->i_lock);
> >  479
> >  480         ihold(inode);    <--  This will cause a warning because of i_count.
> 
> Hmm...   That ihold() is actually a lot more recent than original
> introduction of I_WILL_FREE, but looking at the state of the tree
> back when it was originally introduced...  I'm trying to recall
> what made us go for a separate flag, but so far I've got nothing
> definite.  Hell knows - it had been 10 years ago, and I have a gap
> from late 2004 to September 2005 in mailboxes, so those are no help
> either...  I _think_ it got discussed with akpm, maybe he would be
> able to help reconstructing what happened.
> 
> It looks like you are right regarding the current state of the tree, but
> I really wonder if there's something subtle that got missed during
> one of rewrites in those ten years...  OTOH, it's quite possible that there
> had been no good reason for using a separate flag from the very beginning.

Just for record, I don't see a reason for distinguishing between I_FREEING
and I_WILL_FREE either. There could have been some difference back then
when pdflush was still grabbing inode references to writeback inodes and
there was no other explicit writeback barrier when evicting inodes (these
days we have inode_wait_for_writeback() in evict()). But it's all just
guessing.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR