-
Notifications
You must be signed in to change notification settings - Fork 565
qcow2 compression (zlib, zstd) #7462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
qcow2 compression (zlib, zstd) #7462
Conversation
rbradford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR - really nice structure.
b2f39d7 to
a151f8a
Compare
|
Had to squash all commits together due to impossibility to keep Cargo.lock, block/Cargo.toml, block/src/qcow/decoder.rs in sync if they were modified in different commits. The PR is not big, so I think this is not an issue. |
rbradford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we could unit test with a real compressed data - or alternatively do a boot test in the integration test suite with a compressed qcow2 file (we could just compress one of the existing files.)
a151f8a to
37e2688
Compare
|
I succeeded to boot qcow2-compresed Ubuntu image. qcow2-backing also works well. Hope tests will work well when test qcow2 images are compressed. Will try a bit later... |
bac1c85 to
d4a91a2
Compare
phip1611
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally LGTM, thanks for working on this! I left a few remarks
d4a91a2 to
5c856b8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't use anyhow here - we want to get rid of it in most places in Cloud Hypervisor, not strengthen the dependency to it :) Apart from that, changes are looking good.
5c856b8 to
2ded770
Compare
phip1611
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing my comments. LGTM now! Please fix CI
8e98d97 to
bf3a856
Compare
|
getting a weird error at CI |
Looks like the recurring GitHub network flake we've seen today. I just restarted the worker. |
rbradford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just those two changes and then we can 🚀 this.
bf3a856 to
39c97ad
Compare
|
Great ! lgtm- can you rebase this to drop the merge commit and then we can land it. |
1601bb0 to
4787c0f
Compare
Add support of reading and writing compressed clusters. Support zlib and zstd compressions. L2 cache: store entire L2 entries, not only standard cluster addresses. Read path. Offsets of compressed clusters cannot be determined, therefore replace QcowFile.file_offset_read() with QcowFile.file_read(). This method reads the cluster, decompresses it if necessary and returns the data to the caller. Write path. QcowFile.file_offset_write(): since writing to compressed clusters is not generally possible, allocate a new standard (non-compressed) cluster if compressed L2 entry is encountered; then decompress compressed cluster into new cluster; then return offset inside new cluster to the caller. Processing of standard clusters is not changed. Signed-off-by: Eugene Korenevsky <[email protected]>
Add tests: - zlib: test_virtio_block_qcow2_zlib() - zstd: test_virtio_block_qcow2_zstd() Both these tests use zlib- and zstd-compressed images as OS image. Modify test_virtio_block_qcow2_backing_file() test: it is practical to test qcow2 file-backing with compression, so use zlib-compressed image as a backing file. Signed-off-by: Eugene Korenevsky <[email protected]>
Signed-off-by: Eugene Korenevsky <[email protected]>
4787c0f to
dc44ecd
Compare
Rebased exactly these reviewed commits on top of |
rbradford
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
e6d31a3
Add support of reading and writing compressed clusters.
Support zlib and zstd compressions.