OpenZFS 9284 - arc_reclaim_thread has 2 jobs #8165

brad-lewis · 2018-11-30T18:19:23Z

Following the fix for 9018 (Replace kmem_cache_reap_now() with
kmem_cache_reap_soon), the arc_reclaim_thread() no longer blocks while
reaping. However, the code is still confusing and error-prone, because
this thread has two responsibilities. We should instead separate this
into two threads each with their own responsibility:

keep arc_size under arc_c, by calling arc_adjust(), which
improves arc_is_overflowing()
keep enough free memory in the system, by calling
arc_kmem_reap_now() plus arc_shrink(), which improves
arc_available_memory().
Furthermore, we can use the zthr infrastructure to separate the "should
we do something" from "do it" parts of the logic, and normalize the
start up / shut down of the threads.

Authored by: Brad Lewis brad.lewis@delphix.com
Reviewed by: Matt Ahrens mahrens@delphix.com
Reviewed by: Serapheim Dimitropoulos serapheim@delphix.com
Reviewed by: Pavel Zakharov pavel.zakharov@delphix.com
Reviewed by: Dan Kimmel dan.kimmel@delphix.com
Reviewed by: Paul Dagnelie pcd@delphix.com
Reviewed by: Dan McDonald danmcd@joyent.com
Reviewed by: Tim Kordas tim.kordas@joyent.com
Ported-by: Brad Lewis brad.lewis@delphix.com
OpenZFS-issue: https://www.illumos.org/issues/9284
OpenZFS-commit: openzfs/openzfs@de753e34f9
Signed-off-by: Brad Lewis brad.lewis@delphix.com

Motivation and Context

Following the fix for 9018 (Replace kmem_cache_reap_now() with
kmem_cache_reap_soon), the arc_reclaim_thread() no longer blocks while
reaping. However, the code is still confusing and error-prone, because
this thread has two responsibilities.

Description

We should instead separate this
into two threads each with their own responsibility:

keep arc_size under arc_c, by calling arc_adjust(), which
improves arc_is_overflowing()
keep enough free memory in the system, by calling
arc_kmem_reap_now() plus arc_shrink(), which improves
arc_available_memory().
Furthermore, we can use the zthr infrastructure to separate the "should
we do something" from "do it" parts of the logic, and normalize the
start up / shut down of the threads.

How Has This Been Tested?

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
[X ] Performance enhancement (non-breaking change which improves efficiency)
[X ] Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
I have read the contributing document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.

behlendorf

I'm a little concerned above moving this to zthr given the racy behavior we've seen thus far. See #8087 and #8077. It's a long running thread so I doubt we'll see issue, but we should address the known outstanding zthr problems.

Do we have any testing results which show if this helped or hurt ARC cache performance?

module/zfs/zthr.c

module/zfs/arc.c

brad-lewis · 2018-12-07T23:42:25Z

I made the changes to the code that Brian suggested. Thank you Brian.

I don't have any linux performance data to justify this request. The initial performance issues that led to these openzfs changes have already addressed. As set out in my initial comment, the value in this change is to simplify the code and make it more understandable.

I'll look at the zthr issues mentioned but I agree that since this code has been running for some time at Delphix and in openzfs with no issues reported that it is unlikely for there to be a problem.

codecov · 2018-12-08T09:47:48Z

Codecov Report

Merging #8165 into master will increase coverage by 0.05%.
The diff coverage is 96.51%.

@@            Coverage Diff             @@
##           master    #8165      +/-   ##
==========================================
+ Coverage   78.45%   78.51%   +0.05%     
==========================================
  Files         379      379              
  Lines      114898   114909      +11     
==========================================
+ Hits        90144    90218      +74     
+ Misses      24754    24691      -63

Flag	Coverage Δ
#kernel	`78.9% <96.38%> (-0.06%)`	⬇️
#user	`67.37% <85.33%> (-0.14%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c66401f...79a9242. Read the comment docs.

behlendorf

Looks good. I only have two remaining concerns.

There are a couple style errors accidentally introduced.

./lib/libzpool/kernel.c: 1282: indent by spaces instead of tabs
./lib/libzpool/kernel.c: 1282: non-continuation indented 4 spaces

The zthr dependency I mentioned before. I'm OK with it as is for the safe of code consistency, but this is something we're going to need to revisit when addressing the known ztest issue.

behlendorf · 2018-12-11T20:51:00Z

@dweeezil would you mind reviewing this change too. It's primarily intended to keep us in sync, but we should also benefit by splitting this in to two threads. Though it is less of an issue for us.

dweeezil · 2018-12-13T23:16:16Z

@behlendorf I'll review it this evening.

dweeezil

This LGTM with the caveat that I've not actually tested it (which I'll do today). Let's pull in the log message text from openzfs/openzfs#590 (openzfs/openzfs@fe75174).

Unfortunately although it looks good, it seems to introduce a regression that causes arc_c collapse under simple load-testing.

dweeezil · 2018-12-14T13:47:11Z

module/zfs/arc.c

@@ -366,7 +383,7 @@ static int		arc_min_prescient_prefetch_ms;
 */
 int arc_lotsfree_percent = 10;

-static int arc_dead;
+static boolean_t arc_initialized;


As long as we're renaming this one, how about adding a comment up here.

dweeezil · 2018-12-14T13:48:19Z

module/zfs/arc.c

+	 * becoming implicitly blocked by a system-wide kmem reap -- which,
+	 * on a system with many, many full magazines, can take minutes).
+	 */
+	if (!spl_kmem_cache_reap_active() && free_memory < 0) {


Would it make sense to define kmem_cache_reap_active in terms of spl_kmem_cache_reap_active and then keep this line formatted identically to the upstream?

That does make sense. Then it seemed to make sense to dro the spl prefix from the libzpool stub.

dweeezil · 2018-12-16T16:46:11Z

As I mentioned to @behlendorf via Slack, there was a regression caused by this which would cause the ARC to collapse to its minimum size. My fixes are in a19e1df but I'll also add comments to this PR which correlate to those fixes.

dweeezil · 2018-12-16T16:48:00Z

module/zfs/arc.c

 #ifdef _KERNEL
-				to_free = MAX(to_free, need_free);
+		to_free = MAX(to_free, ptob(arc_need_free));


Change ptob(arc_need_free) to arc_need_free here.

dweeezil · 2018-12-16T16:49:21Z

module/zfs/arc.c

-			arc_ksp->ks_update(arc_ksp, KSTAT_READ);
-#endif
-		mutex_exit(&arc_reclaim_lock);
+		cv_broadcast(&arc_adjust_waiters_cv);


Somewhere along this call path we need to clear arc_need_free to prevent continuous reduction of arc_c.

Thanks @dweezil.

behlendorf · 2018-12-17T18:18:05Z

@dweeezil thanks for catching that arc collapse issue and running it down. Your proposed fix makes sense. @brad-lewis can you add the fix to this PR.

Authored by: Brad Lewis <brad.lewis@delphix.com> Reviewed by: Matt Ahrens <mahrens@delphix.com> Reviewed by: Serapheim Dimitropoulos <serapheim@delphix.com> Reviewed by: Pavel Zakharov <pavel.zakharov@delphix.com> Reviewed by: Dan Kimmel <dan.kimmel@delphix.com> Reviewed by: Paul Dagnelie <pcd@delphix.com> Reviewed by: Dan McDonald <danmcd@joyent.com> Reviewed by: Tim Kordas <tim.kordas@joyent.com> Ported-by: Brad Lewis <brad.lewis@delphix.com> Signed-off-by: Brad Lewis <brad.lewis@delphix.com> OpenZFS-issue: https://www.illumos.org/issues/9284 OpenZFS-commit: openzfs/openzfs@de753e34f9

behlendorf added the Status: Code Review Needed Ready for review and testing label Nov 30, 2018

behlendorf requested changes Dec 5, 2018

View reviewed changes

behlendorf approved these changes Dec 10, 2018

View reviewed changes

behlendorf requested a review from dweeezil December 11, 2018 20:49

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 13, 2018

dweeezil approved these changes Dec 14, 2018

View reviewed changes

behlendorf added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Accepted Ready to integrate (reviewed, tested) labels Dec 14, 2018

dweeezil suggested changes Dec 16, 2018

View reviewed changes

behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels Dec 18, 2018

dweeezil approved these changes Dec 22, 2018

View reviewed changes

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Dec 26, 2018

behlendorf closed this in 3ec34e5 Dec 26, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenZFS 9284 - arc_reclaim_thread has 2 jobs #8165

OpenZFS 9284 - arc_reclaim_thread has 2 jobs #8165

brad-lewis commented Nov 30, 2018

behlendorf left a comment

brad-lewis commented Dec 7, 2018

codecov bot commented Dec 8, 2018 •

edited

Loading

behlendorf left a comment

behlendorf commented Dec 11, 2018

dweeezil commented Dec 13, 2018

dweeezil left a comment •

edited

Loading

dweeezil Dec 14, 2018

dweeezil Dec 14, 2018

brad-lewis Dec 14, 2018

dweeezil commented Dec 16, 2018

dweeezil Dec 16, 2018

dweeezil Dec 16, 2018

brad-lewis Dec 18, 2018

behlendorf commented Dec 17, 2018

OpenZFS 9284 - arc_reclaim_thread has 2 jobs #8165

OpenZFS 9284 - arc_reclaim_thread has 2 jobs #8165

Conversation

brad-lewis commented Nov 30, 2018

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

behlendorf left a comment

Choose a reason for hiding this comment

brad-lewis commented Dec 7, 2018

codecov bot commented Dec 8, 2018 • edited Loading

Codecov Report

behlendorf left a comment

Choose a reason for hiding this comment

behlendorf commented Dec 11, 2018

dweeezil commented Dec 13, 2018

dweeezil left a comment • edited Loading

Choose a reason for hiding this comment

dweeezil Dec 14, 2018

Choose a reason for hiding this comment

dweeezil Dec 14, 2018

Choose a reason for hiding this comment

brad-lewis Dec 14, 2018

Choose a reason for hiding this comment

dweeezil commented Dec 16, 2018

dweeezil Dec 16, 2018

Choose a reason for hiding this comment

dweeezil Dec 16, 2018

Choose a reason for hiding this comment

brad-lewis Dec 18, 2018

Choose a reason for hiding this comment

behlendorf commented Dec 17, 2018

codecov bot commented Dec 8, 2018 •

edited

Loading

dweeezil left a comment •

edited

Loading