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.

Sign up for GitHub

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

Jump to bottom

Restore ARC MFU/MRU pressure #10618

Merged
merged 1 commit into from
Aug 12, 2020
Merged

Restore ARC MFU/MRU pressure #10618

merged 1 commit into from
Aug 12, 2020

Conversation

mattmacy
Copy link
Contributor

The arc_adapt() function tunes LRU/MLU balance according to 4 types of cache
hits (which is passed as state agrument): ghost LRU, LRU, MRU, ghost MRU.
If this function is called with wrong cache hit (state), adaptation will be
sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before call
to arc_access() which revive element in cache and change state from ghost to
real hit.

After this commit, the order of calls was reverted and arc_adapt() is now
called only with «real» hits even if hit was in one of two ghost lists,
which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc code.
See also issue #10548.

Signed-off-by: Matt Macy [email protected]

Motivation and Context

Description

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)
  • 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.
  • I have run the ZFS Test Suite with this change applied.
  • All commit messages are properly formatted and contain Signed-off-by.

@mattmacy
Restore ARC MFU/MRU pressure
562c6dd 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.
See also issue openzfs#10548.

Signed-off-by: Matt Macy <[email protected]>
@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Jul 23, 2020
@behlendorf behlendorf requested review from ahrens and grwilson July 23, 2020 23:58
@mattmacy mattmacy force-pushed the projects/do_adapt branch from afdd72b to 562c6dd Compare July 24, 2020 00:21
@codecov
Copy link

codecov bot commented Jul 24, 2020 •
edited
Loading

Codecov Report

Merging #10618 into master will increase coverage by 0.10%.
The diff coverage is 88.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10618      +/-   ##
==========================================
+ Coverage   79.58%   79.68%   +0.10%     
==========================================
  Files         393      393              
  Lines      124627   124634       +7     
==========================================
+ Hits        99186    99317     +131     
+ Misses      25441    25317     -124
Flag Coverage Δ
#kernel 80.19% <88.00%> (+0.03%) ⬆️
#user 65.90% <80.76%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
module/zfs/arc.c 88.79% <88.46%> (-0.55%) ⬇️
module/zfs/vdev_raidz.c 88.99% <0.00%> (-3.27%) ⬇️
module/zfs/zil.c 91.98% <0.00%> (-1.14%) ⬇️
lib/libzfs/libzfs_changelist.c 85.01% <0.00%> (-1.13%) ⬇️
module/os/linux/zfs/zio_crypt.c 79.97% <0.00%> (-1.11%) ⬇️
cmd/zed/agents/fmd_serd.c 78.21% <0.00%> (-1.00%) ⬇️
module/os/linux/spl/spl-kmem-cache.c 74.90% <0.00%> (-0.75%) ⬇️
module/zfs/btree.c 83.02% <0.00%> (-0.71%) ⬇️
module/zfs/dmu_traverse.c 96.34% <0.00%> (-0.67%) ⬇️
lib/libzutil/os/linux/zutil_import_os.c 80.06% <0.00%> (-0.67%) ⬇️
... and 44 more

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 b197457...562c6dd. Read the comment docs.

@adamdmoss
Copy link
Contributor

Seems to be behaving well; is a similar fix necessary for l2arc policy?

@chrisrd chrisrd mentioned this pull request Jul 29, 2020
Open
@adamdmoss
Copy link
Contributor

Still behaving rather well, FWIW.

@behlendorf behlendorf self-requested a review August 5, 2020 17:58
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 11, 2020
@mmatuska
Copy link
Contributor

I recommend this PR as a backport candidate to 0.8.x
@behlendorf do you plan introducing a tag to pull requests that are subject to stable-backport?

@behlendorf
Copy link
Contributor

@mmatuska we've been using a GitHub project for tracking backports. I'll merge this and move it in to the project.

@behlendorf behlendorf merged commit e111c80 into openzfs:master Aug 12, 2020
DeHackEd pushed a commit to DeHackEd/zfs that referenced this pull request Aug 19, 2020
@mattmacy @DeHackEd
Restore ARC MFU/MRU pressure
6cc5087 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548
Closes openzfs#10618
DeHackEd pushed a commit to DeHackEd/zfs that referenced this pull request Aug 19, 2020
@mattmacy @DeHackEd
Restore ARC MFU/MRU pressure
6a50aa9 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548
Closes openzfs#10618
This was referenced Aug 19, 2020
Closed
Closed
@mattmacy mattmacy deleted the projects/do_adapt branch August 28, 2020 19:08
@chrisrd chrisrd mentioned this pull request Sep 3, 2020
Open
@gdevenyi gdevenyi mentioned this pull request Sep 11, 2020
Closed
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Dec 9, 2020
@mattmacy @behlendorf
Restore ARC MFU/MRU pressure
d757012 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548
Closes openzfs#10618
Conflicts:
	module/zfs/arc.c
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Dec 9, 2020
@mattmacy @tonyhutter
Restore ARC MFU/MRU pressure
6652562 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548
Closes openzfs#10618
tonyhutter pushed a commit that referenced this pull request Dec 14, 2020
@mattmacy @tonyhutter
Restore ARC MFU/MRU pressure
033c788 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes #10548
Closes #10618
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
@mattmacy @jsai20
Restore ARC MFU/MRU pressure
96b3d00 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548 
Closes openzfs#10618
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
@mattmacy
Restore ARC MFU/MRU pressure
ac7417c 
The arc_adapt() function tunes LRU/MLU balance according to 4 types of
cache hits (which is passed as state agrument): ghost LRU, LRU, MRU,
ghost MRU. If this function is called with wrong cache hit (state),
adaptation will be sub-optimal and performance will suffer.

Some time ago upstream received this commit:

6950 ARC should cache compressed data) in arc_read() do next
sequence (access to ghost buffer)

Before this commit, hit to any ghost list was passed arc_adapt() before
call to arc_access() which revive element in cache and change state from
ghost to real hit.

After this commit, the order of calls was reverted and arc_adapt() is
now called only with «real» hits even if hit was in one of two ghost
lists, which renders ghost lists useless and breaks the ARC algorithm.

FreeBSD fixed this problem locally in Change D19094 / Commit r348772.

This change is an adaptation of the above commit to the current arc
code.

Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Matt Macy <[email protected]>
Closes openzfs#10548 
Closes openzfs#10618
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@behlendorf behlendorf behlendorf approved these changes

@ahrens ahrens Awaiting requested review from ahrens

@grwilson grwilson Awaiting requested review from grwilson

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mattmacy @adamdmoss @mmatuska @behlendorf