From: Nick Piggin <npiggin@kernel.dk>
To: Dave Chinner <david@fromorbit.com>
Cc: Nick Piggin <npiggin@kernel.dk>,
Christoph Hellwig <hch@infradead.org>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 17/19] fs: Reduce inode I_FREEING and factor inode disposal
Date: Sun, 17 Oct 2010 18:05:19 +1100 [thread overview]
Message-ID: <20101017070519.GA24641@amd> (raw)
In-Reply-To: <20101017065245.GE29677@dastard>
On Sun, Oct 17, 2010 at 05:52:45PM +1100, Dave Chinner wrote:
> On Sun, Oct 17, 2010 at 04:13:10PM +1100, Nick Piggin wrote:
> > On Sun, Oct 17, 2010 at 03:35:14PM +1100, Nick Piggin wrote:
> > > On Sun, Oct 17, 2010 at 03:13:13PM +1100, Dave Chinner wrote:
> > > > On Sun, Oct 17, 2010 at 01:49:23PM +1100, Nick Piggin wrote:
> > > > > On Sat, Oct 16, 2010 at 09:30:47PM -0400, Christoph Hellwig wrote:
> > > > > > > * inode->i_lock is *always* the innermost lock.
> > > > > > > *
> > > > > > > + * inode->i_lock is *always* the innermost lock.
> > > > > > > + *
> > > > > >
> > > > > > No need to repeat, we got it..
> > > > >
> > > > > Except that I didn't see where you fixed all the places where it is
> > > > > *not* the innermost lock. Like for example places that take dcache_lock
> > > > > inside i_lock.
> > > >
> > > > I can't find any code outside of ceph where the dcache_lock is used
> > > > within 200 lines of code of the inode->i_lock. The ceph code is not
> > > > nesting them, though.
> > >
> > > You mustn't have looked very hard? From ceph:
> > >
> > > spin_unlock(&dcache_lock);
> > > spin_unlock(&inode->i_lock);
> > >
> > > (and yes, acquisition side does go in i_lock->dcache_lock order)
>
> Sorry, easy to miss with a quick grep when the locks are taken in
> different functions.
Easy to see they're nested when they're dropped in adjacent lines. That
should give you a clue to go and check their lock order.
> Anyway, this one looks difficult to fix without knowing something
> about Ceph and wtf it is doing there. It's one to punt to the
> maintainer to solve as it's not critical to this patch set.
I thought the raison detre for your starting to write your own vfs
scale branch was because you objected to i_lock not being an "innermost"
lock (not that it was before my patch).
So I don't get it. If your patch mandates it to be an innermost lock,
then you absolutely do need to fix the filesystems before changing the
lock order.
> > A really quick grep reveals cifs is using GlobalSMBSeslock inside i_lock
> > too.
>
> I'm having a grep-fail day. Where is that one?
Uh, inside one of the 6 places that i_lock is taken in cifs. The only
non-trivial one, not surprisingly.
next prev parent reply other threads:[~2010-10-17 7:05 UTC|newest]
Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-16 8:13 Inode Lock Scalability V4 Dave Chinner
2010-10-16 8:13 ` [PATCH 01/19] fs: switch bdev inode bdi's correctly Dave Chinner
2010-10-16 9:30 ` Nick Piggin
2010-10-16 16:31 ` Christoph Hellwig
2010-10-16 8:13 ` [PATCH 02/19] kernel: add bl_list Dave Chinner
2010-10-16 9:51 ` Nick Piggin
2010-10-16 16:32 ` Christoph Hellwig
2010-10-16 8:13 ` [PATCH 03/19] fs: Convert nr_inodes and nr_unused to per-cpu counters Dave Chinner
2010-10-16 8:29 ` Eric Dumazet
2010-10-16 10:04 ` Dave Chinner
2010-10-16 10:27 ` Eric Dumazet
2010-10-16 17:26 ` Peter Zijlstra
2010-10-17 1:09 ` Dave Chinner
2010-10-17 1:12 ` Christoph Hellwig
2010-10-17 2:16 ` Dave Chinner
2010-10-16 8:13 ` [PATCH 04/19] fs: Implement lazy LRU updates for inodes Dave Chinner
2010-10-16 9:29 ` Nick Piggin
2010-10-16 16:59 ` Christoph Hellwig
2010-10-16 17:29 ` Nick Piggin
2010-10-16 17:34 ` Nick Piggin
2010-10-17 0:47 ` Christoph Hellwig
2010-10-17 0:47 ` Christoph Hellwig
2010-10-17 2:09 ` Nick Piggin
2010-10-17 1:53 ` Dave Chinner
2010-10-16 8:13 ` [PATCH 05/19] fs: inode split IO and LRU lists Dave Chinner
2010-10-16 8:14 ` [PATCH 06/19] fs: Clean up inode reference counting Dave Chinner
2010-10-16 8:14 ` [PATCH 07/19] exofs: use iput() for inode reference count decrements Dave Chinner
2010-10-16 8:14 ` [PATCH 08/19] fs: rework icount to be a locked variable Dave Chinner
2010-10-16 8:14 ` [PATCH 09/19] fs: Factor inode hash operations into functions Dave Chinner
2010-10-16 8:14 ` [PATCH 10/19] fs: Introduce per-bucket inode hash locks Dave Chinner
2010-10-16 8:14 ` [PATCH 11/19] fs: add a per-superblock lock for the inode list Dave Chinner
2010-10-16 8:14 ` [PATCH 12/19] fs: split locking of inode writeback and LRU lists Dave Chinner
2010-10-16 8:14 ` [PATCH 13/19] fs: Protect inode->i_state with the inode->i_lock Dave Chinner
2010-10-16 8:14 ` [PATCH 14/19] fs: introduce a per-cpu last_ino allocator Dave Chinner
2010-10-16 8:14 ` [PATCH 15/19] fs: Make iunique independent of inode_lock Dave Chinner
2010-10-16 8:14 ` [PATCH 16/19] fs: icache remove inode_lock Dave Chinner
2010-10-16 8:14 ` [PATCH 17/19] fs: Reduce inode I_FREEING and factor inode disposal Dave Chinner
2010-10-17 1:30 ` Christoph Hellwig
2010-10-17 2:49 ` Nick Piggin
2010-10-17 4:13 ` Dave Chinner
2010-10-17 4:35 ` Nick Piggin
2010-10-17 5:13 ` Nick Piggin
2010-10-17 6:52 ` Dave Chinner
2010-10-17 7:05 ` Nick Piggin [this message]
2010-10-17 23:39 ` Dave Chinner
2010-10-18 21:27 ` Sage Weil
2010-10-19 3:54 ` Nick Piggin
2010-10-16 8:14 ` [PATCH 18/19] fs: split __inode_add_to_list Dave Chinner
2010-10-16 8:14 ` [PATCH 19/19] fs: do not assign default i_ino in new_inode Dave Chinner
2010-10-16 9:09 ` Eric Dumazet
2010-10-16 16:35 ` Christoph Hellwig
2010-10-18 9:11 ` Eric Dumazet
2010-10-18 14:48 ` Christoph Hellwig
2010-10-16 17:55 ` Inode Lock Scalability V4 Nick Piggin
2010-10-17 2:47 ` Dave Chinner
2010-10-17 2:55 ` Nick Piggin
2010-10-17 2:57 ` Nick Piggin
2010-10-17 6:10 ` Dave Chinner
2010-10-17 6:34 ` Nick Piggin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101017070519.GA24641@amd \
--to=npiggin@kernel.dk \
--cc=david@fromorbit.com \
--cc=hch@infradead.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).