On Fri, Oct 22, 2010 at 02:14:31PM +1100, Dave Chinner wrote: > AFAICT, moving the inode_lru_lock inside i_lock doesn't affect the > locking order of anything else, and I agree that putting a single > trylock in the prune_icache loop is definitely cleaner than anything > else I've been able to think of that retains the current locking > order. It will also remove the wart in sync_inode(). > > So, I'll swallow my rhetoric and agree with you that inode_lru_lock > inside the i_lock is the natural and easiest way to nest these > locks. I'll rework the series to do this. I don't know what's so unclean or wrong about the locking order. I'm still leaning much further than Al as to putting locks inside i_lock. With inode-rcu, we _always_ arrive at the inode _first_, before taking _any_ other locks (because we either come in via an external reference, or we can find the inode from any of the data structures using RCU rather than taking their locks). [The exception is hash insertion uniqueness enforcement, because we have no inode to lock, by definition. But in that case we're OK because the new inode we're about to insert has no concurrency on its i_lock so that can be initialised locked.] So what we have is an inode, which we need to do stuff to. That stuff involves moving it on or off data structures, and updating its refcount and state, etc. That suggests the i_lock -> data-structure-lock locking order. And look at the flexibility -- I could implement that without changing any other code or ordering in the icache. Sure you can reorder things around to cope with different locking strategies, but I'm not actually given a good reason _why_ i_lock first is not the "natural" ordering. Doing that means we simply don't _need_ to change any ordering or concurrency rules (although it doesn't prevent changes). -- 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
