Add zfs_vdev_parallel_open_enabled module parameter #5286
Conversation
|
@ryao, thanks for your PR! By analyzing the history of the files in this pull request, we identified @lundman, @behlendorf and @FransUrbo to be potential reviewers. |
|
@behlendorf Nice bot. :) |
ryao
force-pushed
the
parallel_vdev
branch
3 times, most recently
from
1f0c412 to
711fdc4
Compare
There was a problem hiding this comment.
Thank's @ryao. We're trying out the mention bot, so far it's working pretty well. As for this patch just two changes requested.
| @@ -250,6 +250,25 @@ Use \fB1\fR for yes (default) and \fB0\fR for no. | |||
| .sp | |||
| .ne 2 | |||
| .na | |||
| \fBparallel_vdev_open\fR (bool) | |||
There was a problem hiding this comment.
For consistency with the other options how about we call this zfs_vdev_parallel_open_enabled.
There was a problem hiding this comment.
Thanks for updating the name, but it looks like you overlooked updating the man page.
| @@ -3642,6 +3647,10 @@ EXPORT_SYMBOL(vdev_online); | |||
| EXPORT_SYMBOL(vdev_offline); | |||
| EXPORT_SYMBOL(vdev_clear); | |||
|
|
|||
| module_param(parallel_vdev_open, bool, 0644); | |||
There was a problem hiding this comment.
Should be an int not a bool to resolve the build failure.
| Accelerates vdev open from pool import or reopen from reload from cache by | ||
| allow child vdevs to be opened in parallel. | ||
| .sp | ||
| This almost always deadlocks in unsupported ZFS sandwich configurations where a |
There was a problem hiding this comment.
I think the term "stacked" is more appropriate than "sandwich" here.
ryao
force-pushed
the
parallel_vdev
branch
5 times, most recently
from
7bfb700 to
de61921
Compare
|
@behlendorf I think I got all of the typos out of the man page this time. :) |
ryao
changed the title
|
It occurs to me that in theory we could use this option to enable all the nested zpool tests in the test suite. |
ryao
force-pushed
the
parallel_vdev
branch
2 times, most recently
from
172ff9a to
d4b202e
Compare
|
@behlendorf I reactivated the nested pool tests and modified them to turn parallel import off just for Linux. |
Parallel vdev open causes the driver to deadlock when running ZFS on top of a zvol with something in between, including, but not limited to the loop device. Such configurations are historically unsupported. However, enough people have requested the capability over the years that we ought to give them a knob to make the configuration work in most instances. We have tests for the direct case, but the indirect case continues to be unsupported. Signed-off-by: Richard Yao <ryao@gentoo.org>
|
@ryao thanks, although we probably want to enable the tests in a separate PR. That way we won't need to work through fixing every test failure we see before merging this. It will be nice to get a test run though to see how many of theses tests pass. |
|
|
||
| if is_linux; then | ||
| echo 1 > /sys/module/zfs/parameters/zfs_vdev_parallel_open_enabled | ||
| fi |
There was a problem hiding this comment.
Adding a helper function for this in tests/zfs-tests/include/libtest.shlib would be in order. That way we can keep this is_linux code out of the test cases.
There was a problem hiding this comment.
Agreed, although I'll wait for the buildbot to finish before changing this.
There was a problem hiding this comment.
Best to do it in a new PR too and remove those changes from this PR. Then I'll be able to merge this.
There was a problem hiding this comment.
What if the zfs test suite passes? I have my fingers crossed. :)
|
@ryao the original fix here is good, but it can't be merged while it contains all the modification to the test suite which cause test failures. Can you drop the test suite modifications from this patch. |
|
@ryao I'm closing this for now. When you get a chance to work on this again go ahead and open a new PR which addresses the comments above. |
Parallel vdev open causes the driver to deadlock when running ZFS on top
of a zvol with something in between, including, but not limited to the
loop device. Such configurations are unsupported because we do not have
regression test coverage for them, but enough people have requested the
capability over the years that we ought to give them a knob to make the
configuration work in most instances.
Signed-off-by: Richard Yao ryao@gentoo.org