Re: [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 27, 2012 at 04:41:48PM +0800, Guo Chao wrote:
> On Wed, Sep 26, 2012 at 10:54:09AM +1000, Dave Chinner wrote:
> > On Tue, Sep 25, 2012 at 04:59:55PM +0800, Guo Chao wrote:
> > > > @@ -1078,8 +1098,7 @@ struct inode *iget_locked(struct super_block *sb, unsigned long ino)
> > > >  		struct inode *old;
> > > > 
> > > >  		spin_lock(&inode_hash_lock);
> > > > -		/* We released the lock, so.. */
> > > > -		old = find_inode_fast(sb, head, ino);
> > > > +		old = find_inode_fast(sb, head, ino, true);
> > > >  		if (!old) {
> > > >  			inode->i_ino = ino;
> > > >  			spin_lock(&inode->i_lock);
> > > 
> > > Emmmm ... couldn't we use memory barrier API instead of irrelevant spin
> > > lock on newly allocated inode to publish I_NEW?
> > 
> > Yes, we could.
> > 
> > However, having multiple synchronisation methods for a single
> > variable that should only be used in certain circumstances is
> > something that is easy to misunderstand and get wrong. Memory
> > barriers are much more subtle and harder to understand than spin
> > locks, and every memory barrier needs to be commented to explain
> > what the barrier is actually protecting against.
> > 
> > In the case where a spin lock is guaranteed to be uncontended and
> > the cache line hot in the CPU cache, it makes no sense to replace
> > the spin lock with a memory barrier, especially when every other
> > place we modify the i_state/i_hash fields we have to wrap them
> > with i_lock....
> > 
> > Simple code is good code - save the complexity for something that
> > needs it.
> > 
> 
> Emmm, I doubt "it's simpler and need no document". 

It is simpler because it follows the documented locking rules. THey
are right at the top of fs/inode.c:

/*
 * Inode locking rules:
 *
 * inode->i_lock protects:
 *   inode->i_state, inode->i_hash, __iget()
.....
 * Lock ordering:
.....
 * inode_hash_lock
 *   inode_sb_list_lock
 *   inode->i_lock
 *
.....

If you think it's simpler to have multiple access and update rules
for the same fields that can only be applied in certain
circumstances and can document it as such, then I look forward to
reviewing the patch. :)

> I bet someday there will be other guys stand out and ask "why take spin 
> lock on a inode which apparently does not subject to any race condition?". 

And we now have a thread to point them at so we don't have to
explain it again. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com