Remove zfs_vdev_elevator module option #9317
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
behlendorf
added
the
Status: Design Review Needed
Closed
This matches what i was investigating, unfortunately i had to leave debugging halfway to cover a shift. I was going to propose changing |
loli10K
approved these changes
Sep 12, 2019
Codecov Report
@@ Coverage Diff @@
## master #9317 +/- ##
==========================================
- Coverage 79.14% 79.13% -0.02%
==========================================
Files 401 401
Lines 122077 122042 -35
==========================================
- Hits 96617 96577 -40
- Misses 25460 25465 +5
Continue to review full report at Codecov.
|
@loli10K that's an interesting idea I hadn't really considered. While I do think we want to remove this in master, switching |
|
@behlendorf without any change to the 0.8 release branch i can deadlock my Debian builder, kernel 5.2.14, mirror root pool (with I believe this is the deadlock reported in the original issue, With the following patch applied the same system survives running diff --git a/module/zfs/vdev_disk.c b/module/zfs/vdev_disk.c
index 1419ae6ad..e38089c13 100644
--- a/module/zfs/vdev_disk.c
+++ b/module/zfs/vdev_disk.c
@@ -220,7 +220,7 @@ vdev_elevator_switch(vdev_t *v, char *elevator)
char *envp[] = { NULL };
argv[2] = kmem_asprintf(SET_SCHEDULER_CMD, device, elevator);
- error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_PROC);
+ error = call_usermodehelper(argv[0], argv, envp, UMH_WAIT_EXEC);
strfree(argv[2]);
#endif /* HAVE_ELEVATOR_CHANGE */
if (error) {
Technically we could probably even use |
|
@loli10K thanks for verifying the fix. Just to be on the safe side I'd suggest we use |
12 tasks
|
@behlendorf i shamefully stole part of your PR description and used it in #9321. |
behlendorf
force-pushed
the
remove-zfs_vdev_scheduler
branch
from
f9a619c to
e240739
Compare
behlendorf
added a commit
to behlendorf/zfs
that referenced
this pull request
Sep 16, 2019
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this can result in a system hang if the usermodehelper does not return. This is because it may be called with the spa config lock held as a writter. For example via the zfs_ioc_pool_scan() -> spa_scan() -> spa_scan() -> vdev_reopen() callpath. Additionally, there's a reasonable argument that setting the elevator is the resonsiblity of whatever software is being used to configure the system. Therefore, it's proposed that the zfs_vdev_elevator be removed. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#8664 Issue openzfs#9317
behlendorf
added
Status: Code Review Needed
tonyhutter
approved these changes
Sep 18, 2019
I don't use it personally, but I think it would be best to keep the option (doing nothing) for a while (e.g. all of 0.8.x), especially considering things like distro backports. |
rlaager
approved these changes
Sep 19, 2019
|
I agree with leaving it, possibly even in 0.9x but print warning to logs and possibly somewhere in userspace if the option is set, mainly because it can create an unbootable system once it's removed. People like me use backports so if someone updates using a newly back ported 0.9 and reboots with the option set, the system won't come back. I guess it really depends on how verbose the warnings are and when 0.9 drops. |
behlendorf
added a commit
to behlendorf/zfs
that referenced
this pull request
Sep 23, 2019
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this introduced a bug which could cause a system hang, that issue was subsequently fixed by commit 2a0d418. In order to avoid future bugs in this area, and to simplify the code, this functionality is being deprecated. A console warning has been added to notify any existing consumers and the documentation updated accordingly. This option will remain for the lifetime of the 0.8.x series for compatibility but if planned to be phased out of master. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#8664 Issue openzfs#9317
behlendorf
force-pushed
the
remove-zfs_vdev_scheduler
branch
from
e240739 to
c1cdb9e
Compare
|
Refreshed this PR. In the updated version nothing is removed, but a warning is printed when the module option is set and the documentation is updated accordingly so we can transition to removing this. |
rlaager
approved these changes
Sep 24, 2019
|
It's not critical. However, it would be nice if we could accelerate things a little bit and backport this warning to 0.8.x, then make it non-functional in the next major release, and remove it entirely in the one after. |
|
That seems like a perfectly reasonable timeframe. Absolute worst case, if things get bad for a distro backport, they could always patch it back in. |
behlendorf
added
Status: Accepted
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this introduced a bug which could cause a system hang, that issue was subsequently fixed by commit 2a0d418. In order to avoid future bugs in this area, and to simplify the code, this functionality is being deprecated. A console warning has been added to notify any existing consumers and the documentation updated accordingly. This option will remain for the lifetime of the 0.8.x series for compatibility but if planned to be phased out of master. Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#8664 Issue openzfs#9317
behlendorf
force-pushed
the
remove-zfs_vdev_scheduler
branch
from
c1cdb9e to
848c37d
Compare
12 tasks
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Dec 24, 2019
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this introduced a bug which could cause a system hang, that issue was subsequently fixed by commit 2a0d418. In order to avoid future bugs in this area, and to simplify the code, this functionality is being deprecated. A console warning has been added to notify any existing consumers and the documentation updated accordingly. This option will remain for the lifetime of the 0.8.x series for compatibility but if planned to be phased out of master. Reviewed-by: Richard Laager <[email protected]> Reviewed-by: loli10K <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#8664 Closes openzfs#9317
tonyhutter
pushed a commit
to tonyhutter/zfs
that referenced
this pull request
Dec 27, 2019
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this introduced a bug which could cause a system hang, that issue was subsequently fixed by commit 2a0d418. In order to avoid future bugs in this area, and to simplify the code, this functionality is being deprecated. A console warning has been added to notify any existing consumers and the documentation updated accordingly. This option will remain for the lifetime of the 0.8.x series for compatibility but if planned to be phased out of master. Reviewed-by: Richard Laager <[email protected]> Reviewed-by: loli10K <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue openzfs#8664 Closes openzfs#9317
tonyhutter
pushed a commit
that referenced
this pull request
Jan 23, 2020
Originally the zfs_vdev_elevator module option was added as a convenience so the requested elevator would be automatically set on the underlying block devices. At the time this was simple because the kernel provided an API function which did exactly this. This API was then removed in the Linux 4.12 kernel which prompted us to add compatibly code to set the elevator via a usermodehelper. While well intentioned this introduced a bug which could cause a system hang, that issue was subsequently fixed by commit 2a0d418. In order to avoid future bugs in this area, and to simplify the code, this functionality is being deprecated. A console warning has been added to notify any existing consumers and the documentation updated accordingly. This option will remain for the lifetime of the 0.8.x series for compatibility but if planned to be phased out of master. Reviewed-by: Richard Laager <[email protected]> Reviewed-by: loli10K <[email protected]> Reviewed-by: Tony Hutter <[email protected]> Signed-off-by: Brian Behlendorf <[email protected]> Issue #8664 Closes #9317
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Issue #8664. This PR proposes the removal of the
zfs_vdev_elevatormodule option for the following reasons:
implementation may lead to unexpected consequences.
available elevators can vary significantly; this makes it difficult
to manage from within the
zfs.kosince the available optionscannot be easily queried (or set).
flexible, the initramfs scripts already do this and other scripts
could be updated as appropriate.
Note: This PR entirely removes the module option. However, we
may instead want to leave it in place but disable it and update the
documentation accordingly. This would ensure that any systems
which are setting the module option are still able to load the kmod.
Description
Originally the
zfs_vdev_elevator moduleoption was added as aconvenience so the requested elevator would be automatically set
on the underlying block devices. At the time this was simple
because the kernel provided an API function which did exactly this.
This API was then removed in the Linux 4.12 kernel which prompted
us to add compatibly code to set the elevator via a usermodehelper.
While well intentioned this can result in a system hang if the
usermodehelper does not return. This is because it may be called
with the spa config lock held as a writter. For example via the
zfs_ioc_pool_scan() -> spa_scan() -> spa_scan() -> vdev_reopen()call path.
Additionally, there's a reasonable argument that setting the elevator
is the responsibility of whatever software is being used to configure
the system. Therefore, it's proposed that the zfs_vdev_elevator
be removed.
How Has This Been Tested?
Locally compiled, all referenced to the module option we're remove
the source and documentation.
Types of changes
Checklist:
Signed-off-by.