linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Nick Piggin <npiggin@kernel.dk>
To: Nick Piggin <npiggin@kernel.dk>
Cc: Dave Chinner <david@fromorbit.com>,
	Christoph Hellwig <hch@infradead.org>,
	linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 17/18] fs: icache remove inode_lock
Date: Fri, 15 Oct 2010 14:44:51 +1100	[thread overview]
Message-ID: <20101015034451.GA6827@amd> (raw)
In-Reply-To: <20101015033017.GA6750@amd>

On Fri, Oct 15, 2010 at 02:30:17PM +1100, Nick Piggin wrote:
> On Fri, Oct 15, 2010 at 02:13:43PM +1100, Dave Chinner wrote:
> >  You've shown it can be done, and that's great - it shows
> > us the impact of making those changes, but they need to be analysed
> > separately and treated on own their merits, not lumped with core
> > locking changes necessary for store-free path walking.
> 
> Actually I didn't see anyone else object to doing this. Everybody
> else it seems acknowledges that it needs to be done, and it gets
> done naturally as a side effect of fine grained locking.

Let's just get back to this part, which seems to be one you have
the most issues with maybe?

You're objecting to per-zone locks and per-zone LRUs for inode and
dcache?

Well I have told you why per-zone LRUs are needed, I can expand on
any of the reasons if that is unclear. Per-zone locks I think come
naturally at the same time and they will expose some fs bottlenecks,
but that is simply how scalability development works.

So, do you object to per-zone LRUs in particular, or per-zone locks?
(Ie. the potentially changed reclaim pattern, or the increased
parallelism).

When you looked at this initially, you didn't understand how
reclaim works. It will not fill up a zone with inodes and then start
reclaiming all those inodes, leaving other nodes empty (unless that
is how you configure the machine, but it isn't the default). It
fills up inodes from all nodes (same as today) and it will start
reclaiming from all nodes at about the same pressure when there is
a shortage.

Reclaim basically approximates LRU by scanning a little from the top
of each LRU. When you have many thousands of objects, and reclaim is
a really failable and dumb process anyway, then the perturbation of
the reclaim pattern doesn't matter much. Our zone based page reclaim
works exactly the same way.

I don't think you can possibly be arguing against more scalable
locking in reclaim, so perhaps you are also worried about increased
parallelism in the filesystem callbacks from reclaim? I really can't
see this being a big problem, any more than any other increased
paralellism on fses or other subsystems caused by scaling vfs.

There might be some interesting issues with different locking
designs being hit in different ways, but really we can't stop
progress and test all loads on all filesystems. The way forward is
to fix the bottleneck in the filesystem, or the filesystem sucks
so bad it can't handle it, then just put a lock in there and not
peanalise others.

It's not like I haven't tested it, I've spent the better part of
the past year testing things. The I_FREEING batching stuff is one
example where I found and fixed a small problem exposed by the
reclaim changes.

  reply	other threads:[~2010-10-15  3:44 UTC|newest]

Thread overview: 162+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2010-10-08  5:21 fs: Inode cache scalability V2 Dave Chinner
2010-10-08  5:21 ` [PATCH 01/18] kernel: add bl_list Dave Chinner
2010-10-08  8:18   ` Andi Kleen
2010-10-08 10:33     ` Dave Chinner
2010-10-08  5:21 ` [PATCH 02/18] fs: Convert nr_inodes and nr_unused to per-cpu counters Dave Chinner
2010-10-08  7:01   ` Christoph Hellwig
2010-10-08  5:21 ` [PATCH 03/18] fs: keep inode with backing-dev Dave Chinner
2010-10-08  7:01   ` Christoph Hellwig
2010-10-08  7:27     ` Dave Chinner
2010-10-08  5:21 ` [PATCH 04/18] fs: Implement lazy LRU updates for inodes Dave Chinner
2010-10-08  7:08   ` Christoph Hellwig
2010-10-08  7:31     ` Dave Chinner
2010-10-08  9:08   ` Al Viro
2010-10-08  9:51     ` Dave Chinner
2010-10-08  5:21 ` [PATCH 05/18] fs: inode split IO and LRU lists Dave Chinner
2010-10-08  7:14   ` Christoph Hellwig
2010-10-08  7:38     ` Dave Chinner
2010-10-08  9:16   ` Al Viro
2010-10-08  9:58     ` Dave Chinner
2010-10-08  5:21 ` [PATCH 06/18] fs: Clean up inode reference counting Dave Chinner
2010-10-08  7:20   ` Christoph Hellwig
2010-10-08  7:46     ` Dave Chinner
2010-10-08  8:15       ` Christoph Hellwig
2010-10-08  5:21 ` [PATCH 07/18] exofs: use iput() for inode reference count decrements Dave Chinner
2010-10-08  7:21   ` Christoph Hellwig
2010-10-16  7:56   ` Nick Piggin
2010-10-16 16:29     ` Christoph Hellwig
2010-10-17 15:41       ` Boaz Harrosh
2010-10-08  5:21 ` [PATCH 08/18] fs: add inode reference coutn read accessor Dave Chinner
2010-10-08  7:24   ` Christoph Hellwig
2010-10-08  5:21 ` [PATCH 09/18] fs: rework icount to be a locked variable Dave Chinner
2010-10-08  7:27   ` Christoph Hellwig
2010-10-08  7:50     ` Dave Chinner
2010-10-08  8:17       ` Christoph Hellwig
2010-10-08 13:16         ` Chris Mason
2010-10-08  9:32   ` Al Viro
2010-10-08 10:15     ` Dave Chinner
2010-10-08 13:14       ` Chris Mason
2010-10-08 13:53       ` Christoph Hellwig
2010-10-08 14:09         ` Dave Chinner
2010-10-08  5:21 ` [PATCH 10/18] fs: Factor inode hash operations into functions Dave Chinner
2010-10-08  7:29   ` Christoph Hellwig
2010-10-08  9:41     ` Al Viro
2010-10-08  5:21 ` [PATCH 11/18] fs: Introduce per-bucket inode hash locks Dave Chinner
2010-10-08  7:33   ` Christoph Hellwig
2010-10-08  7:51     ` Dave Chinner
2010-10-08  9:49   ` Al Viro
2010-10-08  9:51     ` Christoph Hellwig
2010-10-08 13:43   ` Christoph Hellwig
2010-10-08 14:17     ` Dave Chinner
2010-10-08 18:54   ` Christoph Hellwig
2010-10-16  7:57     ` Nick Piggin
2010-10-16 16:16       ` Christoph Hellwig
2010-10-16 17:12         ` Nick Piggin
2010-10-17  0:45           ` Christoph Hellwig
2010-10-17  2:06             ` Nick Piggin
2010-10-17  0:46           ` Dave Chinner
2010-10-17  2:25             ` Nick Piggin
2010-10-18 16:16               ` Andi Kleen
2010-10-18 16:21                 ` Christoph Hellwig
2010-10-19  7:00                   ` Nick Piggin
2010-10-19 16:50                     ` Christoph Hellwig
2010-10-20  3:11                       ` Nick Piggin
2010-10-24 15:44                       ` Thomas Gleixner
2010-10-24 21:17                         ` Nick Piggin
2010-10-25  4:41                           ` Thomas Gleixner
2010-10-25  7:04                             ` Thomas Gleixner
2010-10-26  0:12                               ` Nick Piggin
2010-10-26  0:06                             ` Nick Piggin
2010-10-08  5:21 ` [PATCH 12/18] fs: add a per-superblock lock for the inode list Dave Chinner
2010-10-08  7:35   ` Christoph Hellwig
2010-10-08  5:21 ` [PATCH 13/18] fs: split locking of inode writeback and LRU lists Dave Chinner
2010-10-08  7:42   ` Christoph Hellwig
2010-10-08  8:00     ` Dave Chinner
2010-10-08  8:18       ` Christoph Hellwig
2010-10-16  7:57         ` Nick Piggin
2010-10-16 16:20           ` Christoph Hellwig
2010-10-16 17:19             ` Nick Piggin
2010-10-17  1:00               ` Dave Chinner
2010-10-17  2:20                 ` Nick Piggin
2010-10-08  5:21 ` [PATCH 14/18] fs: Protect inode->i_state with th einode->i_lock Dave Chinner
2010-10-08  7:49   ` Christoph Hellwig
2010-10-08  8:04     ` Dave Chinner
2010-10-08  8:18       ` Christoph Hellwig
2010-10-16  7:57         ` Nick Piggin
2010-10-16 16:19           ` Christoph Hellwig
2010-10-09  8:05       ` Christoph Hellwig
2010-10-09 14:52       ` Matthew Wilcox
2010-10-10  2:01         ` Dave Chinner
2010-10-08  5:21 ` [PATCH 15/18] fs: introduce a per-cpu last_ino allocator Dave Chinner
2010-10-08  7:53   ` Christoph Hellwig
2010-10-08  8:05     ` Dave Chinner
2010-10-08  8:22   ` Andi Kleen
2010-10-08  8:44     ` Christoph Hellwig
2010-10-08  9:58     ` Al Viro
2010-10-08 10:09       ` Andi Kleen
2010-10-08 10:19         ` Al Viro
2010-10-08 10:20           ` Eric Dumazet
2010-10-08  9:56   ` Al Viro
2010-10-08 10:03     ` Christoph Hellwig
2010-10-08 10:20       ` Eric Dumazet
2010-10-08 13:48         ` Christoph Hellwig
2010-10-08 14:06           ` Eric Dumazet
2010-10-08 19:10             ` Christoph Hellwig
2010-10-09 17:14             ` Matthew Wilcox
2010-10-16  7:57       ` Nick Piggin
2010-10-16 16:22         ` Christoph Hellwig
2010-10-16 17:21           ` Nick Piggin
2010-10-08  5:21 ` [PATCH 16/18] fs: Make iunique independent of inode_lock Dave Chinner
2010-10-08  7:55   ` Christoph Hellwig
2010-10-08  8:06     ` Dave Chinner
2010-10-08  8:19       ` Christoph Hellwig
2010-10-08  5:21 ` [PATCH 17/18] fs: icache remove inode_lock Dave Chinner
2010-10-08  8:03   ` Christoph Hellwig
2010-10-08  8:09     ` Dave Chinner
2010-10-13  7:20   ` Nick Piggin
2010-10-13  7:27     ` Nick Piggin
2010-10-13 11:28       ` Christoph Hellwig
2010-10-13 12:03         ` Nick Piggin
2010-10-13 12:20           ` Christoph Hellwig
2010-10-13 12:25             ` Nick Piggin
2010-10-13 10:42     ` Eric Dumazet
2010-10-13 12:07       ` Nick Piggin
2010-10-13 11:25     ` Christoph Hellwig
2010-10-13 12:30       ` Nick Piggin
2010-10-13 23:23         ` Dave Chinner
2010-10-14  9:06           ` Nick Piggin
2010-10-14  9:13             ` Nick Piggin
2010-10-14 14:41             ` Christoph Hellwig
2010-10-15  0:14               ` Nick Piggin
2010-10-15  3:13                 ` Dave Chinner
2010-10-15  3:30                   ` Nick Piggin
2010-10-15  3:44                     ` Nick Piggin [this message]
2010-10-15  6:41                       ` Nick Piggin
2010-10-15 10:59                         ` Dave Chinner
2010-10-15 13:03                           ` Nick Piggin
2010-10-15 13:29                             ` Nick Piggin
2010-10-15 17:33                               ` Nick Piggin
2010-10-15 17:52                                 ` Christoph Hellwig
2010-10-15 18:02                                   ` Nick Piggin
2010-10-15 18:14                                     ` Nick Piggin
2010-10-16  2:09                                     ` Nick Piggin
2010-10-15 14:11                             ` Nick Piggin
2010-10-15 20:50                           ` Nick Piggin
2010-10-15 20:56                             ` Nick Piggin
2010-10-15  4:04               ` Nick Piggin
2010-10-15 11:33                 ` Dave Chinner
2010-10-15 13:14                   ` Nick Piggin
2010-10-15 15:38                   ` Nick Piggin
2010-10-16  7:57   ` Nick Piggin
2010-10-08  5:21 ` [PATCH 18/18] fs: Reduce inode I_FREEING and factor inode disposal Dave Chinner
2010-10-08  8:11   ` Christoph Hellwig
2010-10-08 10:18   ` Al Viro
2010-10-08 10:52     ` Dave Chinner
2010-10-08 12:10       ` Al Viro
2010-10-08 13:55         ` Dave Chinner
2010-10-09 17:22   ` Matthew Wilcox
2010-10-09  8:08 ` [PATCH 19/18] fs: split __inode_add_to_list Christoph Hellwig
2010-10-12 10:47   ` Dave Chinner
2010-10-12 11:31     ` Christoph Hellwig
2010-10-12 12:05       ` Dave Chinner
2010-10-09 11:18 ` [PATCH 20/18] fs: do not assign default i_ino in new_inode Christoph Hellwig

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=20101015034451.GA6827@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).