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

spa: document HAVE_SPA_THREAD and use it in more places #15406

Merged
merged 1 commit into from
Oct 23, 2023

Conversation

robn
Copy link
Member

@robn robn commented Oct 16, 2023

Motivation and Context

spa_thread() and the "System Duty Cycle" scheduling class are from Illumos and have not yet been adapted to Linux or FreeBSD.

HAVE_SPA_THREAD has long been explicitly undefined and used to mark spa_thread(), but there's some related taskq code that can never be invoked without it, which makes some already-tricky code harder to read.

HAVE_SYSDC is introduced in this commit to mark the SDC parts. SDC requires spa_thread(), but the inverse is not true, so they are separate.

I don't want to make the call to just remove it because I still harbour hopes that OpenZFS could become a first-class citizen on Illumos someday. But hopefully this will at least make the reason it exists a bit clearer for people without long memories and/or an interest in history.

For those that are interested in the history, the original FreeBSD port of ZFS (before ZFS-on-Linux was adopted there) did have a spa_thread(), but not SDC. The last version of that before it was removed can be read here:

https://github.com/freebsd/freebsd-src/blob/22df1ffd812f0395cdb7c0b1edae1f67b991562a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c

Meanwhile, more information on the SDC scheduling class is here:

https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/disp/sysdc.c

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.

How Has This Been Tested?

This PR compile-checked only on Linux.

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:

@robn robn mentioned this pull request Oct 16, 2023
13 tasks
@amotin
Copy link
Member

amotin commented Oct 17, 2023

I haven't looked on it recently, though I was going to. But I am not sure that spa thread is related to the Illumos sysdc. Unless I am missing something, there was time when on FreeBSD there were per-pool processes that grouped all threads of the respective pool. It did not do anything about scheduling actually, but just grouped load in top process view instead just showing it as "kernel".

@robn
Copy link
Member Author

robn commented Oct 17, 2023

@amotin I think maybe we're both right!

It looks like the last FreeBSD spa.c before it was converted to OpenZFS did have a working spa_thread() gated behind SPA_PROCESS, and then also had a SYSDC define to compile out the SDC parts.

As I understand it (and I don't, really), on Illumos the SDC threads do have to be hung off a "real" thread, and the connection between the setup for taskq_create_sysdc() and spa->spa_proc made me think they were part of the same thing.

I will update this with what we've learned, and probably use the SYSDC define as well. Mostly what I wanted to do was mark the bits of spa_taskqs_init() that aren't currently relevant for OpenZFS to make it easier to read, so I get what I want that way too.

Thanks!

@robn
Copy link
Member Author

robn commented Oct 20, 2023

All updated.

@behlendorf behlendorf added the Status: Code Review Needed Ready for review and testing label Oct 20, 2023
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Oct 20, 2023
module/zfs/spa.c Outdated Show resolved Hide resolved
spa_thread() and the "System Duty Cycle" scheduling class are from
Illumos and have not yet been adapted to Linux or FreeBSD.

HAVE_SPA_THREAD has long been explicitly undefined and used to mark
spa_thread(), but there's some related taskq code that can never be
invoked without it, which makes some already-tricky code harder to read.

HAVE_SYSDC is introduced in this commit to mark the SDC parts. SDC
requires spa_thread(), but the inverse is not true, so they are
separate.

I don't want to make the call to just remove it because I still harbour
hopes that OpenZFS could become a first-class citizen on Illumos
someday. But hopefully this will at least make the reason it exists a
bit clearer for people without long memories and/or an interest in
history.

For those that are interested in the history, the original FreeBSD port
of ZFS (before ZFS-on-Linux was adopted there) did have a spa_thread(),
but not SDC. The last version of that before it was removed can be read
here:

  https://github.com/freebsd/freebsd-src/blob/22df1ffd812f0395cdb7c0b1edae1f67b991562a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c

Meanwhile, more information on the SDC scheduling class is here:

  https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/disp/sysdc.c

Signed-off-by: Rob Norris <[email protected]>
Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
@behlendorf behlendorf merged commit b5e6091 into openzfs:master Oct 23, 2023
19 checks passed
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
spa_thread() and the "System Duty Cycle" scheduling class are from
Illumos and have not yet been adapted to Linux or FreeBSD.

HAVE_SPA_THREAD has long been explicitly undefined and used to mark
spa_thread(), but there's some related taskq code that can never be
invoked without it, which makes some already-tricky code harder to read.

HAVE_SYSDC is introduced in this commit to mark the SDC parts. SDC
requires spa_thread(), but the inverse is not true, so they are
separate.

I don't want to make the call to just remove it because I still harbour
hopes that OpenZFS could become a first-class citizen on Illumos
someday. But hopefully this will at least make the reason it exists a
bit clearer for people without long memories and/or an interest in
history.

For those that are interested in the history, the original FreeBSD port
of ZFS (before ZFS-on-Linux was adopted there) did have a spa_thread(),
but not SDC. The last version of that before it was removed can be read
here:

  https://github.com/freebsd/freebsd-src/blob/22df1ffd812f0395cdb7c0b1edae1f67b991562a/sys/cddl/contrib/opensolaris/uts/common/fs/zfs/spa.c

Meanwhile, more information on the SDC scheduling class is here:

  https://github.com/illumos/illumos-gate/blob/master/usr/src/uts/common/disp/sysdc.c

Sponsored-by: Klara, Inc.
Sponsored-by: Wasabi Technology, Inc.
Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by:  Alexander Motin <[email protected]>
Signed-off-by: Rob Norris <[email protected]>
Closes openzfs#15406
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants