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

zpool iostat should print headers when terminal fills #8262

Merged
merged 1 commit into from Jan 23, 2019

Conversation

Projects
None yet
6 participants
@madwizard
Copy link
Contributor

commented Jan 10, 2019

When zpool iostat fills terminal headers should be printed again.
zpool iostat -n will supress this.

Signed-off-by: Damian Wojsław damian@wojslaw.pl
Closes: #8235
Issue: #8235

Motivation and Context

#8235

Description

zpool_do_iostat:

  • added new int variable that holds default number of rows (24) (winheight),
  • find out the actual number of rows terminal has using ioctl and winsize,
  • every time number of printed rows (tracked by cb.cb_iteration) % winheight == 0, print the header
  • unless -n flag was specified, which reverts to original, one time print of headers

How Has This Been Tested?

run zpool iostat 1 in terminal of various heights,
run zpool iostat -n 1 in terminal of various heights,
run zpool iostat -H 1 in terminal of various heights,
run zpool iostat -H -n 1 in terminal f various heights,
run zpool iostat -H -n
run zpool iostat -n
run zpool iostat -H
Ran the same tests with numbers different than 1.

Ubuntu 18.10, kernel 4.5.0-43-generic

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:

Show resolved Hide resolved cmd/zpool/zpool_main.c Outdated
Show resolved Hide resolved man/man8/zpool.8 Outdated

@madwizard madwizard force-pushed the madwizard:8246 branch from 650e63e to e4201bd Jan 10, 2019

@behlendorf
Copy link
Member

left a comment

Thanks, just a few nits.

Show resolved Hide resolved cmd/zpool/zpool_main.c Outdated
Show resolved Hide resolved cmd/zpool/zpool_main.c Outdated
Show resolved Hide resolved man/man8/zpool.8 Outdated
Show resolved Hide resolved man/man8/zpool.8 Outdated
Show resolved Hide resolved man/man8/zpool.8 Outdated
Show resolved Hide resolved man/man8/zpool.8

@madwizard madwizard force-pushed the madwizard:8246 branch from e4201bd to 06f9706 Jan 10, 2019

@madwizard

This comment has been minimized.

Copy link
Contributor Author

commented Jan 10, 2019

I see tests and builds fail, but as far as I understand outputs, those are not related to my changes.

Show resolved Hide resolved cmd/zpool/zpool_main.c Outdated
@codecov

This comment has been minimized.

Copy link

commented Jan 11, 2019

Codecov Report

Merging #8262 into master will decrease coverage by 0.02%.
The diff coverage is 70%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8262      +/-   ##
==========================================
- Coverage   78.46%   78.44%   -0.03%     
==========================================
  Files         380      380              
  Lines      115654   115662       +8     
==========================================
- Hits        90747    90730      -17     
- Misses      24907    24932      +25
Flag Coverage Δ
#kernel 78.9% <ø> (-0.08%) ⬇️
#user 67.25% <70%> (-0.02%) ⬇️

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 8bd2a28...06f9706. Read the comment docs.

@codecov

This comment has been minimized.

Copy link

commented Jan 11, 2019

Codecov Report

Merging #8262 into master will decrease coverage by 0.03%.
The diff coverage is 57.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8262      +/-   ##
==========================================
- Coverage   78.49%   78.46%   -0.04%     
==========================================
  Files         380      380              
  Lines      115668   115666       -2     
==========================================
- Hits        90794    90752      -42     
- Misses      24874    24914      +40
Flag Coverage Δ
#kernel 78.84% <ø> (-0.09%) ⬇️
#user 67.38% <57.14%> (-0.06%) ⬇️

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 9b626c1...dae176c. Read the comment docs.

@madwizard madwizard force-pushed the madwizard:8246 branch 3 times, most recently from 67bef00 to c420d5e Jan 11, 2019

@madwizard madwizard force-pushed the madwizard:8246 branch from c420d5e to 9d03425 Jan 14, 2019

zpool iostat should print headers when terminal fills
When zpool iostat fills terminal headers should be printed again.
zpool iostat -n will supress this.

If command is not attached to tty it will not implement new
behavior, to not break old scripts.

Signed-off-by: Damian Wojsław <damian@wojslaw.pl>
Closes: #8235
Issue: #8235

@madwizard madwizard force-pushed the madwizard:8246 branch from 9d03425 to dae176c Jan 15, 2019

@madwizard

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2019

@behlendorf What's left for this PR that I should do?

@behlendorf

This comment has been minimized.

Copy link
Member

commented Jan 18, 2019

@madwizard this is ready to go, and I was about to post that this was only blocked by allowing all the OpenZFS developers a reasonable time to comment (7 days, which it's now been). @jclulow would you like to give this a final look?

@jclulow

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

Hi! With the change for checking if stdout is a TTY, this seems fine to me! Thanks for checking, and please pardon my latency in getting back to this.

@behlendorf

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

@jclulow thanks! Then I'll go ahead and get this integrated.

@behlendorf behlendorf merged commit 8fccfa8 into zfsonlinux:master Jan 23, 2019

21 of 24 checks passed

buildbot/Kernel.org Built-in x86_64 (BUILD) Build done.
Details
buildbot/Ubuntu 17.04 x86_64 Coverage (TEST) Build done.
Details
codecov/patch 57.14% of diff hit (target 78.49%)
Details
buildbot/Amazon 2 x86_64 (BUILD) Build done.
Details
buildbot/Amazon 2 x86_64 Release (TEST) Build done.
Details
buildbot/CentOS 6 x86_64 (BUILD) Build done.
Details
buildbot/CentOS 6 x86_64 (TEST) Build done.
Details
buildbot/CentOS 7 x86_64 (BUILD) Build done.
Details
buildbot/CentOS 7 x86_64 (TEST) Build done.
Details
buildbot/Debian 8 arm (BUILD) Build done.
Details
buildbot/Debian 8 ppc (BUILD) Build done.
Details
buildbot/Debian 8 ppc64 (BUILD) Build done.
Details
buildbot/Debian 9 x86_64 (BUILD) Build done.
Details
buildbot/Debian 9 x86_64 (TEST) Build done.
Details
buildbot/Fedora 29 x86_64 (BUILD) Build done.
Details
buildbot/Fedora 29 x86_64 (TEST) Build done.
Details
buildbot/Ubuntu 16.04 aarch64 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 i386 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 x86_64 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 x86_64 (TEST) Build done.
Details
buildbot/Ubuntu 17.10 x86_64 (STYLE) Build done.
Details
buildbot/Ubuntu 18.04 x86_64 (BUILD) Build done.
Details
buildbot/Ubuntu 18.04 x86_64 (TEST) Build done.
Details
codecov/project 78.46% (-0.04%) compared to 9b626c1
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.