Punch holes for shared memory discard#16
Conversation
PR SummaryMedium Risk Overview Adds memfd-backed test helpers and new unit tests (4K and 2M hugepage variants) asserting discarded ranges read back as zeros and that unaligned discards error, and updates the dirty-bitmap integration test to pass an explicit page size to Reviewed by Cursor Bugbot for commit ecc1b51. Bugbot is set up for automated code reviews on this repo. Configure here. |
bf6c99b to
c283e22
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Hugetlb punch hole granularity
- Added hugetlbfs detection via fstatfs and aligned discard ranges to huge page boundaries to ensure fallocate actually frees backing pages for hugetlb-backed memfd.
Or push these changes by commenting:
@cursor push 65591a7673
Preview (65591a7673)
diff --git a/src/vmm/src/vstate/memory.rs b/src/vmm/src/vstate/memory.rs
--- a/src/vmm/src/vstate/memory.rs
+++ b/src/vmm/src/vstate/memory.rs
@@ -8,6 +8,7 @@
use std::fs::File;
use std::io::SeekFrom;
use std::ops::Deref;
+use std::os::unix::io::AsRawFd;
use std::sync::{Arc, Mutex};
use bitvec::vec::BitVec;
@@ -428,12 +429,39 @@
)));
};
+ let (aligned_offset, aligned_len) = {
+ // Check if the memfd is backed by hugetlbfs and adjust alignment accordingly.
+ // SAFETY: fstatfs is called with a valid file descriptor.
+ let mut stat: libc::statfs = unsafe { std::mem::zeroed() };
+ let ret = unsafe { libc::fstatfs(file_offset.file().as_raw_fd(), &mut stat) };
+
+ if ret == 0 && stat.f_type == libc::HUGETLBFS_MAGIC as i64 {
+ // For hugetlbfs, fallocate punch hole rounds offset up and end down
+ // to huge page boundaries. We must align explicitly to avoid
+ // sub-huge-page discards succeeding without actually freeing memory.
+ let hpage_size = stat.f_bsize as u64;
+ let aligned_start = (offset + hpage_size - 1) / hpage_size * hpage_size;
+ let range_end = offset + usize_to_u64(len);
+ let aligned_end = range_end / hpage_size * hpage_size;
+
+ if aligned_start >= aligned_end {
+ // No complete huge pages in range; nothing to discard.
+ return Ok(());
+ }
+
+ (aligned_start, aligned_end - aligned_start)
+ } else {
+ // Regular memfd or fstatfs failed; use caller's range as-is.
+ (offset, usize_to_u64(len))
+ }
+ };
+
fallocate::fallocate(
file_offset.file(),
FallocateMode::PunchHole,
true,
- offset,
- usize_to_u64(len),
+ aligned_offset,
+ aligned_len,
)
.map_err(|err| {
error!("discard_range: punching hole failed: {err:?}");You can send follow-ups to the cloud agent here.
c283e22 to
8e793bf
Compare
3de1861 to
bd473c4
Compare
Previous commit (cd3fe9a) changed the signature of ArchVm::get_dirty_bitmap() to get a page_size argument, but corresponding integration test was not updated to match this change. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
bd473c4 to
be89e3d
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Hugetlbfs alignment check always defaults to 4K pages
- Added with_hugetlbfs(true) call in create() function when MAP_HUGETLB flag is detected, ensuring is_hugetlbfs() returns Some(true) and alignment checks use correct 2MB page size for hugetlbfs-backed regions.
Or push these changes by commenting:
@cursor push 51dbbc73d6
Preview (51dbbc73d6)
diff --git a/src/vmm/src/vstate/memory.rs b/src/vmm/src/vstate/memory.rs
--- a/src/vmm/src/vstate/memory.rs
+++ b/src/vmm/src/vstate/memory.rs
@@ -542,6 +542,10 @@
.with_mmap_prot(libc::PROT_READ | libc::PROT_WRITE)
.with_mmap_flags(libc::MAP_NORESERVE | mmap_flags);
+ if mmap_flags & libc::MAP_HUGETLB != 0 {
+ builder = builder.with_hugetlbfs(true);
+ }
+
if let Some(ref file) = file {
let file_offset = FileOffset::from_arc(Arc::clone(file), offset);You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit be89e3d. Configure here.
GuestRegionMmapExt::discard_range() is used to deallocate guest memory that we don't use any more, for example when we use balloon inflation or free page reporting/hinting. There is the implicit requirement that the range we are discarding is aligned (both starting address and lenght) to the page size used to back the guest memory. If this alignment is not respected by the caller, we can end up with undefined behaviour. For example, if we use huge pages to back memory but we receive from the guest regions to discard that are 4K pages aligned, we might end up removing memory that we are not meant to. This currently doesn't happen but the requirement is not explicitly encoded in the type system. Add a check for these requirements and return an error when they are not met. This way, we can't shoot ourselves in the foot in the future. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
Use fallocate(PUNCH_HOLE|KEEP_SIZE) for MAP_SHARED file-backed guest memory so memfd-backed balloon hinting/reporting clears the shared backing instead of only dropping PTEs. Signed-off-by: Babis Chalios <babis.chalios@e2b.dev>
be89e3d to
ecc1b51
Compare


Use hole punching for
discard_range()on MAP_SHARED file-backed memory so memfd-backed free-page hinting/reporting clears the shared backing instead of only dropping PTEs.Tested with
cargo test -p vmm --lib vstate::memory::tests::test_discard_range -- --nocapture.