Re: [PATCH 1/8] fs: protect inode->i_state with inode->i_lock

[Al Viro <viro@ZenIV.linux.org.uk>]

On Fri, Oct 29, 2010 at 02:23:33PM +1100, Dave Chinner wrote:
> @@ -293,9 +293,11 @@ static void inode_wait_for_writeback(struct inode *inode)
>  
>  	wqh = bit_waitqueue(&inode->i_state, __I_SYNC);
>  	 while (inode->i_state & I_SYNC) {
> +		spin_unlock(&inode->i_lock);

Minor annoyance, but...  Let's replace spaces with tab in that while.

> +		spin_unlock(&inode->i_lock);
> +
>  		pages_skipped = wbc->pages_skipped;
>  		writeback_single_inode(inode, wbc);

	Might make sense to lift locking i_lock into callers of
writeback_single_inode() (it has to grab the damn thing as soon as it's
called) and collapse it with spin_unlock() here.  Separate patch.

> +		spin_unlock(&inode->i_lock);
>  		/*
>  		 * If the inode was already on b_dirty/b_io/b_more_io, don't
>  		 * reposition it (that would break b_dirty time-ordering).

I'm not sure there's any point in dropping it here, actually.

> +	spin_lock(&inode->i_lock);
>  	inode->i_state = I_FREEING | I_CLEAR;
> +	spin_unlock(&inode->i_lock);

_Probably_ not needed here; we already had I_FREEING set by the time
we'd called that and no other thread will modify ->i_state of that
inode at that point.  Check for I_CLEAR will be later in the same
thread, so no barriers are needed.  Separate patch.

> @@ -552,8 +568,6 @@ int invalidate_inodes(struct super_block *sb)
>  		 */
>  		list_move(&inode->i_lru, &dispose);
>  		list_del_init(&inode->i_wb_list);
> -		if (!(inode->i_state & (I_DIRTY | I_SYNC)))
> -			percpu_counter_dec(&nr_inodes_unused);
>  	}
>  	spin_unlock(&inode_lock);

Ho-hum...

I'm not sure we need that list_del_init() here, actually, seeing that
I_FREEING is already set...  For later patch, anyway.

> @@ -917,10 +946,12 @@ static struct inode *get_new_inode_fast(struct super_block *sb,
>  		/* We released the lock, so.. */
>  		old = find_inode_fast(sb, head, ino);
>  		if (!old) {
> +			spin_lock(&inode->i_lock);
>  			inode->i_ino = ino;
> +			inode->i_state = I_NEW;
> +			spin_unlock(&inode->i_lock);

Almost certainly not needed; nobody can find this inode at that point.

> + * wake_up_bit(&inode->i_state, __I_NEW) after removing from the hash list
> + * will DTRT.

Add that i_lock is not regained.