virtio-pmem: fixes and improvements - #5789
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5789 +/- ##
==========================================
- Coverage 83.06% 82.88% -0.19%
==========================================
Files 276 276
Lines 29583 29708 +125
==========================================
+ Hits 24573 24623 +50
- Misses 5010 5085 +75
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
46cccd5 to
813efa7
Compare
d140771 to
e48ab2a
Compare
e4ccee4 to
24441ed
Compare
a0aae35 to
a9a3d01
Compare
1033b96 to
777ceb0
Compare
b9588ce to
ba905dd
Compare
JamesC1305
left a comment
There was a problem hiding this comment.
Overall LGTM. The only thing I wonder is if it could do with integration tests for the rate limiting functionality, similar to how the block device has test_patch_drive_limiter.
1698fab to
28b5de8
Compare
|
Added rate-limiter integration tests as well |
Manciukic
left a comment
There was a problem hiding this comment.
a few more nits for inconsistencies in unit test, error messages, and docs
4419042 to
e525cb5
Compare
Head and status descriptors must be 4 bytes long by the spec. Add validation for this. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
There is only one type of request virto-pmem accepts, but guest still can issue many of them and Firecracker will need to process it all. Instead of doing one `msync` per request, it is less resource intensive to do it once on the first valid descriptor and then duplicate the result to other descriptors. This is safe since the guest will only know the result of the execution after Firecracker will signal it, which will only happen after all descriptors are processed. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Add rate-limiter support to the virtio-pmem device to allow users to configure limits of the I/O bandwidth generated by the `msync` call in the device which could be triggered by the guest FLUSH requests. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Add a section in the pmem.md file describing a way of limiting I/O usage of `msync` calls from a virtio-pmem device. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Mention new rate-limiter API for the virtio-pmem device in the CHANGELOG.md Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Fix incorrect NOTE section formatting in memory usage section Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Update device-api.md with rate-limiter field and new patch request available for virtio-pmem. Signed-off-by: Egor Lazarchuk <yegorlz@amazon.co.uk>
Changes
lenfield to be 4 since the code expects thismsyncresult for better efficiency in case multiple flush requests are presented at oncerate-limitersupportReason
Edge case handling and addition of missing features
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
tools/devtool checkbuild --allto verify that the PR passesbuild checks on all supported architectures.
tools/devtool checkstyleto verify that the PR passes theautomated style checks.
how they are solving the problem in a clear and encompassing way.
in the PR.
CHANGELOG.md.Runbook for Firecracker API changes.
integration tests.
TODO.rust-vmm.