Set "none" scheduler if available (initramfs) #9042

colmbuckley · 2019-07-16T11:46:48Z

Existing zfs initramfs script logic will attempt to set the 'noop' scheduler if it's available on the vdev block devices. Newer kernels have the similar 'none' scheduler on multiqueue devices; this change alters the initramfs script logic to also attempt to set this scheduler.

Motivation and Context

Description

How Has This Been Tested?

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.

ghfields · 2019-07-16T16:58:24Z

I almost tackled this in #8004 but held back since there did not seem to be a consensus about the proper setting for mq devices.

Also check #8427 and its abandoned #8436 pull for Dracut.

I'm not exactly sure of what I am proposing, except to make sure all known efforts are aligned.

richardelling · 2019-07-16T17:54:20Z

I vote for removing scheduler changes from ZFS entirely. It is a problem better solved by OS installer.

colmbuckley · 2019-07-16T18:02:40Z

I vote for removing scheduler changes from ZFS entirely. It is a problem better solved by OS installer.

I fully agree. A note in the installation/system recommendations documentation would probably suffice. It's still not really clear to me why the simple schedulers are preferred; if the ZFS code also uses explicit synchronisation mechanisms.

codecov · 2019-07-16T21:02:20Z

Codecov Report

Merging #9042 into master will decrease coverage by 0.31%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #9042      +/-   ##
==========================================
- Coverage   79.31%   78.99%   -0.32%     
==========================================
  Files         400      400              
  Lines      121942   121653     -289     
==========================================
- Hits        96716    96098     -618     
- Misses      25226    25555     +329

Flag	Coverage Δ
#kernel	`79.65% <ø> (-0.2%)`	⬇️
#user	`66.59% <ø> (-0.5%)`	⬇️

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 f09fda5...320813b. Read the comment docs.

behlendorf · 2019-07-18T23:51:40Z

I vote for removing scheduler changes from ZFS entirely. It is a problem better solved by OS installer.

I'm inclined to agree, this behavior was originally added solely as convenience since at the time the kernel provided an easy mechanism to set this. That hasn't been the case since the 4.12 kernel.

contrib/initramfs/scripts/zfs.in

rlaager

The comment above the changed code says "elevator=noop". That should probably be adjusted in some way. For example, "Set the elevator to noop/none on the root pool's vdevs' disks. ZFS already". You'll probably need to re-wrap it as it's getting longer.
shellcheck is complaining about the while read i which it wants to be while read -r i. The latter seems to work. Maybe we should fix that here? I realize that's not new (and is my fault, as I think I wrote this originally).

colmbuckley · 2019-07-21T20:28:47Z

Before we get into the weeds of defensive shell programming; what are general thoughts about removing this stanza completely and not attempting to set the scheduler in initramfs? I'm more inclined to leave this up to individual systems administrators, or maybe print a warning here if the enclosing devices are using a scheduler we don't trust?

rlaager · 2019-07-21T20:36:06Z

If the proposal is for this to be entirely removed from ZFS (i.e. ZFS will not set none / noop on wholedisk vdevs), I'm on the fence. If the proposal is to remove it from only the initramfs, I am opposed, as that makes things inconsistent for root-on-ZFS installs.

colmbuckley · 2019-07-21T22:53:02Z

If the proposal is for this to be entirely removed from ZFS (i.e. ZFS will not set none / noop on wholedisk vdevs), I'm on the fence. If the proposal is to remove it from only the initramfs, I am opposed, as that makes things inconsistent for root-on-ZFS installs.

Fair enough; that deserves a more thorough design review. In the meantime, implemented the changes you suggest; PTAL.

rlaager

LGTM

behlendorf

Looks good. But would you mind rebasing this on the latest master and force updating the PR.

colmbuckley · 2019-07-31T14:28:33Z

Rebase pushed. I think the commit message from 42c24d9 is sufficient for the whole merge.

behlendorf · 2019-08-15T23:41:06Z

@colmbuckley it looks like this wasn't rebased. Would you mind doing it one more time and force updating the PR. Please also add your signed-off-by to the comment message (git commit --amend -s), and then I'll go ahead and get this merged.

Existing zfs initramfs script logic will attempt to set the 'noop' scheduler if it's available on the vdev block devices. Newer kernels have the similar 'none' scheduler on multiqueue devices; this change alters the initramfs script logic to also attempt to set this scheduler if it's available. Signed-off-by: Colm Buckley <[email protected]>

colmbuckley · 2019-08-18T09:50:32Z

@colmbuckley it looks like this wasn't rebased. Would you mind doing it one more time [...]

Sorry, my git-fu is weak. Done, I think, PTAL.

ghfields

Looks good. Thanks.

colmbuckley · 2019-09-27T01:44:15Z

Didn't make 0.8.2? Bah.

Existing zfs initramfs script logic will attempt to set the 'noop' scheduler if it's available on the vdev block devices. Newer kernels have the similar 'none' scheduler on multiqueue devices; this change alters the initramfs script logic to also attempt to set this scheduler if it's available. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Garrett Fields <[email protected]> Reviewed-by: Richard Laager <[email protected]> Signed-off-by: Colm Buckley <[email protected]> Closes openzfs#9042

Existing zfs initramfs script logic will attempt to set the 'noop' scheduler if it's available on the vdev block devices. Newer kernels have the similar 'none' scheduler on multiqueue devices; this change alters the initramfs script logic to also attempt to set this scheduler if it's available. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Garrett Fields <[email protected]> Reviewed-by: Richard Laager <[email protected]> Signed-off-by: Colm Buckley <[email protected]> Closes #9042

Existing zfs initramfs script logic will attempt to set the 'noop' scheduler if it's available on the vdev block devices. Newer kernels have the similar 'none' scheduler on multiqueue devices; this change alters the initramfs script logic to also attempt to set this scheduler if it's available. Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: Garrett Fields <[email protected]> Reviewed-by: Richard Laager <[email protected]> Signed-off-by: Colm Buckley <[email protected]> Closes openzfs#9042

This effectively reverts 4fc411f (part of openzfs#6807) and f6fbe25 (openzfs#9042) ‒ the code itself and latter PR cite symmetry with whole-disk-vdev behaviour (presumably because rootfs vdevs are rarely whole disks), but the code is broken for NVME devices (indeed, it'd strip the controller number instead of the (potential) partition number, turning "nvme0n1p1" into "nvmen1p1", which would then subsequently fail the sysfs existence check); it could be fixed to handle those (and any others) rather easily by dereferencing /sys/class/block/$devname, but this isn't the place for setting this ‒ as noted in the commit that removed setting the scheduler by default (9e17e6f) ‒ use an udev rule Signed-off-by: Ahelenia Ziemiańska <[email protected]>

This effectively reverts 4fc411f (part of #6807) and f6fbe25 (#9042) ‒ the code itself and latter PR cite symmetry with whole-disk-vdev behaviour (presumably because rootfs vdevs are rarely whole disks), but the code is broken for NVME devices (indeed, it'd strip the controller number instead of the (potential) partition number, turning "nvme0n1p1" into "nvmen1p1", which would then subsequently fail the sysfs existence check); it could be fixed to handle those (and any others) rather easily by dereferencing /sys/class/block/$devname, but this isn't the place for setting this ‒ as noted in the commit that removed setting the scheduler by default (9e17e6f) ‒ use an udev rule Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes #11838

This effectively reverts 4fc411f (part of openzfs#6807) and f6fbe25 (openzfs#9042) ‒ the code itself and latter PR cite symmetry with whole-disk-vdev behaviour (presumably because rootfs vdevs are rarely whole disks), but the code is broken for NVME devices (indeed, it'd strip the controller number instead of the (potential) partition number, turning "nvme0n1p1" into "nvmen1p1", which would then subsequently fail the sysfs existence check); it could be fixed to handle those (and any others) rather easily by dereferencing /sys/class/block/$devname, but this isn't the place for setting this ‒ as noted in the commit that removed setting the scheduler by default (9e17e6f) ‒ use an udev rule Reviewed-by: Brian Behlendorf <[email protected]> Signed-off-by: Ahelenia Ziemiańska <[email protected]> Closes openzfs#11838

colmbuckley mentioned this pull request Jul 16, 2019

ZFS: Unable to set "noop" scheduler #6513

Closed

behlendorf added the Status: Design Review Needed Architecture or design is under discussion label Jul 16, 2019

behlendorf requested a review from rlaager July 18, 2019 23:51

rlaager reviewed Jul 20, 2019

View reviewed changes

contrib/initramfs/scripts/zfs.in Outdated Show resolved Hide resolved

rlaager suggested changes Jul 20, 2019

View reviewed changes

rlaager approved these changes Jul 22, 2019

View reviewed changes

behlendorf approved these changes Jul 30, 2019

View reviewed changes

behlendorf added Status: Code Review Needed Ready for review and testing and removed Status: Design Review Needed Architecture or design is under discussion labels Jul 30, 2019

behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Aug 15, 2019

ghfields approved these changes Aug 18, 2019

View reviewed changes

rlaager approved these changes Aug 18, 2019

View reviewed changes

behlendorf merged commit f6fbe25 into openzfs:master Aug 19, 2019

colmbuckley deleted the patch-1 branch August 22, 2019 04:02

ghfields mentioned this pull request Sep 5, 2019

Wiki: Encrypted ZFS Root on Debian Buster - Plymouth breaks grub #9283

Closed

behlendorf mentioned this pull request Sep 5, 2019

zfs-0.8.2 patchset #9161

Closed

12 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set "none" scheduler if available (initramfs) #9042

Set "none" scheduler if available (initramfs) #9042

colmbuckley commented Jul 16, 2019 •

edited

Loading

ghfields commented Jul 16, 2019

richardelling commented Jul 16, 2019

colmbuckley commented Jul 16, 2019

codecov bot commented Jul 16, 2019 •

edited

Loading

behlendorf commented Jul 18, 2019

rlaager left a comment

colmbuckley commented Jul 21, 2019

rlaager commented Jul 21, 2019

colmbuckley commented Jul 21, 2019

rlaager left a comment

behlendorf left a comment

colmbuckley commented Jul 31, 2019

behlendorf commented Aug 15, 2019

colmbuckley commented Aug 18, 2019

ghfields left a comment

colmbuckley commented Sep 27, 2019

Set "none" scheduler if available (initramfs) #9042

Set "none" scheduler if available (initramfs) #9042

Conversation

colmbuckley commented Jul 16, 2019 • edited Loading

Motivation and Context

Description

How Has This Been Tested?

Types of changes

Checklist:

ghfields commented Jul 16, 2019

richardelling commented Jul 16, 2019

colmbuckley commented Jul 16, 2019

codecov bot commented Jul 16, 2019 • edited Loading

Codecov Report

behlendorf commented Jul 18, 2019

rlaager left a comment

Choose a reason for hiding this comment

colmbuckley commented Jul 21, 2019

rlaager commented Jul 21, 2019

colmbuckley commented Jul 21, 2019

rlaager left a comment

Choose a reason for hiding this comment

behlendorf left a comment

Choose a reason for hiding this comment

colmbuckley commented Jul 31, 2019

behlendorf commented Aug 15, 2019

colmbuckley commented Aug 18, 2019

ghfields left a comment

Choose a reason for hiding this comment

colmbuckley commented Sep 27, 2019

colmbuckley commented Jul 16, 2019 •

edited

Loading

codecov bot commented Jul 16, 2019 •

edited

Loading