Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Target ARC size can get reduced to arc_c_min #8864

Merged
merged 1 commit into from
Jun 12, 2019

Conversation

ahrens
Copy link
Member

@ahrens ahrens commented Jun 6, 2019

Motivation and Context

Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance. We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns
TRUE, and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

Description

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

External-issue: DLPX-59431

Reviewed at Delphix by @tonynguien @pzakha @grwilson

How Has This Been Tested?

Testing mostly on illumos so far. Used at Delphix since July 2018.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • 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:

@ahrens ahrens added Status: Code Review Needed Ready for review and testing Type: Performance Performance improvement or performance problem labels Jun 6, 2019
@behlendorf behlendorf requested a review from dweeezil June 7, 2019 17:13
Copy link
Contributor

@dweeezil dweeezil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final paragraph of the comment does not seem to jive with the code that has been removed:

To fix this, the ARC should reduce arc_c by the requested amount, not necessarily all the way down to arc_size, which may be arbitrarily small.

While it's certainly true that arc_size can be arbitrarily small, however, due to the MAX(), in case of a very small arc_size, the previous code would have likely set arc_c to arc_c_min which is presumably a reasonable value and not itself arbitrarily small.

Also, although technically correct, the "We only call" paragraph could be misleading, implying that the only calls to arc_reduce_target_size() are from within ZFS itself. It's also called indirectly from the kernel as part of the __arc_shrinker_function() shrinker.

It looks like the MAX() in the removed code was added as part of the upstream commit openzfs/openzfs@49e3519a. It would be interesting to know the rationale behind this very old change.

In any case, think the rationale and implementation of this fix is correct because we don't want to be setting arc_c to one of the potentially 2 much lower values. I ran a handful of tests, including forcing the shrinker into action and everything behaved as expected.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 10, 2019
@ahrens
Copy link
Member Author

ahrens commented Jun 11, 2019

@dweeezil

the previous code would have likely set arc_c to arc_c_min

That's right.

which is presumably a reasonable value and not itself arbitrarily small

Unfortunately it is not very reasonable. arc_c_min is 1/32 of all memory, or 32MB, whichever is more. Reducing from a "reasonable" ARC of say around 1/2 all memory down (e.g. 128GB on a 256GB system) to 1/32 of all memory (e.g. 8GB) is epsilon from "arbitrarily small". That said, I'll adjust the commit message to make it more clear that we're talking still talking about getting reduced to arc_c_min (not below).

the "We only call" paragraph could be misleading

I'll remove the "only" to make this more clear.

It looks like the MAX() in the removed code was added as part of the upstream commit openzfs/openzfs@49e3519. It would be interesting to know the rationale behind this very old change.

We can ask the author, @mmaybee, although it has been 13 years...

Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

External-issue: DLPX-59431
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
@mmaybee
Copy link
Contributor

mmaybee commented Jun 11, 2019

Heh, I'll try to dredge up some ancient arc memories...

I believe the intent here was to be as responsive as possible to memory pressure. If arc_shrink() is being called, and the current arc size is already below the new target size, then go ahead and target the current size... but stay above the minimum arc size. The arc target size would grow again if/when memory pressure relaxed. Given the amount of churn in the arc since I wrote this code, I'm not surprised that a little tweaking is necessary now.

@codecov
Copy link

codecov bot commented Jun 12, 2019

Codecov Report

Merging #8864 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8864      +/-   ##
==========================================
+ Coverage   78.64%   78.82%   +0.18%     
==========================================
  Files         382      382              
  Lines      117808   117789      -19     
==========================================
+ Hits        92645    92851     +206     
+ Misses      25163    24938     -225
Flag Coverage Δ
#kernel 79.32% <ø> (-0.06%) ⬇️
#user 67.57% <ø> (+0.61%) ⬆️

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 5662fd5...c267e0e. Read the comment docs.

@behlendorf behlendorf merged commit d9cd66e into openzfs:master Jun 12, 2019
@ahrens ahrens deleted the DLPX-59431 branch June 12, 2019 20:16
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 13, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes openzfs#8864
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 22, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes openzfs#8864
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Aug 23, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes openzfs#8864
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 17, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes openzfs#8864
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 18, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes openzfs#8864
tonyhutter pushed a commit that referenced this pull request Sep 26, 2019
Sometimes the target ARC size is reduced to arc_c_min, which impacts
performance.  We've seen this happen as part of the random_reads
performance regression test, where the ARC size is reduced before the
reads test starts which impacts how long it takes for system to reach
good IOPS performance.

We call arc_reduce_target_size when arc_reap_cb_check() returns TRUE,
and arc_available_memory() is less than arc_c>>arc_shrink_shift.

However, arc_available_memory() could easily be low, even when arc_c is
low, because we can have tons of unused bufs in the abd kmem cache. This
would be especially true just after the DMU requests a bunch of stuff be
evicted from the ARC (e.g. due to "zpool export").

To fix this, the ARC should reduce arc_c by the requested amount, not
all the way down to arc_size (or arc_c_min), which can be very small.

Reviewed-by: Tim Chase <tim@chase2k.com>
Reviewed by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: George Melikov <mail@gmelikov.ru>
Signed-off-by: Matthew Ahrens <mahrens@delphix.com>
External-issue: DLPX-59431
Closes #8864
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested) Type: Performance Performance improvement or performance problem
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants