On Tue, Jul 06, 2010 at 06:38:28AM -0400, Theodore Tso wrote: > > On Jul 6, 2010, at 12:34 AM, Nick Piggin wrote: > > On Tue, Jul 06, 2010 at 08:41:06AM +1000, Dave Chinner wrote: > >> > >> > >> 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", > >> 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... > > > > If some filesystem introduces a lock ordering problem from not > > reading the newly added documentation, lockdep should catch it pretty > > quick. > > I assume you mean inline documentation in the source, because I > quickly scanned the source and couldn't find any significant changes > to any files in Documentation. > > It would be nice if the new state of affairs is documented in a single file, > so that people who want to understand this new locking system don't > have to go crawling through the code, or searching mailing list archives > to figure out what's going on. These type of internal details of lock ordering I think work best in source files (see rmap.c and filemap.c) so it's a little closer to the source code. That's in inode.c and dcache.c. Locking for filesystem callback APIs I agree is just as good to be in Documentation/filesystems/Locking (which I need to update a bit), but it's never been used for this stuff before. I'm always open to suggestions of how to document it better though. > A lot of the text in this mail thread, including your discussion of the new > locking hierarchy, and why things are the way they are, would be good > fodder for a new documentation file. And if you don't want to rename > i_lock, because no better name can be found, we should at least > document that starting as of 2.6.35/36 the meaning of i_lock changed. Well there is not much definition of what i_lock is. It is really not an "innermost" lock anyway (whatever that exactly means). CEPH even takes dcache_lock inside i_lock. NFS uses it pretty extensively too. So it really is *the* non sleeping lock to protect inode fields that I can see. As far as filesystems go, inode changes matter very little really, but the best I can do is just to comment and document the locking and try to audit each filesystem. -- 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
