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

Initramfs fixes #6807

Merged
merged 3 commits into from
Nov 3, 2017
Merged

Initramfs fixes #6807

merged 3 commits into from
Nov 3, 2017

Conversation

rlaager
Copy link
Member

@rlaager rlaager commented Nov 2, 2017

Description

Besides the two minor cleanups (one whitespace and one typo), the point of this pull request is to make the initramfs script set elevator=noop on the root pool's disks.

ZFS already sets elevator=noop for wholedisk vdevs (for all pools), but typical root-on-ZFS installations use partitions. This sets elevator=noop on the disks in the root pool.

Ubuntu 16.04 and 16.10 had this. It was lost in 17.04 due to Debian switching to the upstream initramfs script.

How Has This Been Tested?

I originally tested the main code for this feature for Ubuntu 16.04, and it was integrated and has seen real-world use.

I tested this particular integration on an Ubuntu 17.10 system.

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.

@rlaager
initramfs: Fix inconsistent whitespace
4767c7a 
This fixes one instance of inconsistent whitespace.

Signed-off-by: Richard Laager <[email protected]>
@rlaager
initramfs: Fix a spelling error
11b9dcf 
This fixes a typo in a comment.

Signed-off-by: Richard Laager <[email protected]>
@rlaager
initramfs: Set elevator=noop on the rpool's disks
4fc411f 
ZFS already sets elevator=noop for wholedisk vdevs (for all pools), but
typical root-on-ZFS installations use partitions.  This sets
elevator=noop on the disks in the root pool.

Ubuntu 16.04 and 16.10 had this.  It was lost in 17.04 due to Debian
switching to this upstream initramfs script.

Signed-off-by: Richard Laager <[email protected]>
gmelikov
gmelikov approved these changes Nov 2, 2017
View reviewed changes
Copy link
Member

@gmelikov gmelikov left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@behlendorf behlendorf merged commit 34c2b36 into openzfs:master Nov 3, 2017
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Nov 21, 2017
@behlendorf @tonyhutter
Initramfs fixes
414f4a9 
* initramfs: Fix inconsistent whitespace
* initramfs: Fix a spelling error
* initramfs: Set elevator=noop on the rpool's disks

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: George Melikov <[email protected]>
Signed-off-by: Richard Laager <[email protected]>
Closes openzfs#6807
@colmbuckley
Copy link
Contributor

This now fails with an error on Debian Buster with 4.19.37 kernel (as distributed); the "noop" scheduler does not seem to be included in this kernel:

lugh# cat /sys/block/sda/queue/scheduler
[mq-deadline] none
lugh# echo noop > /sys/block/sda/queue/scheduler
echo: write error: invalid argument

Should this now be using "none" with suitable tests? The code change is fairly trivial, but I don't understand enough about why the original change was made.

@colmbuckley
Copy link
Contributor

colmbuckley commented Jul 16, 2019 •
edited
Loading

Oh, seems that this is the distinction between multiqueue and non-multiqueue schedulers. What scheduler should be chosen in a multiqueue environment?

Assuming "none", something like:

                SCHEDULER="/sys/block/$i/queue/scheduler"
                if [ -e "$SCHEDULER" ]
                then
                        case $(cat "$SCHEDULER") in
                                *noop*) echo noop > "$SCHEDULER" ;;
                                *none*) echo none > "$SCHEDULER" ;;
                        esac
                fi

is probably the easiest fix.

@colmbuckley
Copy link
Contributor

This is addressed #8004 - which I guess did not make it into 0.7.12. 0.7.13 should be hitting Debian in the next few weeks, so I guess it'll be fixed then.

@colmbuckley colmbuckley mentioned this pull request Jul 16, 2019
Closed
nabijaczleweli added a commit to nabijaczleweli/zfs that referenced this pull request Apr 3, 2021
@nabijaczleweli
i-t: don't brokenly set the scheduler for root pool vdev's disks
99f1849 
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]>
behlendorf pushed a commit that referenced this pull request Apr 7, 2021
@nabijaczleweli @behlendorf
i-t: don't brokenly set the scheduler for root pool vdev's disks
aa5a4eb 
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
behlendorf pushed a commit that referenced this pull request Apr 7, 2021
@nabijaczleweli @behlendorf
i-t: don't brokenly set the scheduler for root pool vdev's disks
04344d5 
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
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Apr 9, 2021
@nabijaczleweli @mcmilk
i-t: don't brokenly set the scheduler for root pool vdev's disks
603eee7 
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
mcmilk pushed a commit to mcmilk/zfs that referenced this pull request Apr 9, 2021
@nabijaczleweli @mcmilk
i-t: don't brokenly set the scheduler for root pool vdev's disks
0c11432 
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
adamdmoss pushed a commit to adamdmoss/zfs that referenced this pull request Apr 10, 2021
@nabijaczleweli @adamdmoss
i-t: don't brokenly set the scheduler for root pool vdev's disks
fb90c43 
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
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
@nabijaczleweli
i-t: don't brokenly set the scheduler for root pool vdev's disks
61348c0 
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmelikov gmelikov gmelikov approved these changes

@behlendorf behlendorf behlendorf approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@rlaager @colmbuckley @behlendorf @gmelikov