From: Nick Piggin <npiggin@kernel.dk>
To: Christoph Hellwig <hch@infradead.org>
Cc: Nick Piggin <npiggin@kernel.dk>,
Dave Chinner <david@fromorbit.com>,
linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 04/19] fs: Implement lazy LRU updates for inodes.
Date: Sun, 17 Oct 2010 04:29:24 +1100 [thread overview]
Message-ID: <20101016172924.GA3519@amd> (raw)
In-Reply-To: <20101016165930.GA20626@infradead.org>
On Sat, Oct 16, 2010 at 12:59:30PM -0400, Christoph Hellwig wrote:
> On Sat, Oct 16, 2010 at 08:29:16PM +1100, Nick Piggin wrote:
> > On Sat, Oct 16, 2010 at 07:13:58PM +1100, Dave Chinner wrote:
> > > @@ -502,11 +527,15 @@ static void prune_icache(int nr_to_scan)
> > > iput(inode);
> > > spin_lock(&inode_lock);
> > >
> > > - if (inode != list_entry(inode_unused.next,
> > > - struct inode, i_list))
> > > - continue; /* wrong inode or list_empty */
> > > - if (!can_unuse(inode))
> > > + /*
> > > + * if we can't reclaim this inode immediately, give it
> > > + * another pass through the free list so we don't spin
> > > + * on it.
> > > + */
> > > + if (!can_unuse(inode)) {
> > > + list_move(&inode->i_list, &inode_unused);
> > > continue;
> > > + }
> > > }
> > > list_move(&inode->i_list, &freeable);
> > > WARN_ON(inode->i_state & I_NEW);
> >
> > This is a bug, actually 2 bugs, which is why I omitted it in the version
> > you picked up. I agree we want the optimisation though, so I've added it
> > back in my tree.
> >
> > After you iput() and then re take the inode lock, you can't reference
> > the inode because you don't know what happened to it. You need to keep
> > that pointer check to verify it is still there.
>
> I don't think the pointer check will work either. By the time we retake
> the lru lock the inode might already have been reaped through a call
> to invalidate_inodes. There's no way we can do anything with it after
I don't think you're right. If we re take inode_lock, ensure it is on
the LRU, and call the can_unuse checks, there is no more problem than
the regular loop taking items from the LRU, AFAIKS.
> iput. What we could do is using variant of can_unuse to decide to move
> the inode to the front of the lru before doing the iput. That way
> we'll get to it next after retaking the lru lock if it's still there.
This might actually be the better approach anyway (even for upstream)
-- it means we don't have to worry about the "check head element"
heuristic of the LRU check which could get false negatives if there is
a lot of concurrency on the LRU.
next prev parent reply other threads:[~2010-10-16 17:29 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 [this message]
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
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=20101016172924.GA3519@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).