Skip to content

i-t: fix root=zfs:AUTO, don't attempt to badly set scheduler #11838

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
Closed
wants to merge 2 commits into from
Closed

i-t: fix root=zfs:AUTO, don't attempt to badly set scheduler #11838

wants to merge 2 commits into from

Conversation

nabijaczleweli
Copy link
Contributor

@nabijaczleweli nabijaczleweli commented Apr 3, 2021 •
edited
Loading

Motivation and Context

root=zfs:AUTO was broken on i-t, and the scheduler setting could only be described as "weird" and "broken" and "nothing else did it"

Description

See commit messages

How Has This Been Tested?

Boots with root=zfs:tzpfmest-zoot/root, root=zfs:AUTO, boot=zfs.
Also, #11278 (comment)

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:

@nabijaczleweli
i-t: fix root=zfs:AUTO
a3ea28d 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11278
@nabijaczleweli nabijaczleweli mentioned this pull request Apr 3, 2021
Closed
@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]>
@nabijaczleweli nabijaczleweli changed the title i-t: fix root=zfs:AUTO i-t: fix root=zfs:AUTO, don't attempt to badly set scheduler Apr 3, 2021
behlendorf
behlendorf approved these changes Apr 6, 2021
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Thanks. The scheduler bit here was for some legacy functionality which was used to largely disable the IO scheduler since ZFS has it's own (only allow front and back merges). However, this functionality was removed a while ago since it was hard to support and users were encourages just to write a udev rule is they wanted to change the default scheduler.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Apr 6, 2021
@behlendorf behlendorf closed this in 55419e0 Apr 7, 2021
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: fix root=zfs:AUTO
46aec6d 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes #11278
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: fix root=zfs:AUTO
7cb8aab 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11278
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
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: fix root=zfs:AUTO
f8aa53d 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11278
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: fix root=zfs:AUTO
f1e63d9 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11278
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: fix root=zfs:AUTO
5e0935c 
IFS= would break loops in import_pool(), which would fault
any automatic import

Additionally $ZFS_BOOTFS from cmdline would interfere with find_rootfs()

If many pools were present, same thing could happen across multiple
find_rootfs() runs, so bail out early and clean up in error path

Suggested-by: @nachtgeist
Reviewed-by: Brian Behlendorf <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Closes openzfs#11278
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
@Artturin Artturin mentioned this pull request Aug 20, 2023
Merged
12 tasks
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

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

Successfully merging this pull request may close these issues.
2 participants
@nabijaczleweli @behlendorf