Re: [PATCH 16/21] fs: Protect inode->i_state with the inode->i_lock

[Al Viro]

On Thu, Oct 21, 2010 at 11:49:41AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
> 
> We currently protect the per-inode state flags with the inode_lock.
> Using a global lock to protect per-object state is overkill when we
> coul duse a per-inode lock to protect the state.  Use the
> inode->i_lock for this, and wrap all the state changes and checks
> with the inode->i_lock.

> @@ -1424,22 +1449,30 @@ static void iput_final(struct inode *inode)
>  	if (!drop) {
>  		if (sb->s_flags & MS_ACTIVE) {
>  			inode->i_state |= I_REFERENCED;
> -			if (!(inode->i_state & (I_DIRTY|I_SYNC)))
> +			if (!(inode->i_state & (I_DIRTY|I_SYNC)) &&
> +			    list_empty(&inode->i_lru)) {
> +				spin_unlock(&inode->i_lock);
>  				inode_lru_list_add(inode);
> +				return;

Sorry, that's broken.  Leaving aside leaking inode_lock here (you remove
taking it later anyway), this is racy.

Look: inode is hashed.  It's alive and well.  It just has no references outside
of the lists.  Right?  Now, what's to stop another CPU from
	a) looking it up in icache
	b) doing unlink()
	c) dropping the last reference
	d) freeing the sucker
... just as you are about to call inode_lru_list_add() here?

For paths in iput() where we do set I_FREEING/I_WILL_FREE it's perfectly
fine to drop all locks once that's done.  Inode is ours, nobody will pick
it and we are free to do as we wish.

For the path that leaves the inode alive and hashed - sorry, can't do.
AFAICS, unlike hash, wb and sb locks, lru lock should nest *inside*
->i_lock.  And yes, it means trylock in prune_icache(), with "put it in
the end of the list for one more spin" if we fail.  In that case it's
really cleaner that way.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/