On Wed, Aug 04, 2021 at 09:09:16PM +1000, Dave Chinner wrote:

On Tue, Aug 03, 2021 at 08:20:30PM -0700, Darrick J. Wong wrote:

quoted

For everyone else following along at home, I've posted the current draft
version of this whole thing in:

https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/log/?h=deferred-inactivation-5.15

Overall looks good - fixes to freeze problems I hit are found
in other replies to this.

I omitted the commits:

xfs: queue inodegc worker immediately when memory is tight
xfs: throttle inode inactivation queuing on memory reclaim

in my test kernel because I think they are unnecessary.

I think the first is unnecessary because reclaim of inodes from the
VFS is usually in large batches and so early triggers aren't
desirable when we're getting thousands of inodes being evicted by
the superblock shrinker at a time. If we've only got a handful of
inodes queued, then inactivating them early isn't going to make much
of an impact on free memory. I could be wrong, but so far I have no
evidence that expediting inactivation is necessary.

I think this was a lot more necessary under the old design because I let
the number of tagged inodes grow quite large before triggering gc work,
much less throttling anything.  256 is low enough that it should be
manageable.

Does it matter that we no longer inactivate inodes in inode number
order?  I guess it could be nice to be able to dump inode cluster
buffers as soon as practicable, but OTOH I suspect that only matters for
the case of mass deletion, in which case we'll probably catch up soon
enough?

Anyway, I'll try turning both of these off with my silly deltree
exerciser and see what happens.

The second patch is the custom shrinker. Again, I just don't think
this is necessary because if there is any amount of inactivation of
evicted inodes needed due to reclaim, we'll already be triggering it
to run via the deferred queue flush thresholds. Hence we don't
really need any mechanism to tell us that there is memory pressure;
the deferred work reacts to eviction from reclaim in exactly the
same way it reacts to eviction from unlink....

Yep.  I came to the same conclusion last night; it looks like my fast
fstests setup for that passed.

I've been running the patchset without these two patches on my 512MB
test VM, and the only OOM kill I get from fstests is g/531. This is
the "many open-but-unlinked" test, which creates 50,000 open
unlinked files per CPU. So for this test VM which has 4 CPUs, that's
200,000 open, dirty iunlinked inodes and a lot of pinned inode
cluster buffers. At ~2kB of memory per unlinked inode (ignoring the
cluster buffers) this would consume about 400MB of the 512MB of RAM
the VM has. It OOM kills the test programs that hold the open files
long before it gets to 200,000 files, so this test never passed
before this patchset on this machine...

Yeah... I actually tried running fstests on a 512M VM and whooeee did I
see a lot of OOM kills.  Clearly we've all gotten spoiled by cheap DRAM.

I have a couple of extra patches to set up per-cpu hotplug
infrastructure before the deferred inode inactivation patch - I'll
post them after I finish this email. I'm going to leave it running
tests overnight.

Ok, I'll jam those on the front end of the series.

Darrick, I'm pretty happy with the way the patchset is behaving now.
If you want to fold in the bug fixes I've posted and add in
the hotplug patches, then I think it's ready to be posted in full
again (if it all passes your testing) for review.

It's probably about time for that.  Now that we do percpu thingies, I
think it might also be time for a test that runs fstests while plugging
and unplugging the non-bsp processors.

[narrator: ...and thus he unleashed another terrifying bug mountain]

--D

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com