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

Jump to bottom

Zpool iostat: remove latency/queue scaling (#7694) #7945

Merged
merged 1 commit into from Sep 25, 2018
Merged

Zpool iostat: remove latency/queue scaling (#7694) #7945

merged 1 commit into from Sep 25, 2018

Conversation

GregorKopka
Copy link
Contributor

Motivation and Context

Issue 7694 - zpool iostat figures for -l latency and -q queue depth are wrong with duration != 1.

Description

Removed the scaling for latency and queue depth as these have no temporal component.

How Has This Been Tested?

Visually. Please review.

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.

Zpool iostat: remove latency/queue scaling (openzfs#7694)
9e9b0d2 
Bandwidth and iops are average per second while *_wait are averages
per request for latency or, for queue depths, an instantaneous
measurement at the end of an interval (according to man zpool).

When calculating the first two it makes sense to do
x/interval_duration (x being the increase in total bytes or number of
requests over the duration of the interval, interval_duration in
seconds) to 'scale' from amount/interval_duration to amount/second.

But applying the same math for the latter (*_wait latencies/queue) is
wrong as there is no interval_duration component in the values (these
are time/requests to get to average_time/request or already an
absulute number).

This bug leads to the only correct continuous *_wait figures for both
latencies and queue depths from 'zpool iostat -l/q' being with
duration=1 as then the wrong math cancels itself (x/1 is a nop).

This removes temporal scaling from latency and queue depth figures.
Closes: openzfs#7694

Signed-off-by: Gregor Kopka <gregor@kopka.net>
@codecov
Copy link

codecov bot commented Sep 23, 2018 •
edited

Codecov Report

Merging #7945 into master will increase coverage by 0.03%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7945      +/-   ##
==========================================
+ Coverage   78.43%   78.46%   +0.03%     
==========================================
  Files         376      376              
  Lines      114017   114017              
==========================================
+ Hits        89424    89469      +45     
+ Misses      24593    24548      -45
Flag Coverage Δ
#kernel 78.81% <ø> (-0.04%) ⬇️
#user 67.42% <100%> (-0.03%) ⬇️

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 dda5500...9e9b0d2. Read the comment docs.

tonyhutter
tonyhutter approved these changes Sep 24, 2018
View reviewed changes
Copy link
Contributor

@tonyhutter tonyhutter left a comment

Choose a reason for hiding this comment

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

Thanks for this patch. I ran an IO test and did a side-by side comparison of the unpatched and patched versions. I saw the patched version gave sane values for interval != 1, and also confirmed the patched version's latency averages matched what I saw in the histograms.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Sep 24, 2018
@tonyhutter tonyhutter added this to To do in 0.7.12 Sep 25, 2018
@behlendorf behlendorf merged commit b954e36 into openzfs:master Sep 25, 2018
@tonyhutter tonyhutter moved this from To do to In progress in 0.7.12 Oct 26, 2018
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Oct 31, 2018
@GregorKopka @tonyhutter
Zpool iostat: remove latency/queue scaling
77a4ccb 
Bandwidth and iops are average per second while *_wait are averages
per request for latency or, for queue depths, an instantaneous
measurement at the end of an interval (according to man zpool).

When calculating the first two it makes sense to do
x/interval_duration (x being the increase in total bytes or number of
requests over the duration of the interval, interval_duration in
seconds) to 'scale' from amount/interval_duration to amount/second.

But applying the same math for the latter (*_wait latencies/queue) is
wrong as there is no interval_duration component in the values (these
are time/requests to get to average_time/request or already an
absulute number).

This bug leads to the only correct continuous *_wait figures for both
latencies and queue depths from 'zpool iostat -l/q' being with
duration=1 as then the wrong math cancels itself (x/1 is a nop).

This removes temporal scaling from latency and queue depth figures.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gregor Kopka <gregor@kopka.net>
Closes openzfs#7945 
Closes openzfs#7694
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 5, 2018
@GregorKopka @tonyhutter
Zpool iostat: remove latency/queue scaling
3cf583a 
Bandwidth and iops are average per second while *_wait are averages
per request for latency or, for queue depths, an instantaneous
measurement at the end of an interval (according to man zpool).

When calculating the first two it makes sense to do
x/interval_duration (x being the increase in total bytes or number of
requests over the duration of the interval, interval_duration in
seconds) to 'scale' from amount/interval_duration to amount/second.

But applying the same math for the latter (*_wait latencies/queue) is
wrong as there is no interval_duration component in the values (these
are time/requests to get to average_time/request or already an
absulute number).

This bug leads to the only correct continuous *_wait figures for both
latencies and queue depths from 'zpool iostat -l/q' being with
duration=1 as then the wrong math cancels itself (x/1 is a nop).

This removes temporal scaling from latency and queue depth figures.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gregor Kopka <gregor@kopka.net>
Closes openzfs#7945 
Closes openzfs#7694
tonyhutter pushed a commit that referenced this pull request Nov 13, 2018
@GregorKopka @tonyhutter
Zpool iostat: remove latency/queue scaling
5f07d51 
Bandwidth and iops are average per second while *_wait are averages
per request for latency or, for queue depths, an instantaneous
measurement at the end of an interval (according to man zpool).

When calculating the first two it makes sense to do
x/interval_duration (x being the increase in total bytes or number of
requests over the duration of the interval, interval_duration in
seconds) to 'scale' from amount/interval_duration to amount/second.

But applying the same math for the latter (*_wait latencies/queue) is
wrong as there is no interval_duration component in the values (these
are time/requests to get to average_time/request or already an
absulute number).

This bug leads to the only correct continuous *_wait figures for both
latencies and queue depths from 'zpool iostat -l/q' being with
duration=1 as then the wrong math cancels itself (x/1 is a nop).

This removes temporal scaling from latency and queue depth figures.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gregor Kopka <gregor@kopka.net>
Closes #7945
Closes #7694
GregorKopka added a commit to GregorKopka/zfs that referenced this pull request Jan 7, 2019
@GregorKopka
Zpool iostat: remove latency/queue scaling
fa8981a 
Bandwidth and iops are average per second while *_wait are averages
per request for latency or, for queue depths, an instantaneous
measurement at the end of an interval (according to man zpool).

When calculating the first two it makes sense to do
x/interval_duration (x being the increase in total bytes or number of
requests over the duration of the interval, interval_duration in
seconds) to 'scale' from amount/interval_duration to amount/second.

But applying the same math for the latter (*_wait latencies/queue) is
wrong as there is no interval_duration component in the values (these
are time/requests to get to average_time/request or already an
absulute number).

This bug leads to the only correct continuous *_wait figures for both
latencies and queue depths from 'zpool iostat -l/q' being with
duration=1 as then the wrong math cancels itself (x/1 is a nop).

This removes temporal scaling from latency and queue depth figures.

Reviewed-by: Tony Hutter <hutter2@llnl.gov>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Gregor Kopka <gregor@kopka.net>
Closes openzfs#7945 
Closes openzfs#7694
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

@tonyhutter tonyhutter tonyhutter approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
No open projects
0.7.12
  
In progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@GregorKopka @behlendorf @tonyhutter