Message-ID: <ZTYAUyiTYsX43O9F@dread.disaster.area>
Date:   Mon, 23 Oct 2023 16:10:43 +1100
From:   Dave Chinner <david@...morbit.com>
To:     Mateusz Guzik <mjguzik@...il.com>
Cc:     Christian Brauner <brauner@...nel.org>,
        Dave Chinner <dchinner@...hat.com>,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-bcachefs@...r.kernel.org,
        Kent Overstreet <kent.overstreet@...ux.dev>,
        Alexander Viro <viro@...iv.linux.org.uk>
Subject: Re: (subset) [PATCH 22/32] vfs: inode cache conversion to hash-bl

On Fri, Oct 20, 2023 at 07:49:18PM +0200, Mateusz Guzik wrote:
> On 10/20/23, Dave Chinner <david@...morbit.com> wrote:
> > On Thu, Oct 19, 2023 at 05:59:58PM +0200, Mateusz Guzik wrote:
> >> > To be clear there is no urgency as far as I'm concerned, but I did run
> >> > into something which is primarily bottlenecked by inode hash lock and
> >> > looks like the above should sort it out.
> >> >
> >> > Looks like the patch was simply forgotten.
> >> >
> >> > tl;dr can this land in -next please
> >>
> >> In case you can't be arsed, here is something funny which may convince
> >> you to expedite. ;)
> >>
> >> I did some benching by running 20 processes in parallel, each doing stat
> >> on a tree of 1 million files (one tree per proc, 1000 dirs x 1000 files,
> >> so 20 mln inodes in total).  Box had 24 cores and 24G RAM.
> >>
> >> Best times:
> >> Linux:          7.60s user 1306.90s system 1863% cpu 1:10.55 total
> >> FreeBSD:        3.49s user 345.12s system 1983% cpu 17.573 total
> >> OpenBSD:        5.01s user 6463.66s system 2000% cpu 5:23.42 total
> >> DragonflyBSD:   11.73s user 1316.76s system 1023% cpu 2:09.78 total
> >> OmniosCE:       9.17s user 516.53s system 1550% cpu 33.905 total
> >>
> >> NetBSD failed to complete the run, OOM-killing workers:
> >> http://mail-index.netbsd.org/tech-kern/2023/10/19/msg029242.html
> >> OpenBSD is shafted by a big kernel lock, so no surprise it takes a long
> >> time.
> >>
> >> So what I find funny is that Linux needed more time than OmniosCE (an
> >> Illumos variant, fork of Solaris).
> >>
> >> It also needed more time than FreeBSD, which is not necessarily funny
> >> but not that great either.
> >>
> >> All systems were mostly busy contending on locks and in particular Linux
> >> was almost exclusively busy waiting on inode hash lock.
> >
> > Did you bother to test the patch, or are you just complaining
> > that nobody has already done the work for you?
> 
> Why are you giving me attitude?

Look in the mirror, mate.

Starting off with a derogatory statement like:

"In case you can't be arsed, ..."

is a really good way to start a fight.

I don't think anyone working on this stuff couldn't be bothered to
get their lazy arses off their couches to get it merged. Though you
may not have intended it that way, that's exactly what "can't be
arsed" means. 

I have not asked for this code to be merged because I'm not ready to
ask for it to be merged. I'm trying to be careful and cautious about
changing core kernel code that every linux installation out there
uses because I care about this code being robust and stable. That's
the exact opposite of "can't be arsed"....

Further, you have asked for code that is not ready to be merged to
be merged without reviewing it or even testing it to see if it
solved your reported problem. This is pretty basic stuff - it you
want it merged, then *you also need to put effort into getting it
merged* regardless of who wrote the code. TANSTAAFL.

But you've done neither - you've just made demands and thrown
hypocritical shade implying busy people working on complex code are
lazy arses.

Perhaps you should consider your words more carefully in future?

> > Because if you tested the patch, you'd have realised that by itself
> > it does nothing to improve performance of the concurrent find+stat
> > workload. The lock contention simply moves to the sb_inode_list_lock
> > instead.
> >
> 
> Is that something you benched? While it may be there is no change,
> going from one bottleneck to another does not automatically mean there
> are no gains in performance.

Of course I have. I wouldn't have said anything if this wasn't a
subject I have specific knowledge and expertise in. As I've already
said, I've been running this specific "will it scale" find+stat
micro-benchmark for well over a decade. For example:

https://lore.kernel.org/linux-xfs/20130603074452.GZ29466@dastard/

That's dated June 2013, and the workload is:

"8-way 50 million zero-length file create, 8-way
find+stat of all the files, 8-unlink of all the files:"

Yeah, this workload only scaled to a bit over 4 CPUs a decade ago,
hence I only tested to 8-way....

> For example, this thing on FreeBSD used to take over one minute (just
> like on Linux right now), vast majority of which was spent on
> multicore issues. I massaged it down to ~18 seconds, despite it still
> being mostly bottlenecked on locks.
> 
> So I benched the hashbl change and it provides a marked improvement:
> stock:          7.60s user 1306.90s system 1863% cpu 1:10.55 total
> patched:  6.34s user 453.87s system 1312% cpu 35.052 total
> 
> But indeed as expected it is still bottlenecked on locks.

That's better than I expected, but then again I haven't looked at
this code in detail since around 5.17 and lots has changed since
then.  What filesystem was this? What kernel?  What locks is it
bottlenecked on now?  Did you test the vfs-scale branch I pointed
you at, or just the hash-bl patches?

> > IOWs, those sb_inode_list_lock changes haven't been included for the
> > same reason as the hash-bl patches: outside micro-benchmarks, these
> > locks just don't show up in profiles on production machines.
> > Hence there's no urgency to "fix" these lock contention
> > problems despite the ease with which micro-benchmarks can reproduce
> > it...
> 
> The above is not a made-up microbenchmark though.

I didn't say anything about it being "made up".

There's typically a huge difference in behaviour between the
microbenchmark which immediately discards retrieved data and has no
memory footprint to speak of versus an application that comparing
the retrieved data with an in-memory index of inodes held
in a memory constrained environment to determine if anything has
changed and then doing different work if they have changed.

IOWs, while microbenchmarks can easily produce contention, it's much
less obvious that applications doing substantial userspace work
between similar data retrieval operations will experience similar kernel
level contention problems.

What is lacking here is real world evidence showing this is a
production level problem that needs to be solved immediately....

> I got someone running FreeBSD whose workload mostly consists of
> stating tens of millions of files in parallel and which was suffering
> a lot from perf standpoint -- flamegraphs show that contending on
> locks due to memory reclamation induced by stat calls is almost
> everything that was going on at the time.

.... and "one person's workload on FreeBSD" is not significant
evidence there's a Linux kernel problem that needs to be solved
immediately.

> Said workload probably should not do that to begin with (instead
> have a db with everything it normally stats for?), but here we
> are.

As you state, the right fix for the application is to avoid scanning
tens of millions of inodes repeatedly.  We have functionality in
linux like fanotify to watch and report changes to individual files
in a huge filesystem, so even if this was running on Linux the
push-back would be to use fanotify and avoid repeatedly poll the
entire filesystem to find individual file changes.

> That is to say, while I would not be in position to test Linux in the
> above workload, the problem (high inode turnover in memory) is very
> much real.

Yup, XFS currently bottlenecks at about 800,000 inodes/s being
streamed through memory my old 32p test machine - it's largely the
sb->s_inode_list_lock that is the limitation. The vfs-scale branch
I've pointed to brings that up to about 1.5 million inodes/s before
the next set of limits are hit - the system is CPU bound due to the
aggregate memory demand of ~10GB/s being allocated and freed by the
mm subsystem (not lock contention). Hence further improvements are
all about improving per-lookup operation CPU and memory efficiency..

> All that said, if a real deployment which runs into the problem is
> needed to justify the change, then I can't help (wrong system).

Well, that's kind of the point, though - if users and customers are
not reporting that they have production workloads where 800,000
inodes/s throughput through the inode cache is the performance
limiting factor, then why risk destabilising core code by changing
it?

Yes, we can go faster (as the vfs-scale branch shows), but if
applications aren't limited by the existing code, why risk breaking
every linux installation out there by pushing something that isn't
100% baked?  Nobody wins if the new code is faster for a few but has
bugs that many people hit, so if there's no urgency to change the
code I won't hurry to push the change. I've carried this code for
years, a few months here or there isn't going to change anything
material....

If you think that's wrong or want it faster than I might address it,
then by all means you can take the vfs-scale branch and do the
validation work needed to get it pushed it upstream sooner.

Cheers,

Dave.
-- 
Dave Chinner
david@...morbit.com

Powered by blists - more mailing lists