feat(virtio-block): Add fallocate support by LDagnachew · Pull Request #5511 · firecracker-microvm/firecracker

LDagnachew · 2025-11-11T22:57:55Z

Changes

Refactored unmap/fstrim/discard logic from deprecated branch + PR (see here for implementation details).
Added/Updated Functional Testing for fallocate.

Reason

Attempting to resolve Issue #2708.

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.

LDagnachew · 2025-11-15T01:10:29Z

also disclaimer this still needs to be style checked, but this should be the near final implementation, other than writing a more rigorous testing suite.

LDagnachew · 2025-11-17T17:24:43Z

@bchalios @ShadowCurse whenever you have time do y'all mind taking a look? I really want to bring this issue to the finish line, thanks!

ShadowCurse

Sorry for late review. Did a first pass and left some comments. One thing we need to think about is the fact that fallocate is not supported on all filesystems, so either we need to make this feature optional for the device, or make failures to fallocate to just print error logs and not return actual errors.

resources/seccomp/aarch64-unknown-linux-musl.json

src/vmm/src/devices/virtio/block/virtio/io/async_io.rs

src/vmm/src/devices/virtio/block/virtio/io/mod.rs

src/vmm/src/devices/virtio/block/virtio/io/sync_io.rs

src/vmm/src/devices/virtio/block/virtio/request.rs

ShadowCurse · 2025-11-24T17:49:55Z

src/vmm/src/devices/virtio/block/virtio/request.rs

+                    if end_sector > num_disk_sectors {
+                        return Err(VirtioBlockError::BeyondDiskSize);
+                    }


Instead of returnning an error, you can just ignore invalid segment and print an error log.

My bad, this was incorrect suggestion, we better to abort the whole request if we find invalid segment, so Err(..) this should be.

LDagnachew · 2025-12-01T20:22:19Z

Hi, sorry for the late response, but I will get this handled within the next day or two and mark for another review. Thanks!

- Updated unmap/fstrim/discard deprecated logic. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

- fallocate unblocked in seccomp, thus allowing discard requests. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

Update Python files to comply with Black and pylint checks. These changes are formatting-only and do not alter functionality. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

- added safety comments + minor error handling. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

ShadowCurse

On top of everything, we need to figure out the way to test if we can even call fallocate on the file since some filesystems do not support this operation. Most likely we should do a dummy fallocate call that does not actually change the file as a check.
Also this feature should be user configurable, just like others, so the block API should have a new field to enable this feature.

ShadowCurse · 2026-02-13T12:12:07Z

src/vmm/src/devices/virtio/block/virtio/device.rs

@@ -27,7 +27,7 @@ use crate::devices::virtio::block::CacheType;
 use crate::devices::virtio::block::virtio::metrics::{BlockDeviceMetrics, BlockMetricsPerDevice};


commit: style: fix Python formatting and lint issues
everything here belongs to the first implementation commit. Please don't do separate "running a formatter" type of commits

ShadowCurse · 2026-02-13T12:13:46Z

src/vmm/src/devices/virtio/block/virtio/io/async_io.rs

@@ -110,6 +110,7 @@ impl AsyncFileEngine {
        Ok(())


commit: style: replace errors for fallocate/discard with error logs
same here, merge this into main implementation commit.

ShadowCurse · 2026-02-20T13:41:02Z

src/vmm/src/io_uring/operation/mod.rs

+            opcode: OpCode::Fallocate,
+            addr: None,
+            len: Some(len),
+            flags: 0,


libc::FALLOC_FL_PUNCH_HOLE | libc::FALLOC_FL_KEEP_SIZE should be in flags

ShadowCurse · 2026-02-20T13:43:12Z

src/vmm/src/devices/virtio/block/virtio/request.rs

            if req.r#type != RequestType::Flush {
                return Err(VirtioBlockError::DescriptorChainTooShort);
            }
+            data_desc = desc;


this seems wrong

ShadowCurse · 2026-02-20T13:48:37Z

src/vmm/src/devices/virtio/block/virtio/request.rs

+                status_desc = data_desc
+                    .next_descriptor()
+                    .ok_or(VirtioBlockError::DescriptorChainTooShort)?;
+                if !status_desc.is_write_only() {
+                    return Err(VirtioBlockError::UnexpectedReadOnlyDescriptor);
+                }


status_desc is already obtained from the data_desc above and there is a check for status_desc just bellow.

ShadowCurse · 2026-02-20T14:22:08Z

src/vmm/src/devices/virtio/block/virtio/io/sync_io.rs

+        let off_i64 = i64::try_from(offset).map_err(|_| {
+            SyncIoError::Discard(std::io::Error::new(
+                std::io::ErrorKind::InvalidInput,
+                "offset overflow",
+            ))
+        })?;


I wonder if there is a way to remove this. The syscall wants, i64, but in reality it is just 64 bit value which will be checked by the kernel in any case.

ShadowCurse · 2026-02-20T14:32:59Z

tests/integration_tests/functional/test_discard.py

+    vm.ssh.run("mount -o remount,discard /")
+    exit_code, stdout, _ = vm.ssh.run("mount | grep /dev/vda")
+    assert (
+        exit_code == 0
+    ), f"Failed to remount root filesystem with discard option: {stdout}"
+
+    exit_code, stdout, _ = vm.ssh.run("fstrim -v /")
+    assert exit_code == 0, f"fstrim -v failed: {stdout}"
+    assert "trimmed" in stdout, f"Unexpected fstrim output: {stdout}"


You test wrong block device (should be vdb for the test_disk you added).

There is no check for the actual discard capability after mount | grep ...command.

What should happen after fstrim is called? How do you test that the discrard was correctly executed? Maybe you can check FC metrics and check the underlying test_disk with stat or smth to see if the hole was successfully created.

ShadowCurse · 2026-02-20T14:33:39Z

tests/integration_tests/functional/test_discard.py

+    exit_code, stdout, _ = vm.ssh.run("fstrim -v /")
+    assert exit_code == 0, f"fstrim -v failed: {stdout}"
+    assert "trimmed" in stdout, f"Unexpected fstrim output: {stdout}"
+    vm.kill()


There is no need to explicitly kill the vm. It will be killed automatically at the end of the test

ShadowCurse · 2026-02-20T14:34:32Z

tests/integration_tests/functional/test_discard.py

+import host_tools.drive as drive_tools
+
+
+def test_discard_support(uvm_plain_rw):


Because discard is implemented in both sync and async engines, you need to parametrize the test.

ShadowCurse · 2026-02-20T14:36:31Z

src/vmm/src/devices/virtio/block/virtio/request.rs

+                    if end_sector > num_disk_sectors {
+                        return Err(VirtioBlockError::BeyondDiskSize);
+                    }


My bad, this was incorrect suggestion, we better to abort the whole request if we find invalid segment, so Err(..) this should be.

bchalios mentioned this pull request Nov 14, 2025

Unmap/discard/fstrim #2708

Open

LDagnachew force-pushed the unmap_fstrim_discard branch 2 times, most recently from 41e7941 to abda29f Compare November 15, 2025 02:37

LDagnachew marked this pull request as ready for review November 15, 2025 02:39

LDagnachew changed the title ~~feat(virtio-block): Add fallocate support (draft)~~ feat(virtio-block): Add fallocate support Nov 15, 2025

ShadowCurse reviewed Nov 24, 2025

View reviewed changes

JamesC1305 added the Status: Awaiting author Indicates that an issue or pull request requires author action label Dec 10, 2025

LDagnachew force-pushed the unmap_fstrim_discard branch 2 times, most recently from 8fd863f to dacdcff Compare January 6, 2026 21:07

LDagnachew added 4 commits January 7, 2026 02:11

virtio-blk: Refactoring unmap/fstrim/discard logic

47348e8

- Updated unmap/fstrim/discard deprecated logic. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

fix: Unblock virtio-blk discard by permitting fallocate in seccomp

bfda007

- fallocate unblocked in seccomp, thus allowing discard requests. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

style: fix Python formatting and lint issues

3458e54

Update Python files to comply with Black and pylint checks. These changes are formatting-only and do not alter functionality. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

style: replace errors for fallocate/discard with error logs

dd0a7e1

- added safety comments + minor error handling. Signed-off-by: LDagnachew <leulmdagnachew@gmail.com>

LDagnachew force-pushed the unmap_fstrim_discard branch from f23cb9b to dd0a7e1 Compare January 7, 2026 02:19

LDagnachew requested a review from ShadowCurse January 7, 2026 02:20

ShadowCurse reviewed Feb 20, 2026

View reviewed changes

		@@ -27,7 +27,7 @@ use crate::devices::virtio::block::CacheType;
		use crate::devices::virtio::block::virtio::metrics::{BlockDeviceMetrics, BlockMetricsPerDevice};

		import host_tools.drive as drive_tools


		def test_discard_support(uvm_plain_rw):

Conversation

LDagnachew commented Nov 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Reason

License Acceptance

PR Checklist

Uh oh!

LDagnachew commented Nov 15, 2025

Uh oh!

LDagnachew commented Nov 17, 2025

Uh oh!

ShadowCurse left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LDagnachew commented Dec 1, 2025

Uh oh!

ShadowCurse left a comment

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!

3 participants

LDagnachew commented Nov 11, 2025 •

edited

Loading