On Wed, Jun 30, 2010 at 10:05:02PM +1000, Nick Piggin wrote: > On Wed, Jun 30, 2010 at 05:27:02PM +1000, Dave Chinner wrote: > > On Thu, Jun 24, 2010 at 01:02:41PM +1000, npiggin@xxxxxxx wrote: > > > Protect inode->i_count with i_lock, rather than having it atomic. > > > Next step should also be to move things together (eg. the refcount increment > > > into d_instantiate, which will remove a lock/unlock cycle on i_lock). > > ..... > > > Index: linux-2.6/fs/inode.c > > > =================================================================== > > > --- linux-2.6.orig/fs/inode.c > > > +++ linux-2.6/fs/inode.c > > > @@ -33,14 +33,13 @@ > > > * inode_hash_lock protects: > > > * inode hash table, i_hash > > > * inode->i_lock protects: > > > - * i_state > > > + * i_state, i_count > > > * > > > * Ordering: > > > * inode_lock > > > * sb_inode_list_lock > > > * inode->i_lock > > > - * inode_lock > > > - * inode_hash_lock > > > + * inode_hash_lock > > > */ > > > > I thought that the rule governing the use of inode->i_lock was that > > it can be used anywhere as long as it is the innermost lock. > > > > Hmmm, no references in the code or documentation. Google gives a > > pretty good reference: > > > > http://www.mail-archive.com/linux-ext4@xxxxxxxxxxxxxxx/msg02584.html > > > > Perhaps a different/new lock needs to be used here? > > Well I just changed the order (and documented it to boot :)). It's > pretty easy to verify that LOR is no problem. inode hash is only > taken in a very few places so other code outside inode.c is fine to > use i_lock as an innermost lock. It's not just the inode_hash_lock - you move four or five other locks under inode->i_lock as the series progresses. IOWs, there's now many paths and locking orders where the i_lock is not innermost. If we go forward with this, it's only going to get more complex and eventually somewhere we'll need a new lock for an innermost operation because inode->i_lock is no longer safe to use.... Seriously: use a new lock for high level inode operations you are optimising - don't repurpose an existing lock with different usage rules just because it's convenient. 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
