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

Remove pool io kstats #12212

Merged
merged 1 commit into from
Jun 10, 2021
Merged

Remove pool io kstats #12212

merged 1 commit into from
Jun 10, 2021

Conversation

amotin
Copy link
Member

@amotin amotin commented Jun 9, 2021

This mostly reverts "3537 want pool io kstats" commit of 8 years ago.

From one side this code using pool-wide locks became pretty bad for
performance, creating significant lock contention in I/O pipeline.
From another, there are more efficient ways now to obtain detailed
statistics, while this statistics is illumos-specific and much less
usable on Linux and FreeBSD, reported only via procfs/sysctls.

This commit does not remove KSTAT_TYPE_IO implementation, that may
be removed later together with already unused KSTAT_TYPE_INTR and
KSTAT_TYPE_TIMER.

Closes: #12121

How Has This Been Tested?

On 40-thread FreeBSD system heavy uncached 4KB reads from 8 ZVOLs show dramatic reduction of time spent on vdev queues locks. Time spent inside vdev_queue_*() reduced from 8.7% to 2.7%.

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)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@amotin
Remove pool io kstats
b22a98b 
This mostly reverts "3537 want pool io kstats" commit of 8 years ago.

From one side this code using pool-wide locks became pretty bad for
performance, creating significant lock contention in I/O pipeline.
From another, there are more efficient ways now to obtain detailed
statistics, while this statistics is illumos-specific and much less
usable on Linux and FreeBSD, reported only via procfs/sysctls.

This commit does not remove KSTAT_TYPE_IO implementation, that may
be removed later together with already unused KSTAT_TYPE_INTR and
KSTAT_TYPE_TIMER.

Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
@amotin amotin requested review from behlendorf and ahrens June 9, 2021 20:47
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jun 9, 2021
@behlendorf behlendorf closed this in b0f3e8a Jun 9, 2021
behlendorf pushed a commit that referenced this pull request Jun 9, 2021
@nabijaczleweli @behlendorf
Move properties, parameters, events, and concepts around manual sections
2badb34 
The pages moved as follows:
  zpool-features.{5 => 7}
  spl{-module-parameters.5 => .4}
  zfs{-module-parameters.5 => .4}
  zfs-events.5 => into zpool-events.8
  zfsconcepts.{8 => 7}
  zfsprops.{8 => 7}
  zpoolconcepts.{8 => 7}
  zpoolprops.{8 => 7}

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Co-authored-by: Daniel Ebdrup Jensen <debdrup@FreeBSD.org>
Closes #12149
Closes #12212
behlendorf pushed a commit that referenced this pull request Jun 9, 2021
@nabijaczleweli @behlendorf
Consistentify miscellaneous style on remaining manpages
1b37cc1 
Most notably this fixes the vdev_id(8) non-.Xrs in vdev_id.conf.5

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes #12212
behlendorf pushed a commit that referenced this pull request Jun 9, 2021
@nabijaczleweli @behlendorf
zgenhostid.8: revisit
4b7ed6a 
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes #12212
@behlendorf
Copy link
Contributor

Re-opening, I accidentally referenced the wrong PR (this one) when merging an unrelated change.

@behlendorf behlendorf reopened this Jun 9, 2021
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 10, 2021
@behlendorf behlendorf self-assigned this Jun 10, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
man: use one Makefile, use OpenZFS for .Os
beeadb4 
The prevailing style is to use either nothing, or the originating
organisational umbrella (here: OpenZFS), and these aren't Linux manpages

This also deduplicates the substitution code, and makes adding/removing
sexions simpler in future

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
Move properties, parameters, events, and concepts around manual sections
2e93407 
The pages moved as follows:
  zpool-features.{5 => 7}
  spl{-module-parameters.5 => .4}
  zfs{-module-parameters.5 => .4}
  zfs-events.5 => into zpool-events.8
  zfsconcepts.{8 => 7}
  zfsprops.{8 => 7}
  zpoolconcepts.{8 => 7}
  zpoolprops.{8 => 7}

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Co-authored-by: Daniel Ebdrup Jensen <debdrup@FreeBSD.org>
Closes openzfs#12149
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
Consistentify miscellaneous style on remaining manpages
9c005c9 
Most notably this fixes the vdev_id(8) non-.Xrs in vdev_id.conf.5

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
zgenhostid.8: revisit
6e70a77 
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
@behlendorf behlendorf merged commit 371f88d into openzfs:master Jun 10, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@amotin @behlendorf
Remove pool io kstats
493985b 
This mostly reverts "3537 want pool io kstats" commit of 8 years ago.

From one side this code using pool-wide locks became pretty bad for
performance, creating significant lock contention in I/O pipeline.
From another, there are more efficient ways now to obtain detailed
statistics, while this statistics is illumos-specific and much less
usable on Linux and FreeBSD, reported only via procfs/sysctls.

This commit does not remove KSTAT_TYPE_IO implementation, that may
be removed later together with already unused KSTAT_TYPE_INTR and
KSTAT_TYPE_TIMER.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
man: use one Makefile, use OpenZFS for .Os
0bef46e 
The prevailing style is to use either nothing, or the originating
organisational umbrella (here: OpenZFS), and these aren't Linux manpages

This also deduplicates the substitution code, and makes adding/removing
sexions simpler in future

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
Move properties, parameters, events, and concepts around manual sections
a444efb 
The pages moved as follows:
  zpool-features.{5 => 7}
  spl{-module-parameters.5 => .4}
  zfs{-module-parameters.5 => .4}
  zfs-events.5 => into zpool-events.8
  zfsconcepts.{8 => 7}
  zfsprops.{8 => 7}
  zpoolconcepts.{8 => 7}
  zpoolprops.{8 => 7}

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Co-authored-by: Daniel Ebdrup Jensen <debdrup@FreeBSD.org>
Closes openzfs#12149
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
Consistentify miscellaneous style on remaining manpages
14973b9 
Most notably this fixes the vdev_id(8) non-.Xrs in vdev_id.conf.5

Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@nabijaczleweli @behlendorf
zgenhostid.8: revisit
4e0fff2 
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Ahelenia Ziemiańska <nabijaczleweli@nabijaczleweli.xyz>
Closes openzfs#12212
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jun 10, 2021
@amotin @behlendorf
Remove pool io kstats
efdfb14 
This mostly reverts "3537 want pool io kstats" commit of 8 years ago.

From one side this code using pool-wide locks became pretty bad for
performance, creating significant lock contention in I/O pipeline.
From another, there are more efficient ways now to obtain detailed
statistics, while this statistics is illumos-specific and much less
usable on Linux and FreeBSD, reported only via procfs/sysctls.

This commit does not remove KSTAT_TYPE_IO implementation, that may
be removed later together with already unused KSTAT_TYPE_INTR and
KSTAT_TYPE_TIMER.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12212
@snajpa
Copy link
Contributor

snajpa commented Jun 16, 2021

@amotin "there are more efficient ways now to obtain detailed statistics" ... such as, please?

The only one I can think of involves executing a binary (zpool iostat) and either leaving it running and parsing its output, or launching it over and over again. Where can I find the said more efficient way to access pool io stats?

Thanks! :)

@amotin
Copy link
Member Author

amotin commented Jun 16, 2021

@snajpa There is per-disk statistics on each OS. There is zpool iostat. May be the numbers of zpool iostat can be fetched somehow directly, but I haven't looked. But per-pool statistics summing all vdevs in a way it was implemented was both inefficient in collecting and pretty useless in case of any special vdevs present, since all it can tell is "there is some activity".

@chrisrd chrisrd mentioned this pull request Jul 5, 2021
Open
tle211212 pushed a commit to tle211212/zfs that referenced this pull request Jul 9, 2021
@amotin @tle211212
Remove pool io kstats (openzfs#12212)
0e43884 
This mostly reverts "3537 want pool io kstats" commit of 8 years ago.

From one side this code using pool-wide locks became pretty bad for
performance, creating significant lock contention in I/O pipeline.
From another, there are more efficient ways now to obtain detailed
statistics, while this statistics is illumos-specific and much less
usable on Linux and FreeBSD, reported only via procfs/sysctls.

This commit does not remove KSTAT_TYPE_IO implementation, that may
be removed later together with already unused KSTAT_TYPE_INTR and
KSTAT_TYPE_TIMER.

Reviewed-by: Matthew Ahrens <mahrens@delphix.com>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by: Alexander Motin <mav@FreeBSD.org>
Sponsored-By: iXsystems, Inc.
Closes openzfs#12212
Conflicts:
	module/os/freebsd/spl/spl_kstat.c
@amotin amotin deleted the no_io_history branch August 24, 2021 20:18
@allanjude
Copy link
Contributor

@amotin "there are more efficient ways now to obtain detailed statistics" ... such as, please?

The only one I can think of involves executing a binary (zpool iostat) and either leaving it running and parsing its output, or launching it over and over again. Where can I find the said more efficient way to access pool io stats?

Thanks! :)

Sorry to bring up an old thread, but with the vdev properties feature that got merged last year, you can get these raw counters via zpool get <counters> $pool $vdev
It has both bytes and iops read/write etc.

@reefland reefland mentioned this pull request Jul 3, 2022
Closed
@AceSlash AceSlash mentioned this pull request Aug 29, 2022
Closed
@AceSlash
Copy link

@allanjude : the feature you are talking about is not yet available on ZFS 2.1. So for ZFS 2.1 we have no more file with the io and no way to get them from ZFS.

I don't know, is it just me or should this feature not be removed until there is a working replacement? This breaks monitoring tools.

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 ahrens approved these changes

Assignees

@behlendorf behlendorf

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

Successfully merging this pull request may close these issues.

IO is bottlenecked through a single lock
6 participants
@amotin @behlendorf @snajpa @allanjude @AceSlash @ahrens