Fix hangs with parallel pool import #16178
Closed
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
Sponsored by: Axcient Signed-off-by: Alan Somers <[email protected]>
During parallel zpool import, /sbin/zpool will create a separate thread pool for each pool, used to mount that pool's datasets. If the total thread count exceed's the system's limit on threads per process, then tpool_dispatch may fail. If it does, directly execute the mount operation instead. Sponsored by: Axcient Signed-off-by: Alan Somers <[email protected]> Fixes openzfs#16172
behlendorf
approved these changes
May 9, 2024
There was a problem hiding this comment.
However, it changes the libzfs ABI.
Thanks for calling this out. The original parallel import code has ended up in tagged release yet so this isn't a major problem.
Add an ATF test for tpool_dispatch to ensure correct behavior if it cannot start even one worker. Due to lack of test infrastructure within the OpenZFS repository, this commit is not actually included in this PR. Instead, I plan to commit it directly to FreeBSD.
Good.
@grwilson can you review this.
|
@behlendorf if it's ok to change the libzfs abi, could you please tell me how to update the libzfs.abi file? It looks like it's automatically generated somehow. |
|
@asomers you can download an updated abi file from the artifacts for this PR (at the bottom). You'll just need to replace the current abi file in this PR with the new once after inspecting the differences, then force update the PR. https://github.com/openzfs/zfs/actions/runs/9007873783?pr=16178 |
Ever since a10d50f, ZFS has mounted file systems in parallel when importing a pool. It uses a fixed size of 512 for the thread pool. But since c183d16, it has also imported pools in parallel. So the total number of threads at one time is 513 * npools + 1. That can easily exceed the system's limit on the number of threads per process, which will cause one or more pools to be unable to allocate any worker threads, forcing them to fallback to slow serial mounting . To forestall that, manage the threadpool size in /sbin/zpool, not libzfs. Use the same size (512), but divided by the number of pools. This is a backwards-incompatible change to the libzfs abi. Sponsored by: Axcient Signed-off-by: Alan Somers <[email protected]>
Ahh, that's easy. Done! |
behlendorf
added
Status: Accepted
grwilson
approved these changes
May 14, 2024
behlendorf
pushed a commit
that referenced
this pull request
May 14, 2024
During parallel zpool import, /sbin/zpool will create a separate thread pool for each pool, used to mount that pool's datasets. If the total thread count exceed's the system's limit on threads per process, then tpool_dispatch may fail. If it does, directly execute the mount operation instead. Sponsored by: Axcient Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alan Somers <[email protected]> Closes #16178 Fixes #16172
behlendorf
pushed a commit
that referenced
this pull request
May 14, 2024
Ever since a10d50f, ZFS has mounted file systems in parallel when importing a pool. It uses a fixed size of 512 for the thread pool. But since c183d16, it has also imported pools in parallel. So the total number of threads at one time is 513 * npools + 1. That can easily exceed the system's limit on the number of threads per process, which will cause one or more pools to be unable to allocate any worker threads, forcing them to fallback to slow serial mounting . To forestall that, manage the threadpool size in /sbin/zpool, not libzfs. Use the same size (512), but divided by the number of pools. This is a backwards-incompatible change to the libzfs abi. Sponsored by: Axcient Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alan Somers <[email protected]> Closes #16178
|
Now that this PR has been merged downstream into FreeBSD, here is a review for the libtpool ATF test. https://reviews.freebsd.org/D45587 |
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this pull request
Sep 4, 2024
Sponsored by: Axcient Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alan Somers <[email protected]> Closes openzfs#16178
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this pull request
Sep 4, 2024
During parallel zpool import, /sbin/zpool will create a separate thread pool for each pool, used to mount that pool's datasets. If the total thread count exceed's the system's limit on threads per process, then tpool_dispatch may fail. If it does, directly execute the mount operation instead. Sponsored by: Axcient Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alan Somers <[email protected]> Closes openzfs#16178 Fixes openzfs#16172
lundman
pushed a commit
to openzfsonwindows/openzfs
that referenced
this pull request
Sep 4, 2024
Ever since a10d50f, ZFS has mounted file systems in parallel when importing a pool. It uses a fixed size of 512 for the thread pool. But since c183d16, it has also imported pools in parallel. So the total number of threads at one time is 513 * npools + 1. That can easily exceed the system's limit on the number of threads per process, which will cause one or more pools to be unable to allocate any worker threads, forcing them to fallback to slow serial mounting . To forestall that, manage the threadpool size in /sbin/zpool, not libzfs. Use the same size (512), but divided by the number of pools. This is a backwards-incompatible change to the libzfs abi. Sponsored by: Axcient Reviewed-by: Brian Behlendorf <[email protected]> Reviewed-by: George Wilson <[email protected]> Signed-off-by: Alan Somers <[email protected]> Closes openzfs#16178
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
The parallel pool import feature (#16093) can greatly increase the number of threads used by /sbin/zpool . If it reaches the OS-imposed per-process limit and then tries to import an additional pool, the command will hang. This PR fixes such hangs.
Description
The PR consists of four distinct commits:
tpool_dispatchreturns an error if it cannot start at least 1 worker. Previously it would silently fail.tpool_dispatchfails while mounting file systems, make libzfs fall back to serial execution. This fixes the bug, though with inconsistent performance.tpool_dispatchto ensure correct behavior if it cannot start even one worker. Due to lack of test infrastructure within the OpenZFS repository, this commit is not actually included in this PR. Instead, I plan to commit it directly to FreeBSD.How Has This Been Tested?
In addition to the aforementioned libtpool test, I have tested this change by importing 6 pools in parallel, each with 1501 file systems. Before my changes, that would fail on 7 out of 8 attempts. Now it always succeeds, with consistent performance.
Types of changes
Checklist:
Signed-off-by.Fixes #16172