virtio-blk: add discard request support by JackThomson2 · Pull Request #6142 · firecracker-microvm/firecracker

JackThomson2 · 2026-08-20T10:41:09Z

Changes

Update of the PR: #5908

To fix a few warning and update against main

Also fixed a bug in which the config space was no longer calling .to_le()

Reason

...

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

This functionality cannot be added in rust-vmm.

codecov · 2026-08-24T15:13:43Z

Codecov Report

❌ Patch coverage is 67.64706% with 44 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.97%. Comparing base (3c6414c) to head (8765668).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/vmm/src/devices/virtio/block/virtio/io/mod.rs	0.00%	19 Missing ⚠️
.../vmm/src/devices/virtio/block/virtio/io/sync_io.rs	58.82%	14 Missing ⚠️
src/vmm/src/devices/virtio/block/virtio/request.rs	88.46%	6 Missing ⚠️
...vmm/src/devices/virtio/block/virtio/io/async_io.rs	0.00%	3 Missing ⚠️
.../vmm/src/devices/virtio/block/vhost_user/device.rs	66.66%	1 Missing ⚠️
src/vmm/src/devices/virtio/block/virtio/device.rs	95.65%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6142      +/-   ##
==========================================
- Coverage   83.03%   82.97%   -0.07%     
==========================================
  Files         277      277              
  Lines       31123    31254     +131     
==========================================
+ Hits        25844    25933      +89     
- Misses       5279     5321      +42

Flag	Coverage Δ
5.10-m5n.metal	`83.20% <67.64%> (-0.08%)`	⬇️
5.10-m6a.metal	`82.56% <67.64%> (-0.08%)`	⬇️
5.10-m6g.metal	`80.02% <67.64%> (-0.07%)`	⬇️
5.10-m6i.metal	`83.20% <67.64%> (-0.08%)`	⬇️
5.10-m7a.metal-48xl	`82.55% <67.64%> (-0.08%)`	⬇️
5.10-m7g.metal	`80.02% <67.64%> (-0.07%)`	⬇️
5.10-m7i.metal-24xl	`83.17% <67.64%> (-0.08%)`	⬇️
5.10-m7i.metal-48xl	`83.17% <67.64%> (-0.09%)`	⬇️
5.10-m8g.metal-24xl	`80.01% <67.64%> (-0.07%)`	⬇️
5.10-m8g.metal-48xl	`80.01% <67.64%> (-0.07%)`	⬇️
5.10-m8i.metal-48xl	`83.16% <67.64%> (-0.09%)`	⬇️
5.10-m8i.metal-96xl	`83.17% <67.64%> (-0.08%)`	⬇️
5.10-m9g.metal-48xl	`80.01% <67.64%> (-0.07%)`	⬇️
6.1-m5n.metal	`83.22% <67.64%> (-0.08%)`	⬇️
6.1-m6a.metal	`82.58% <67.64%> (-0.08%)`	⬇️
6.1-m6g.metal	`80.02% <67.64%> (-0.07%)`	⬇️
6.1-m6i.metal	`83.22% <67.64%> (-0.08%)`	⬇️
6.1-m7a.metal-48xl	`82.57% <67.64%> (-0.08%)`	⬇️
6.1-m7g.metal	`80.02% <67.64%> (-0.07%)`	⬇️
6.1-m7i.metal-24xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.1-m7i.metal-48xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.1-m8g.metal-24xl	`80.01% <67.64%> (-0.07%)`	⬇️
6.1-m8g.metal-48xl	`80.01% <67.64%> (-0.07%)`	⬇️
6.1-m8i.metal-48xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.1-m8i.metal-96xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.1-m9g.metal-48xl	`80.01% <67.64%> (-0.07%)`	⬇️
6.18-m5n.metal	`83.21% <67.64%> (-0.09%)`	⬇️
6.18-m6a.metal	`82.58% <67.64%> (-0.08%)`	⬇️
6.18-m6g.metal	`80.12% <67.64%> (-0.07%)`	⬇️
6.18-m6i.metal	`83.21% <67.64%> (-0.08%)`	⬇️
6.18-m7a.metal-48xl	`82.57% <67.64%> (-0.08%)`	⬇️
6.18-m7g.metal	`80.12% <67.64%> (-0.07%)`	⬇️
6.18-m7i.metal-24xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.18-m7i.metal-48xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.18-m8g.metal-24xl	`80.12% <67.64%> (-0.07%)`	⬇️
6.18-m8g.metal-48xl	`80.12% <67.64%> (-0.07%)`	⬇️
6.18-m8i.metal-48xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.18-m8i.metal-96xl	`83.23% <67.64%> (-0.08%)`	⬇️
6.18-m9g.metal-48xl	`80.12% <67.64%> (-0.07%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ShadowCurse

overall lgtm

ShadowCurse · 2026-09-02T13:29:30Z

+/// Maximum sectors accepted in a single discard range.
+///
+/// Keep this bounded so guest fstrim is split into predictable chunks instead
+/// of letting one large discard ioctl block the VMM thread for too long.
+pub const MAX_DISCARD_SECTORS: u32 = (128_u32 << 20) / SECTOR_SIZE;
+/// Maximum number of segments accepted in a single discard request.
+pub const MAX_DISCARD_SEG: u32 = 1;
+/// Discard alignment, expressed in 512-byte sectors.
+pub const DISCARD_SECTOR_ALIGNMENT: u32 = 1;


Since other defaults for the ConfigSpace are in the default impl, we better move these there as well.

done, just kept MAX_DISCARD_SECTORS as it's also used in request.rs

ShadowCurse · 2026-09-02T13:40:07Z

+        } else if config.discard {
+            avail_features |= 1u64 << VIRTIO_BLK_F_DISCARD;


I would move this out of else branch so just

if config.is_read_only { ... } if config.discard { ... }

Since the check for is_read_only && discrard is just a couple lines above

ShadowCurse · 2026-09-02T13:50:43Z

+            config_space.discard_sector_alignment =
+                query_discard_alignment(disk_properties.file_engine.file())
+                    .map_err(|err| {
+                        VirtioBlockError::BackingFile(err, disk_properties.file_path.clone())
+                    })?
+                    .to_le();


I don's see why you need to separately query alignment here.
Instead this logic should just be in a code bellow:

if config.blk_size.is_none() && config.topology.is_none() { if let Some((blk_size, topology)) = query_blk_attrs(disk_properties.file_engine.file()) .and_then(calculate_blk_size_and_topology) { config_space.blk_size = blk_size; config_space.topology = topology; config_space.discard_sector_alignment = blk_size / SECTOR_SIZE; } } else { if let Some(blk_size) = config.blk_size { config_space.blk_size = blk_size; config_space.discard_sector_alignment = blk_size / SECTOR_SIZE; } if let Some(topology) = config.topology { config_space.topology = topology; } }

We can leave if blk_size < SECTOR_SIZE || !blk_size.is_multiple_of(SECTOR_SIZE) { out for now since we already don't have this check.

If the user specify only topology then we never set the discard_sector_alignment correctly and I'd also prefer to query rather than trust the user to put the correct blk_size?

well, the reason we expose blk_size to the user is for them to configure it however they want. I think it is reasonable to only query if user did not provide anything. This is the current behavior we have anyway.

applied code suggestion, removed query helper

ShadowCurse · 2026-09-02T13:52:56Z

+        )?;
        self.config_space.capacity = self.disk.nsectors.to_le(); // virtio_block_config_space();
+        if let Some(new_discard_alignment) = new_discard_alignment {
+            self.config_space.discard_sector_alignment = new_discard_alignment.to_le();


Guest will only check the capacity field on the config update, so there is no reason to do this dance with querying new alignment.

The query new alignment check is to ensure that the new disk alignment divides the previous disk alignment otherwise the guest can submit valid discard requests that the new backend rejects.

It is fine if guest will reject them. We will detect this and report to the guest about the failures. It is user responsibility to replace backing file with something that has same logical size and so same discard alignment. We should add this to the doc I think.

removed alignment check, updated doc accordingly

ShadowCurse · 2026-09-02T13:54:01Z

+            max_discard_sectors: MAX_DISCARD_SECTORS.to_le(),
+            max_discard_seg: MAX_DISCARD_SEG.to_le(),
+            discard_sector_alignment: DISCARD_SECTOR_ALIGNMENT.to_le(),


also more of a nit: we are on little endian anyway, so no reason to call to_le

ShadowCurse · 2026-09-02T13:58:07Z

+        let discard_sector_alignment = if avail_features & (1u64 << VIRTIO_BLK_F_DISCARD) != 0 {
+            query_discard_alignment(disk_properties.file_engine.file())
+                .map_err(|err| VirtioBlockError::BackingFile(err, state.disk_path.clone()))?
+        } else {
+            DISCARD_SECTOR_ALIGNMENT
+        };


just store state.virtio_state.discard_sector_alignment directly

ShadowCurse · 2026-09-02T14:11:25Z

  new VIRTIO_BLK_F_BLK_SIZE and VIRTIO_BLK_F_TOPOLOGY features to the
  virtio-block device. More information is in the new [block](docs/block.md)
  documentation.
+- [#5908](https://github.com/firecracker-microvm/firecracker/pull/5908): Add


If we are touching this PR anyway, how about moving CHANGELOG and docs changes into a separate commits?

Extend the virtio-blk config space through the discard limit fields so the device can report discard capability details when the feature is enabled. [jackabt@amazon.com: rebased onto main and reconciled with the read-only config space test] Signed-off-by: Jonas Savulionis <jonas@esnet.lt> Signed-off-by: Jack Thomson <jackabt@amazon.com>

Add opt-in virtio-blk discard support for sync file-backed drives. The device advertises VIRTIO_BLK_F_DISCARD only when enabled, validates guest discard ranges once during request parsing, and executes discard through BLKDISCARD for block devices or fallocate hole punching for regular files. Derive discard alignment from the logical block size advertised to the guest and persist that alignment across snapshot restore. Disk image updates retain the original value. Reject discard for read-only drives and the async IO engine for now, and return UNSUPP if a guest submits discard without negotiating the feature. [jackabt@amazon.com: rebased onto main and updated the config round-trip expectations for the normalised discard field in test_block_config, resources, and the two device manager persistence tests] Signed-off-by: Jonas Savulionis <jonas@esnet.lt> Signed-off-by: Rekas <aure369@gmail.com> Signed-off-by: Jack Thomson <jackabt@amazon.com> Signed-off-by: Pierre Bertholom <pbertho@amazon.com>

Allow the sync virtio-blk discard path to punch holes in regular files and issue BLKDISCARD for block-device backed drives. [jackabt@amazon.com: rebased onto main] Signed-off-by: Jonas Savulionis <jonas@esnet.lt> Signed-off-by: Rekas <aure369@gmail.com> Signed-off-by: Jack Thomson <jackabt@amazon.com>

Cover the discard drive API by validating successful configuration, guest-visible discard granularity, and rejection of unsupported read-only or async combinations. Add a guest functional test that writes data to a file-backed drive, issues blkdiscard, and verifies host block allocation decreases. [jackabt@amazon.com: rebased onto main] Signed-off-by: Jonas Savulionis <jonas@esnet.lt> Signed-off-by: Rekas <aure369@gmail.com> Signed-off-by: Jack Thomson <jackabt@amazon.com> Signed-off-by: Pierre Bertholom <pbertho@amazon.com>

Document the supported discard configurations, advertised alignment rules, and user responsibility for compatible block device updates. Add the feature to the changelog. [jackabt@amazon.com: rebased onto main and moved the changelog entry to the unreleased section] Signed-off-by: Jonas Savulionis <jonas@esnet.lt> Signed-off-by: Rekas <aure369@gmail.com> Signed-off-by: Jack Thomson <jackabt@amazon.com> Signed-off-by: Pierre Bertholom <pbertho@amazon.com>

JackThomson2 force-pushed the pr-5908-rebase branch 2 times, most recently from 5d91a17 to 9da880c Compare August 24, 2026 15:08

JackThomson2 force-pushed the pr-5908-rebase branch from 9da880c to 09dd359 Compare August 24, 2026 15:41

JackThomson2 added the Status: Awaiting review Indicates that a pull request is ready to be reviewed label Aug 24, 2026

ShadowCurse reviewed Aug 25, 2026

View reviewed changes

Comment thread src/vmm/src/devices/virtio/block/virtio/device.rs Outdated

Comment thread src/vmm/src/devices/virtio/block/virtio/io/sync_io.rs Outdated

JackThomson2 force-pushed the pr-5908-rebase branch from 09dd359 to f05fee0 Compare August 26, 2026 16:17

ShadowCurse previously approved these changes Aug 27, 2026

View reviewed changes

fabian-kessel mentioned this pull request Aug 31, 2026

feat(v1.16.1): backport virtio-blk discard (upstream PR #6142) philipparndt/firecracker#1

Open

PierreBertholom dismissed ShadowCurse’s stale review via 8e495da September 1, 2026 15:49

PierreBertholom force-pushed the pr-5908-rebase branch 2 times, most recently from 8e495da to c9f0926 Compare September 1, 2026 17:02

ShadowCurse reviewed Sep 2, 2026

View reviewed changes

bacarrdy and others added 4 commits September 2, 2026 17:08

PierreBertholom force-pushed the pr-5908-rebase branch from c9f0926 to 8d3fb21 Compare September 2, 2026 16:09

PierreBertholom force-pushed the pr-5908-rebase branch from 8d3fb21 to 8765668 Compare September 2, 2026 16:24

		} else if config.discard {
		avail_features \|= 1u64 << VIRTIO_BLK_F_DISCARD;

Uh oh!

Conversation

JackThomson2 commented Aug 20, 2026

Changes

Reason

License Acceptance

PR Checklist

Uh oh!

codecov Bot commented Aug 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ShadowCurse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov Bot commented Aug 24, 2026 •

edited

Loading