Introduce VIRTIO_BLK_F_SEG_MAX#6885
Conversation
|
The VHDX test on ARM64 is broken by this. Need to investigate. |
And on x86-64 too - so I don't think it's architecture specific! |
Just noticed that. QCOW tests are passing, so this is probably a latent bug in VHDX implementation. |
|
I will resend this PR after #6890 is merged. |
|
There is another check we should add in all the Virtio device config validation function. The queue size should be a power of 2. This PR introduces a new |
liuw
force-pushed
the
blk-seg-max
branch
2 times, most recently
from
c69522a to
ba31f83
Compare
|
@cloud-hypervisor/cloud-hypervisor-reviewers any more comments on this PR? |
|
LGTM |
likebreath
left a comment
likebreath
left a comment
There was a problem hiding this comment.
Good work. I can see single queue tests benefit more comparing with multiple queue tests, which is kind of expected.
For the random read/write tests, they are seeing the most significant improvements, and basically matching up with the sequential tests performance. Do you think this is expected?
virtio-devices/src/block.rs
Outdated
| physical_block_exp, | ||
| min_io_size: (topology.minimum_io_size / logical_block_size) as u16, | ||
| opt_io_size: (topology.optimal_io_size / logical_block_size) as u32, | ||
| seg_max: (queue_size - 2) as u32, |
There was a problem hiding this comment.
Can you please explain why the seg_max is set to this value?
There was a problem hiding this comment.
A request is consist of at least one our header and one in header, IIRC. What's left in the queue can be used for data segments.
There was a problem hiding this comment.
Thank you for the explanation. Can you please share some pointers for a more detailed context? I always find virtio spec way too concise to understand by itself.
There was a problem hiding this comment.
I looked at QEMU code. It was always like that since the beginning with no explanation.
The closest I can find is virtblk_add_req in Linux.
My only expectation is the performance will improve a lot. I cannot say one way or another whether random rws can be better or worse than seq rws. |
This allows the guest to put in more than one segment per request. It can improve the throughput of the system. Introduce a new check to make sure the queue size configured by the user is large enough to hold at least one segment. Signed-off-by: Wei Liu <liuwe@microsoft.com>
The size was set to one because without VIRTIO_BLK_F_SEG_MAX, the guest only used one data descriptor per request. The value 32 is empirically derived from booting a guest. This value eliminates all SmallVec allocations observable by DHAT. Signed-off-by: Wei Liu <liuwe@microsoft.com>
Yes, the performance improvements are substantial and awesome. No doubt on that. It was the random read/write matching up with sequential read/write across the board that puzzled me. I wanted to see if you have any insights. (I always thought random read/write are supposed to be much slower.) |
|
@TimePrinciple @rbradford The risc-v runner is offline. Would you please take a look? Thanks. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
test_virtio_block_vhdx is failing on musl on AMD - this might just be a timing flake |
1f7b809
I've synced the message in our Slack channel, and unfortunately it looks like the process is about to take much longer than expected(Our lab is building a new server room). I have moved that machine to my office and get it online for now(and will be moved into server room when it's completed) 🙂 |
6 participants
Significant improvements in block device performance across the board.
With this new feature:
Without:
Cc @russell-islam @xietou