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@xxxxxxxxxxxxx
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
