From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Chinner Subject: Re: [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage Date: Thu, 27 Sep 2012 21:51:28 +1000 Message-ID: <20120927115128.GT15236@dastard> References: <20120921224912.GA20960@dastard> <20120924024221.GB7984@yanx> <20120924042343.GC20960@dastard> <20120924061205.GA27244@yanx> <20120924062812.GD20960@dastard> <20120924070852.GA10280@yanx> <20120924082654.GG20960@dastard> <20120925085955.GA15073@yanx> <20120926005409.GG29154@dastard> <20120927084148.GA29769@yanx> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: linux-fsdevel@vger.kernel.org To: Guo Chao Return-path: Received: from ipmail05.adl6.internode.on.net ([150.101.137.143]:20571 "EHLO ipmail05.adl6.internode.on.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750793Ab2I0Lvc (ORCPT ); Thu, 27 Sep 2012 07:51:32 -0400 Content-Disposition: inline In-Reply-To: <20120927084148.GA29769@yanx> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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