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

Ensure arc_size_break are filled in arc_summary.py #6770

Merged
merged 1 commit into from
Oct 23, 2017

Conversation

dinatale2
Copy link
Contributor

@dinatale2 dinatale2 commented Oct 19, 2017

Description

When printing output in arc_summary, all possible keys
are expected in the output data structure. For the arc
size breakdown, output is conditionally filled and does
not expect arc target size to be equal to the arc size.

Motivation and Context

Just a small bug fix.

How Has This Been Tested?

Locally, on a VM.

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.

@behlendorf
Copy link
Contributor

As mentioned in issue #5526 we really should fix this and directly use mfu_size and mru_size which were added to the arcstats long ago. There's no reason calculate this anymore.

@dinatale2
Copy link
Contributor Author

Updated to resolve issue #5526.

@@ -241,8 +241,7 @@ def get_arc_summary(Kstat):
'num': fBytes(mfu_size),
}

elif arc_size < target_size:
mfu_size = (target_size - mru_size)
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd argue that what we really want to report here is the mru/fmu percentages independent of the arc size. In which case the conditional can be removed and this can be calculated as.

mru_per = fPerc(mru_size, mru_size + mfu_size)
mfu_per = fPerc(mfu_size, mru_size + mfu_size)

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label Oct 19, 2017
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
@behlendorf behlendorf added Ready for Testing and removed Status: Work in Progress Not yet ready for general review labels Oct 20, 2017
@codecov
Copy link

codecov bot commented Oct 20, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6770      +/-   ##
==========================================
+ Coverage   74.36%   74.51%   +0.14%     
==========================================
  Files         297      297              
  Lines       94371    94371              
==========================================
+ Hits        70181    70317     +136     
+ Misses      24190    24054     -136
Flag Coverage Δ
#kernel 74.55% <ø> (+0.27%) ⬆️
#user 66.76% <ø> (+0.35%) ⬆️
Impacted Files Coverage Δ
cmd/zed/agents/zfs_mod.c 65.61% <0%> (-3.16%) ⬇️
module/zfs/spa_errlog.c 90% <0%> (-2.31%) ⬇️
module/zfs/zfs_rlock.c 88.93% <0%> (-1.64%) ⬇️
module/zfs/dsl_userhold.c 88.49% <0%> (-1.2%) ⬇️
module/zfs/vdev_queue.c 96.02% <0%> (-0.73%) ⬇️
module/zfs/spa.c 82.87% <0%> (-0.53%) ⬇️
module/zfs/dsl_dir.c 80.12% <0%> (-0.26%) ⬇️
module/zfs/dsl_destroy.c 93.83% <0%> (-0.23%) ⬇️
module/zfs/dmu_objset.c 90.11% <0%> (-0.18%) ⬇️
module/zfs/zap.c 67.35% <0%> (-0.18%) ⬇️
... and 34 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 6044cf5...72f98d0. Read the comment docs.

@behlendorf
Copy link
Contributor

@AndCycle @richardelling it'd be great to get your thoughts on this little bit of cleanup. The notion here is to simply update the script to give you an authoritative breakdown of the mru/mfu split. Nothing fancy.

@richardelling
Copy link
Contributor

Agree, let's use the metrics at our disposal. +1

@AndCycle
Copy link

LGTM

@behlendorf behlendorf merged commit 64b8c58 into openzfs:master Oct 23, 2017
@behlendorf behlendorf added this to PR Needed for 0.7.4 in 0.7.4 Oct 23, 2017
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Nov 4, 2017
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: AndCycle <andcycle@andcycle.idv.tw>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#5526
Closes openzfs#6770
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Nov 6, 2017
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: AndCycle <andcycle@andcycle.idv.tw>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#5526
Closes openzfs#6770
@dinatale2 dinatale2 deleted the arcsummary-fix branch November 9, 2017 23:53
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 21, 2017
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: AndCycle <andcycle@andcycle.idv.tw>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#5526 
Closes openzfs#6770
@behlendorf behlendorf moved this from PR Needed for 0.7.4 to Merged to 0.7.4 in 0.7.4 Dec 12, 2017
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Jan 29, 2018
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: AndCycle <andcycle@andcycle.idv.tw>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#5526
Closes openzfs#6770
Nasf-Fan pushed a commit to Nasf-Fan/zfs that referenced this pull request Feb 13, 2018
Use mfu_size and mru_size pulled from the arcstats
kstat file to calculate the mfu and mru percentages
for arc size breakdown.

Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Richard Elling <Richard.Elling@RichardElling.com>
Reviewed-by: AndCycle <andcycle@andcycle.idv.tw>
Signed-off-by: Giuseppe Di Natale <dinatale2@llnl.gov>
Closes openzfs#5526
Closes openzfs#6770
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
0.7.4
Merged to 0.7.4
Development

Successfully merging this pull request may close these issues.

None yet

5 participants