lkml.org 
[lkml]   [2012]   [Sep]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC v4 Patch 0/4] fs/inode.c: optimization for inode lock usage
On Mon, Sep 24, 2012 at 02:23:43PM +1000, Dave Chinner wrote:
> On Mon, Sep 24, 2012 at 10:42:21AM +0800, Guo Chao wrote:
> > On Sat, Sep 22, 2012 at 08:49:12AM +1000, Dave Chinner wrote:
> >
> > > On Fri, Sep 21, 2012 at 05:31:02PM +0800, Guo Chao wrote:
> > > > This patchset optimizes several places which take the per inode spin lock.
> > > > They have not been fully tested yet, thus they are marked as RFC.
> > >
> > > Inodes are RCU freed. The i_lock spinlock on the i_state field forms
> > > part of the memory barrier that allows the RCU read side to
> > > correctly detect a freed inode during a RCU protected cache lookup
> > > (hash list traversal for the VFS, or a radix tree traversal for XFS).
> > > The i_lock usage around the hahs list operations ensures the hash
> > > list operations are atomic with state changes so that such changes
> > > are correctly detected during RCU-protected traversals...
> > >
> > > IOWs, removing the i_lock from around the i_state transitions and
> > > inode hash insert/remove/traversal operations will cause races in
> > > the RCU lookups and result in incorrectly using freed inodes instead
> > > of failing the lookup and creating a new one.
> > >
> > > So I don't think this is a good idea at all...
> > >
> >
> > Hello, Dave:
> >
> > Thanks for your explanation.
> >
> > Though I can't fully understand it, your concern seems to be that
> > RCU inode lookup will be bothered by this change. But we do not have
> > RCU inode lookup in VFS: inode lookup is done by rather a tranditional
> > way.
>
> Ah, I'd forgotten that neither of these RCU-based lookups ever got
> merged:
>
> https://lkml.org/lkml/2010/6/23/397
> http://thread.gmane.org/gmane.linux.kernel/1056494
>
> That, however, is the approach that the inode caches shoul dbe
> moving towards - RCU lookups to reduce locking, not changing
> i_lock/i_state atomicity that has been designed to facilitate RCU
> safe lookups...
>
> > XFS gives me the impression that it implements its own inode cache.
> > There may be such thing there. I have little knowledge on XFS, but I
> > guess it's unlikely impacted by the change of code implementing VFS
> > inode cache.
>
> Yeah, I dropped the generic inode hash RCU conversion - the
> SLAB_DESTROY_BY_RCU was proving to be rather complex, and I didn't
> have any motiviation to see it through because I'd already converted
> XFs to avoid the global inode_hash_lock and use RCU lookups on it's
> internal inode cache...
>
> > As far as I can see, RCU inode free is for RCU dentry lookup, which
> > seems have nothing to do with 'detect a freed inode'.
>
> If you know nothing of the history of this, then it might seem that
> way
>
> > Taking i_lock in these
> > places looks like to me a result of following old lock scheme blindly when
> > breaking the big global inode lock.
>
> The i_state/i_hash_list/i_lock relationship was created specifically
> during the inode_lock breakup to allow us to guarantee that certain
> fields of the inode are unchanging without needing to take multiple
> nested locks:
>
> $ gl -n 1 250df6e
> commit 250df6ed274d767da844a5d9f05720b804240197
> Author: Dave Chinner <dchinner@redhat.com>
> Date: Tue Mar 22 22:23:36 2011 +1100
>
> fs: protect inode->i_state with inode->i_lock
>
> Protect inode state transitions and validity checks with the
> inode->i_lock. This enables us to make inode state transitions
> independently of the inode_lock and is the first step to peeling
> away the inode_lock from the code.
>
> This requires that __iget() is done atomically with i_state checks
> during list traversals so that we don't race with another thread
> marking the inode I_FREEING between the state check and grabbing the
> reference.
>
> Also remove the unlock_new_inode() memory barrier optimisation
> required to avoid taking the inode_lock when clearing I_NEW.
> Simplify the code by simply taking the inode->i_lock around the
> state change and wakeup. Because the wakeup is no longer tricky,
> remove the wake_up_inode() function and open code the wakeup where
> necessary.
>
> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
>
> The inode hash lookup needs to check i_state atomically during the
> traversal so inodes being freed are skipped (e.g. I_FREEING,
> I_WILL_FREE). those i_state flags are set only with the i_lock held,
> and so inode hash lookups need to take the i_lock to guarantee the
> i_state field is correct. This inode data field synchronisation is
> separate to the cache hash list traversal protection.
>
> The only way to do this is to have an inner lock (inode->i_lock)
> that protects both the inode->i_hash_list and inode->i_state fields,
> and a lock order that provides outer list traversal protections
> (inode_hash_lock). Whether the outer lock is the inode_hash_lock or
> rcu_read_lock(), the lock order and the data fields the locks are
> protecting are the same....
>
> > Of course, maybe they are there for something. Could you speak
> > more about the race this change (patch 1,2?) brings up? Thank you.
>
> When you drop the lock from the i_state initialisation, you end up
> dropping the implicit unlock->lock memory barrier that the
> inode->i_lock provides. i.e. you get this in iget_locked():
>
>
> thread 1 thread 2
>
> lock(inode_hash_lock)
> for_each_hash_item()
>
> inode->i_state = I_NEW
> hash_list_insert
>
> <finds newly inserted inode>
> lock(inode->i_lock)
> unlock(inode->i_lock)
> unlock(inode_hash_lock)
>
> wait_on_inode()
> <see inode->i_state = 0 >
> <uses inode before initialisation
> is complete>
>
> IOWs, there is no unlock->lock transition occurring on any lock, so
> there are no implicit memory barriers in this code, and so other
> CPUs are not guaranteed to see the "inode->i_state = I_NEW" write
> that thread 2 did. The lock/unlock pair around this I_NEW assignment
> guarantees that thread 1 will see the change to i_state correctly.
>
> So even without RCU, dropping the i_lock from these
> i_state/hash insert/remove operations will result in races
> occurring...
>

This interleave can never happen because of inode_hash_lock. I_NEW will
not be missed by other CPUs in the same sense: though inode is put onto
the hash list, it cannot be accessed by others before inode_hash_lock
is freed.

> Seriously, if you want to improve the locking of this code, go back
> an resurrect the basic RCU hash traversal patches (i.e. Nick's
> original patch rather than my SLAB_DESTROY_BY_RCU based ones). That
> has much more benefit to many more workloads than just removing a
> non-global, uncontended locks like this patch set does.
>

Ah, this is intended to be a code clean patchset actually. I thought these
locks are redundant in an obvious and trivial manner. If, on the contrary,
they are such tricky, then never mind :) Thanks for your patient.

Regards,
Guo Chao




\
 
 \ /
  Last update: 2012-09-24 09:01    [W:0.052 / U:1.492 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site