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

From: Nick Piggin <npiggin@suse.de>
To: Theodore Tso <tytso@MIT.EDU>
Cc: Dave Chinner <david@fromorbit.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org,
	John Stultz <johnstul@us.ibm.com>,
	Frank Mayhar <fmayhar@google.com>
Subject: Re: [patch 29/52] fs: icache lock i_count
Date: Tue, 6 Jul 2010 23:04:37 +1000	[thread overview]
Message-ID: <20100706130437.GJ11732@laptop> (raw)
In-Reply-To: <48F48148-9909-47D0-A850-04B22403C006@mit.edu>

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.