Re: [patch 29/52] fs: icache lock i_count

[Nick Piggin]

On Tue, Jul 06, 2010 at 08:41:06AM +1000, Dave Chinner wrote:
> On Sat, Jul 03, 2010 at 03:06:52PM +1000, Nick Piggin wrote:
> > So it makes a lot of sense to have a lock to rule the inode (as opposed
> > to now we have a lock to rule *all* inodes).
> 
> I don't disagree with this approach - I object to the fact that you
> repurpose an existing lock and change it's locking rules to "rule
> the inode". We don't have any one lock that "rules the inode",

"rule the inode" was in regard to the inode cache / dirty / writeout
handling; ie. what inode_lock is currently for. The idea is that in
these code paths, we tend to want to take an inode, lock it, and then
manipulate it (including putting it on or taking it off data
structures).


> anyway, so adding a new "i_list_lock" for the new VFS level locking
> strategies makes it a lot more self-contained. Fundamentally I'm
> less concerned about the additional memory usage than I am about
> having landmines planted around i_lock...

(it's not just lists, its refcount and i_state too)

I just don't see a problem. _No_ filesystem takes any of the locks
that nest inside i_lock. They may call some inode.c functions, but
shouldn't do so while holding i_lock if they are treating i_lock as
an innermost lock. So they can keep using i_lock. I did go through
and attempt to look at all filesystems.

Within inode.c, lock ordering is 2 levels deep, same as it was when
we had inode_lock and i_lock.

If some filesystem introduces a lock ordering problem from not
reading the newly added documentation, lockdep should catch it pretty
quick.

So I don't see a landmine they don't have (in much larger doses) with
i_mutex, reentrant reclaim, page lock, buffer lock, mmap_sem etc
already.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/