CP-53446: Split plug and unplug atomics to enable live migration downtime reduction later #6362
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
psafont
changed the title
|
It helps when reviewing to add the reason for the change into the PR:
|
psafont
changed the title
lindig
reviewed
Mar 17, 2025
snwoods
changed the title
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
2 times, most recently
from
65ec25e to
0926423
Compare
|
I've added comments to make things clearer and cleaned some things up. I've also consolidated the two flags into one because I can't see a situation where we would need to split one and not the other. |
psafont
reviewed
Mar 18, 2025
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
5 times, most recently
from
3a6d281 to
17bca64
Compare
robhoes
reviewed
Mar 20, 2025
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
from
17bca64 to
faea8ea
Compare
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
from
faea8ea to
8906fbb
Compare
|
The changes seem good, but the commit messages are a bit too bare, could you add more context to them? |
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
from
8906fbb to
71f220e
Compare
edwintorok
reviewed
Apr 2, 2025
edwintorok
reviewed
Apr 2, 2025
Signed-off-by: Steven Woods <[email protected]>
This consists of two parts, splitting the attach_and_activate into functionally equivalent attach and activate functions, and splitting the VBD_plug atomic itself's behaviour into two new atomics, VBD_attach and VBD_activate. If the new xenopsd_vbd_plug_unplug_legacy flag is true, the only difference will be that VBD_plug calls attach and activate sequentially instead of attach_and_activate. If xenopsd_vbd_plug_unplug_legacy is false, the VBD_attach and VBD_activate atomics will be used in place of VBD_plug in all places that it is used. This should still be functionally equivalent. The purpose of this change is to allow optimisations in this area as there are some situations where they do not need to be called at the same time. For example VBD_attach could be called outside of VM migrate downtime to reduce the overall downtime. Signed-off-by: Steven Woods <[email protected]>
This consists of two parts, splitting the unplug function into functionally equivalent deactivate and detach functions, and splitting the VBD_unplug atomic itself's behaviour into two new atomics: VBD_deactivate and VBD_detach If the xenopsd_vbd_plug_unplug_legacy flag is true, the only difference will be that VBD_unplug calls deactivate and detach sequentially instead of unplug If xenopsd_vbd_plug_unplug_legacy is false, the VBD_deactivate and VBD_detach atomics will be used in place of VBD_unplug in all places that it is used. This should still be functionally equivalent. The purpose of this change is to allow optimisations in this area as there are some situations where they do not need to be called at the same time. For example we could skip detaching on reboot and only deactivate and activate again reducing reboot time. Signed-off-by: Steven Woods <[email protected]>
snwoods
force-pushed
the
private/stevenwo/CP-53446
branch
from
71f220e to
1a46f33
Compare
snwoods
commented
Apr 4, 2025
edwintorok
reviewed
Apr 17, 2025
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.
Split the plug and unplug atomics into separate attach/activate and deactivate/detach atomics. This is gated by two separate flags and both are switched off by default. When switched on, this should still be a no-op by calling them in serial where plug/unplug are currently used, but will allow us to later use these atomics separately elsewhere for optimisation.
Opening this as a draft PR as plug/unplug is fundamental so the logic needs to be watertight before we consider merging.