Fix ARC hit rate #6989

behlendorf · 2017-12-21T18:51:11Z

Description

When the compressed ARC feature was added in commit d3c2ae1 the method of reference counting in the ARC was modified. As part of this accounting change the arc_buf_add_ref() function was removed entirely.

This would have been fine but the arc_buf_add_ref() function served a second undocumented purpose of updating the ARC access information when taking a hold on a dbuf. Without this logic in place a cached dbuf would not migrate its associated arc_buf_hdr_t to the MFU list. This negatively impacts the ARC hit rate, particularly on systems with a small ARC.

This change reinstates the missing call to arc_access() from dbuf_hold() by implementing a new arc_buf_access() function.

Motivation and Context

As reported in #6171 a significant drop in the ARC hit rate was reported by users upgrading to the 0.7.x releases.

How Has This Been Tested?

Manually with the following trivial test case which reads a file twice. After the first read the file size should be reflected in the arcstat mru_size. After the second read the file size should have been migrated to the arcstat mfu_size.

Notice that when testing without the patch the mfu_size doesn't increase on the second read.

$ zfs mount -a
$ grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    412672
mfu_size                        4    739328

$ cat /tank/file >/dev/null
 grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    21483008
mfu_size                        4    786432

$ cat /tank/file >/dev/null
 grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    21483008
mfu_size                        4    786432

After applying this patch the behavior is once again as expected. This matches the long standing behavior from the zfs-0.6.5.x releases.

$ zfs mount -a
$ grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    415232
mfu_size                        4    737792

$  cat /tank/file >/dev/null
$ grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    21384192
mfu_size                        4    889856

$ cat /tank/file >/dev/null
$ grep -E 'mru_size|mfu_size' /proc/spl/kstat/zfs/arcstats
mru_size                        4    1084928
mfu_size                        4    21198336

Additional testing included a full local run of the ZTS.

@grwilson since this code was also dropped from OpenZFS I believe it suffers from a similar regression. Can you please review this.

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:

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.
Change has been approved by a ZFS on Linux member.

tonyhutter · 2017-12-21T20:29:01Z

module/zfs/arc.c

+		return;
+	}
+
+	mutex_exit(&buf->b_evict_lock);


lock b_evict_lock
lock hash_lock
unlock b_evict_lock
unlock hash_lock

I assume this is deadlock safe?

Yes it should be, the same ordering is used throughout the ARC.

sempervictus · 2017-12-22T01:57:07Z

Zloop seems happy, small vms with commensurately small ARCs show an improvement. I also havent seen one instance yet of the weird ARC collapse (ignoring min/max) syndrome observed on a number of systems since 0.7.0. Gonna fast track it to the staging boxes as i cant see a way this would hurt anything other than maybe some unforseen memory conditions leading to crash, but not eating the data (which is fine in staging).

grwilson · 2017-12-22T15:49:26Z

module/zfs/arc.c

+	hash_lock = HDR_LOCK(buf->b_hdr);
+	mutex_enter(hash_lock);
+
+	if (!HDR_IN_HASH_TABLE(hdr)) {


Did you actually hit this case while testing? I'm trying to understand how a dbuf with data associated with it would not be in the hash table. I don't think we can evict it prior to someone calling remove_reference

Yes, I did. It comes up pretty quickly when running the ZTS. What I think can happen here is we start with a dbuf which has data in it but before we can take the hash lock it gets released. This appears to be possible because I'm using HDR_LOCK to get the hash lock and buf_discard_identity() is called outside the b_evict_lock in arc_released(). I'd love a better solution for this if you have one.

dweeezil

@behlendorf I'm on vacation now and not in a position to give this a proper review [EDIT: until later in the week] but the rationale in the commit comment seems perfectly reasonable to me.

sempervictus · 2017-12-25T18:56:55Z

We ended up tossing this PR into a prod fixup patch cycle when we hit an unrelated kernel bug (skipped staging on those hosts). There were a couple months of other commits in the branch which passed test and stage, so I can't quantify how much performance gain this gave us, but arc isn't collapsing, isn't ignoring min or max, and seems to be running great. Will toss up a note if we see something evil, but as of now, I'm a thumbs up on this.

When the compressed ARC feature was added in commit d3c2ae1 the method of reference counting in the ARC was modified. As part of this accounting change the arc_buf_add_ref() function was removed entirely. This would have be fine but the arc_buf_add_ref() function served a second undocumented purpose of updating the ARC access information when taking a hold on a dbuf. Without this logic in place a cached dbuf would not migrate its associated arc_buf_hdr_t to the MFU list. This would negatively impact the ARC hit rate, particularly on systems with a small ARC. This change reinstates the missing call to arc_access() from dbuf_hold() by implementing a new arc_buf_access() function. Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Issue openzfs#6171

dinatale2 · 2017-12-28T05:21:40Z

module/zfs/arc.c

+	 * to handle the case where it is concurrently being released.
+	 */
+	if (hdr->b_l1hdr.b_state == arc_anon || HDR_EMPTY(hdr)) {
+		mutex_exit(&buf->b_evict_lock);


Any reason why arcstat_access_skip is not being bumped here? It's the same condition as below and results in an early exit.

In my local testing I did add an additional kstat here in order to determine if including this additional check was worthwhile. Keep in mind this check could be dropped entirely since it's safe to rely on the one under the mutex. Based on my test results 99.99+% of the released headers were caught here without the need to take the hash lock, so I opted to keep it as an optimization to minimize any potential lock contention. After determining that I was only really interested in how many slipped through since that was the exceptional case. So the value is only bumped in the second conditional.

codecov · 2017-12-28T06:30:31Z

Codecov Report

Merging #6989 into master will decrease coverage by 0.12%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #6989      +/-   ##
==========================================
- Coverage   75.49%   75.36%   -0.13%     
==========================================
  Files         296      296              
  Lines       95500    95520      +20     
==========================================
- Hits        72095    71990     -105     
- Misses      23405    23530     +125

Flag	Coverage Δ
#kernel	`74.51% <100%> (-0.34%)`	⬇️
#user	`67.66% <95.23%> (-0.21%)`	⬇️

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 823d48b...e0014d5. Read the comment docs.

dweeezil

LGTM, however, ~~I'm not sure why it's so important to update the access information in this context and will be posting additional follow-up outside of the review context~~. [EDIT] Apparently it's the introduction of the dbuf cache.

dweeezil · 2017-12-30T15:37:24Z

As I alluded to in my review comment, its kind of a shame this type of interface is needed in the first place but at least it's a better documented interface with this patch. As @behlendorf pointed out, the arc_access() call was rather "buried" in arc_buf_add_ref() without any specific commentary.

When the compressed ARC feature was added in commit d3c2ae1 the method of reference counting in the ARC was modified. As part of this accounting change the arc_buf_add_ref() function was removed entirely. This would have be fine but the arc_buf_add_ref() function served a second undocumented purpose of updating the ARC access information when taking a hold on a dbuf. Without this logic in place a cached dbuf would not migrate its associated arc_buf_hdr_t to the MFU list. This would negatively impact the ARC hit rate, particularly on systems with a small ARC. This change reinstates the missing call to arc_access() from dbuf_hold() by implementing a new arc_buf_access() function. Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tim Chase <tim@chase2k.com> Reviewed by: George Wilson <george.wilson@delphix.com> Reviewed-by: George Melikov <mail@gmelikov.ru> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#6171 Closes openzfs#6852 Closes openzfs#6989

When the compressed ARC feature was added in commit d3c2ae1 the method of reference counting in the ARC was modified. As part of this accounting change the arc_buf_add_ref() function was removed entirely. This would have be fine but the arc_buf_add_ref() function served a second undocumented purpose of updating the ARC access information when taking a hold on a dbuf. Without this logic in place a cached dbuf would not migrate its associated arc_buf_hdr_t to the MFU list. This would negatively impact the ARC hit rate, particularly on systems with a small ARC. This change reinstates the missing call to arc_access() from dbuf_hold() by implementing a new arc_buf_access() function. Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tim Chase <tim@chase2k.com> Reviewed by: George Wilson <george.wilson@delphix.com> Reviewed-by: George Melikov <mail@gmelikov.ru> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes #6171 Closes #6852 Closes #6989

When the compressed ARC feature was added in commit d3c2ae1 the method of reference counting in the ARC was modified. As part of this accounting change the arc_buf_add_ref() function was removed entirely. This would have be fine but the arc_buf_add_ref() function served a second undocumented purpose of updating the ARC access information when taking a hold on a dbuf. Without this logic in place a cached dbuf would not migrate its associated arc_buf_hdr_t to the MFU list. This would negatively impact the ARC hit rate, particularly on systems with a small ARC. This change reinstates the missing call to arc_access() from dbuf_hold() by implementing a new arc_buf_access() function. Reviewed-by: Giuseppe Di Natale <dinatale2@llnl.gov> Reviewed-by: Tony Hutter <hutter2@llnl.gov> Reviewed-by: Tim Chase <tim@chase2k.com> Reviewed by: George Wilson <george.wilson@delphix.com> Reviewed-by: George Melikov <mail@gmelikov.ru> Signed-off-by: Brian Behlendorf <behlendorf1@llnl.gov> Closes openzfs#6171 Closes openzfs#6852 Closes openzfs#6989

behlendorf requested review from dweeezil and dinatale2 December 21, 2017 18:51

behlendorf mentioned this pull request Dec 21, 2017

z_null_int high disk I/O #6171

Closed

tonyhutter reviewed Dec 21, 2017

View reviewed changes

tonyhutter approved these changes Dec 21, 2017

View reviewed changes

dinatale2 approved these changes Dec 21, 2017

View reviewed changes

behlendorf mentioned this pull request Dec 21, 2017

task z_zvol blocked for more than 120 seconds. #6890

Closed

grwilson reviewed Dec 22, 2017

View reviewed changes

dweeezil reviewed Dec 25, 2017

View reviewed changes

behlendorf force-pushed the issue-6171 branch from 48c5f29 to e0014d5 Compare December 27, 2017 18:46

dinatale2 reviewed Dec 28, 2017

View reviewed changes

behlendorf mentioned this pull request Dec 28, 2017

ARC going to 2G regardless of module parameters #6852

Closed

dweeezil approved these changes Dec 30, 2017

View reviewed changes

gmelikov approved these changes Dec 30, 2017

View reviewed changes

kernelOfTruth mentioned this pull request Dec 31, 2017

[buildbot] zfs master 18.11.2017 (0.7.3) + openzfs 8585 + pr6568 + zfetch prefetch + ARC fixes (incl. commit callbacks from tail of list) #7006

Closed

grwilson approved these changes Jan 2, 2018

View reviewed changes

x041 mentioned this pull request Jan 7, 2018

Fix ARC hit rate x041/zfs#1

Merged

behlendorf merged commit 0873bb6 into openzfs:master Jan 8, 2018

kernelOfTruth mentioned this pull request Jan 8, 2018

[buildbot][pt2] zfs master 18.11.2017 (0.7.3) + openzfs 8585 + pr6568 + zfetch prefetch + ARC fixes (incl. commit callbacks from tail of list) #7020

Closed

amotin mentioned this pull request Apr 2, 2018

9433 Fix ARC hit rate openzfs/openzfs#606

Closed

behlendorf deleted the issue-6171 branch April 19, 2021 19:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ARC hit rate #6989

Fix ARC hit rate #6989

behlendorf commented Dec 21, 2017 •

edited

Loading

tonyhutter Dec 21, 2017

behlendorf Dec 21, 2017

sempervictus commented Dec 22, 2017 •

edited

Loading

grwilson Dec 22, 2017

behlendorf Dec 22, 2017

dweeezil left a comment •

edited

Loading

sempervictus commented Dec 25, 2017

dinatale2 Dec 28, 2017

behlendorf Dec 28, 2017

codecov bot commented Dec 28, 2017

dweeezil left a comment •

edited

Loading

dweeezil commented Dec 30, 2017

Fix ARC hit rate #6989

Fix ARC hit rate #6989

Conversation

behlendorf commented Dec 21, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

tonyhutter Dec 21, 2017

Choose a reason for hiding this comment

behlendorf Dec 21, 2017

Choose a reason for hiding this comment

sempervictus commented Dec 22, 2017 • edited Loading

grwilson Dec 22, 2017

Choose a reason for hiding this comment

behlendorf Dec 22, 2017

Choose a reason for hiding this comment

dweeezil left a comment • edited Loading

Choose a reason for hiding this comment

sempervictus commented Dec 25, 2017

dinatale2 Dec 28, 2017

Choose a reason for hiding this comment

behlendorf Dec 28, 2017

Choose a reason for hiding this comment

codecov bot commented Dec 28, 2017

Codecov Report

dweeezil left a comment • edited Loading

Choose a reason for hiding this comment

dweeezil commented Dec 30, 2017

behlendorf commented Dec 21, 2017 •

edited

Loading

sempervictus commented Dec 22, 2017 •

edited

Loading

dweeezil left a comment •

edited

Loading

dweeezil left a comment •

edited

Loading