Virtiofs clamp max write#3694
Conversation
The guest virtio-fs driver builds single physically-contiguous request buffers up to the negotiated FUSE max_write (1 MiB by default). On a bounce-buffered transport, a guest forced to use the Linux swiotlb can only map 256 KiB per mapping (IO_TLB_SEGSIZE * IO_TLB_SIZE), regardless of the total swiotlb window. The transport then fails to map the larger buffers, producing cascading I/O errors that surface as queue/walker corruption. Add fuse::Session::with_max_write and fuse::DEFAULT_MAX_WRITE so callers can negotiate a smaller max_write, and add VirtioFsDeviceOptions with a max_dma_mapping_size field that clamps the negotiated max_write to the transport's per-mapping DMA limit. The in-VMM path leaves this unset and keeps the full 1 MiB default; only bounce-buffered callers (e.g. WSL) opt into the smaller size. Also fold the redundant num_request_queues field into the device's config and add tests covering the clamping behavior.
There was a problem hiding this comment.
Pull request overview
This PR addresses virtio-fs I/O failures on bounce-buffered transports by ensuring the negotiated FUSE max_write does not exceed the transport’s per-DMA-mapping limit (e.g., Linux swiotlb’s effective 256 KiB cap), and refactors virtio-fs device construction to use an options struct.
Changes:
- Add
fuse::DEFAULT_MAX_WRITEandfuse::Session::with_max_writeto allow callers to negotiate a smallermax_write. - Introduce
VirtioFsDeviceOptionswithmax_dma_mapping_sizeto clamp virtio-fsmax_writefor bounce-buffered transports; fold request-queue count into device config usage. - Add unit tests covering
max_writeclamping behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vm/devices/virtio/virtiofs/src/virtio.rs | Adds VirtioFsDeviceOptions, clamps max_write based on per-mapping DMA limits, and updates tests accordingly. |
| vm/devices/support/fs/fuse/src/session.rs | Introduces DEFAULT_MAX_WRITE and Session::with_max_write; updates FUSE_INIT negotiation and adds tests. |
| vm/devices/support/fs/fuse/src/lib.rs | Re-exports DEFAULT_MAX_WRITE for consumers. |
Clamp info.max_write back to the session's negotiated maximum after the filesystem's init callback, so a callback can lower it but never raise it above the transport's per-mapping limit. Update the field and with_max_write doc comments to document this contract, and add a test.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
vm/devices/support/fs/fuse/src/session.rs:96
Session::with_max_writeacceptsmax_write = 0, which will cause FUSE_INIT to advertisemax_write = 0(andmax_pages = 0). That effectively breaks I/O negotiation and can lead to confusing failures later. Since this is a host-provided configuration value, it should be validated up front as a non-zero max write size.
pub fn with_max_write<T>(fs: T, max_write: u32) -> Self
where
T: 'static + Fuse + Send + Sync,
{
Self {
A zero per-mapping DMA limit would negotiate a max_write of 0, producing an unusable device. Treat it as a misconfiguration and assert early during device construction. Document the non-zero requirement and add a test.
Describe max_write as the offered value and hard upper bound rather than the final negotiated value, since a filesystem init callback may lower it. Avoids overstating what the device guarantees.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
vm/devices/support/fs/fuse/src/session.rs:96
Session::with_max_writeacceptsmax_write = 0, which would advertisemax_write = 0/max_pages = 0during FUSE_INIT and likely produce an unusable session. Since this is a transport-configuration API (similar toVirtioFsDeviceOptions::max_dma_mapping_size), treat zero as an invalid configuration and fail fast.
pub fn with_max_write<T>(fs: T, max_write: u32) -> Self
where
T: 'static + Fuse + Send + Sync,
{
Self {
| /// Transports that bounce I/O through a limited DMA window (e.g. a guest | ||
| /// forced to use the Linux swiotlb, which has a hard 256 KiB per-mapping | ||
| /// cap) should negotiate a smaller value via [`Session::with_max_write`] so | ||
| /// the guest never builds request buffers the transport cannot map. |
There was a problem hiding this comment.
Shouldn't this be the responsibility of the guest to not negotiate a size that's bigger than it can support? I.e., isn't this a guest bug?
There was a problem hiding this comment.
From my understanding of the FUSE protocol max_write is chosen entirely by the server in the FUSE_INIT reply — the guest doesn't propose it. The guest kernel just adopts whatever the server offers. The swiotlb limit lives below the FUSE client in the DMA layer, and the guest virtio-fs driver doesn't clamp the accepted max_write to dma_max_mapping_size(). So even if we consider that guest behavior non-ideal, the server is the only party that can actually set a value the transport can satisfy.
However, the comment may be misleading, and needs to be reviesed.
Does that make sense?
There was a problem hiding this comment.
I pushed another iteration with a revised comment. Please let me know if you think a different behavior is needed.
There was a problem hiding this comment.
Shouldn't this be the responsibility of the guest... isn't this a guest bug?
Yes — it is also a guest bug, and the guest side is fixable. I put together a guest patch and validated it in isolation: microsoft/WSL2-Linux-Kernel#261.
It mirrors what virtio-blk already does — clamps the FUSE connection's max_pages_limit by the transport's per-mapping DMA limit (virtio_max_dma_size()), right next to the existing virtqueue-size clamp in virtio_fs_get_tree(). process_init_reply() then caps the accepted max_pages (and transitively max_write) to a value the guest can always DMA-map, regardless of what the server offers. When no DMA limit applies, virtio_max_dma_size() returns SIZE_MAX, so it's a no-op.
Validation (WSL DrvFsTests::WSL2VirtioFs, swiotlb=force active, identical unclamped host server offering 1 MiB max_write in both runs):
| Guest kernel | GetDents |
|---|---|
| inbox (no clamp) | EIO (wedged request vq) |
| patched (#261) | PASS |
So the guest can negotiate down on its own. I'd suggest landing both: this host-side clamp as defense-in-depth for guests we don't control, and the guest clamp as the root-cause fix for WSL.
There was a problem hiding this comment.
If WSL wants to clamp for safety (since it knows swiotlb is in use) then let's make the PR do just that without affecting default OpenVMM behavior.
But yeah, I think we should just get this patch upstream and backported to all the LTS Linux trees--this is a virtio-fs driver bug that we're working around.
Reword the DEFAULT_MAX_WRITE, Session::new, and Session::with_max_write doc comments to say max_write is advertised/dictated by the server rather than negotiated with the guest. The guest adopts whatever the server offers in the FUSE_INIT reply, so the previous "negotiate" wording was misleading.
|
Canceling due to moving transfer size clamping to the guest side (virtio-fs). |
virtiofs: clamp FUSE max_write on bounce-buffered transports
The guest virtio-fs driver builds single physically-contiguous request
buffers up to the negotiated FUSE max_write (1 MiB by default). On a
bounce-buffered transport, a guest forced to use the Linux swiotlb can
only map 256 KiB per mapping (IO_TLB_SEGSIZE * IO_TLB_SIZE), regardless
of the total swiotlb window. The transport then fails to map the larger
buffers, producing cascading I/O errors that surface as queue/walker
corruption.
Add fuse::Session::with_max_write and fuse::DEFAULT_MAX_WRITE so callers
can negotiate a smaller max_write, and add VirtioFsDeviceOptions with a
max_dma_mapping_size field that clamps the negotiated max_write to the
transport's per-mapping DMA limit. The in-VMM path leaves this unset and
keeps the full 1 MiB default; only bounce-buffered callers (e.g. WSL)
opt into the smaller size.
Also fold the redundant num_request_queues field into the device's
config and add tests covering the clamping behavior.