Reset dirty log on full snapshot path #4385
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
Grandmother
force-pushed
the
hackathon
branch
4 times, most recently
from
8498cbc
to
f88092d
Compare
9 tasks
Grandmother
force-pushed
the
hackathon
branch
4 times, most recently
from
a70c889
to
4f8a7e9
Compare
When a diff snapshot it taken, it always contains all pages that were dirtied since either instance start, or since the last diff snapshot was taken, whichever is more recent. This is not what we document it to do: > the diff consists of the memory pages which have been dirtied since > the last snapshot creation or since the creation of the microVM, > whichever of these events was the most recent. Here "last snapshot creation" includes full snapshots, not just diff snapshots (and that makes sense, if I take a diff snapshot after a full snapshot, I expect the diff snapshot to be a diff compared to the full snapshot). Signed-off-by: Roman Kovtyukh <[email protected]> Co-authored-by: Muskaan Singla <[email protected]>
Test that taking full snapshot is as described in documentation: > the diff consists of the memory pages which have been dirtied since > the last snapshot creation or since the creation of the microVM, > whichever of these events was the most recent. Signed-off-by: Roman Kovtyukh <[email protected]> Co-authored-by: Pablo Barbáchano <[email protected]>
Grandmother
force-pushed
the
hackathon
branch
from
4f8a7e9
to
f996dc6
Compare
pb8o
reviewed
Jan 22, 2024
| @@ -249,7 +249,10 @@ fn snapshot_memory_to_file( | |||
| .dump_dirty(&mut file, &dirty_bitmap) | |||
| .map_err(Memory) | |||
| } | |||
| SnapshotType::Full => vmm.guest_memory().dump(&mut file).map_err(Memory), | |||
| SnapshotType::Full => { | |||
| let _ = vmm.get_dirty_bitmap().map_err(DirtyBitmap)?; | |||
There was a problem hiding this comment.
I think we should only get the bitmap if it exists (see CI errors). Probably by handling the error here.
9 tasks
|
Closing this, as the issue was addressed by #4563 (using as well commits from here) |
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.
Changes
get_dirty_bitmapon the full snapshot pathReason
Vmm:get_dirty_bitmapthus doesn’t callKVM_GET_DIRTY_LOGwhich doesn’t reset dirty pages.Closes #4543.
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the
Apache 2.0 license. For more information on following Developer Certificate of Origin and signing
off your commits, please check
CONTRIBUTING.md.PR Checklist
If a specific issue led to this PR, this PR closes the issue.
The description of changes is clear and encompassing.
Any required documentation changes (code and docs) are included in this PR.
API changes follow the Runbook for Firecracker API
changes.
User-facing changes are mentioned in
CHANGELOG.md.All added/changed functionality is tested.
New
TODOs link to an issue.Commits meet contribution quality
standards.
This functionality cannot be added in
rust-vmm.