[ BUGFIX ] Fabrics connect timeouts #1711
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
tiagolobocastro
requested review from
dsavitskiy,
abhilashshetty04 and
dsharma-dc
tiagolobocastro
commented
Aug 6, 2024
|
bors try |
tryBuild failed: |
tiagolobocastro
force-pushed
the
fabrics-connect
branch
from
182d2f7 to
e0f2b9a
Compare
Allows passing this via env NVMF_FABRICS_CONNECT_TIMEOUT. Also defaults it to 1s for now, rather than 500ms. Signed-off-by: Tiago Castro <[email protected]>
Otherwise a returned negative value translates into an unknown Errno. Signed-off-by: Tiago Castro <[email protected]>
Initially this block_on was added because the remove callback was running in blocking fashion, but this has since changed and unplug is actually called from async context. As such, we don't need the block_on and simply call the async code directly. Also, simplify complete notification, as we can simply close the sender. Signed-off-by: Tiago Castro <[email protected]>
tiagolobocastro
force-pushed
the
fabrics-connect
branch
from
e0f2b9a to
8893313
Compare
|
bors try |
tryBuild succeeded: |
dsharma-dc
approved these changes
Aug 9, 2024
|
Do we know the reason why historically these paths were using blocking on futures to complete? |
dsavitskiy
approved these changes
Aug 9, 2024
Split creating from starting the subsystem. This way we can start the subsystem in master reactor, and then move to the next spdk subsystem. Signed-off-by: Tiago Castro <[email protected]>
the unplug code was originally not async, and suspect that's why block_on was used. The others I don't know, perhaps was just used as a "shotcut"... |
tiagolobocastro
force-pushed
the
fabrics-connect
branch
from
8893313 to
d5b5d44
Compare
|
Resolves #1710 |
bors-openebs-mayastor bot
pushed a commit
that referenced
this pull request
Aug 9, 2024
1711: [ BUGFIX ] Fabrics connect timeouts r=tiagolobocastro a=tiagolobocastro
Reactor block_on may prevent spdk thread messages from running and therefore this can lead to starvation of messages pulled from the thread ring, which are not polled during the block_on.
There are still a few uses remaining, most during init setup, so mostly harmless, though the Nexus Bdev destruction which runs on blocking code, does still contain a block_on.
---
fix(nvmf/target): remove usage of block_on
Split creating from starting the subsystem.
This way we can start the subsystem in master reactor, and then move
to the next spdk subsystem.
Signed-off-by: Tiago Castro <[email protected]>
---
fix(nexus-child/unplug): remove usage of block_on
Initially this block_on was added because the remove callback was running in blocking
fashion, but this has since changed and unplug is actually called from async context.
As such, we don't need the block_on and simply call the async code directly.
Also, simplify complete notification, as we can simply close the sender.
Signed-off-by: Tiago Castro <[email protected]>
---
fix(nvmx/qpair): return errno with absolute value
Otherwise a returned negative value translates into an unknown Errno.
Signed-off-by: Tiago Castro <[email protected]>
---
feat: allow custom fabrics connect timeout
Allows passing this via env NVMF_FABRICS_CONNECT_TIMEOUT.
Also defaults it to 3s for now, rather than 500ms.
Signed-off-by: Tiago Castro <[email protected]>
Co-authored-by: Tiago Castro <[email protected]>
|
oops typo |
|
Canceled. |
Should this become an unsafe function? Signed-off-by: Tiago Castro <[email protected]>
tiagolobocastro
force-pushed
the
fabrics-connect
branch
from
5c36253 to
d9e19b3
Compare
|
bors merge |
|
Build succeeded: |
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.
Reactor block_on may prevent spdk thread messages from running and therefore this can lead to starvation of messages pulled from the thread ring, which are not polled during the block_on.
There are still a few uses remaining, most during init setup, so mostly harmless, though the Nexus Bdev destruction which runs on blocking code, does still contain a block_on.