From 34d7f69f272faee39f7c8bb7a316ed590b868f36 Mon Sep 17 00:00:00 2001 From: David Stevens Date: Mon, 21 Nov 2022 14:49:19 +0900 Subject: [PATCH] swap: fix for VMs with highmem Handle the case where VM memory is split up into multiple regions by taking into account the region end when swapping out memory. Also rename SwapFile.len to SwapFile.num_pages to make the units clear. BUG=b:215093219 TEST=cargo test --package base TEST=enable/disable swap on VM with >4GB of memory Change-Id: I8dcdadd39c3f7cbd22c1c042498153966a1a0431 Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/4040653 Commit-Queue: David Stevens Commit-Queue: Keiichi Watanabe Reviewed-by: Shin Kawamura Reviewed-by: Keiichi Watanabe --- base/src/sys/unix/file.rs | 38 +++++++++++++++++++++++++++++++++----- swap/src/file.rs | 4 ++-- swap/src/page_handler.rs | 11 ++++++----- 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/base/src/sys/unix/file.rs b/base/src/sys/unix/file.rs index f34b1c77f4..44c1d869a2 100644 --- a/base/src/sys/unix/file.rs +++ b/base/src/sys/unix/file.rs @@ -35,6 +35,7 @@ fn lseek(fd: &dyn AsRawDescriptor, offset: u64, option: LseekOption) -> Result { fd: &'a dyn AsRawDescriptor, offset: u64, + end: u64, } impl<'a> FileDataIterator<'a> { @@ -44,13 +45,24 @@ impl<'a> FileDataIterator<'a> { /// /// * `fd` - the [trait@AsRawDescriptor] of the file /// * `offset` - the offset to start traversing from. - pub fn new(fd: &'a dyn AsRawDescriptor, offset: u64) -> Self { - Self { fd, offset } + /// * `len` - the len of the region over which to iterate + pub fn new(fd: &'a dyn AsRawDescriptor, offset: u64, len: u64) -> Self { + Self { + fd, + offset, + end: offset + len, + } } fn find_next_data(&self) -> Result>> { let offset_data = match lseek(self.fd, self.offset, LseekOption::Data) { - Ok(offset) => offset, + Ok(offset) => { + if offset >= self.end { + return Ok(None); + } else { + offset + } + } Err(e) => { return match e.errno() { libc::ENXIO => Ok(None), @@ -60,7 +72,7 @@ impl<'a> FileDataIterator<'a> { }; let offset_hole = lseek(self.fd, offset_data, LseekOption::Hole)?; - Ok(Some(offset_data..offset_hole)) + Ok(Some(offset_data..offset_hole.min(self.end))) } } @@ -99,11 +111,27 @@ mod tests { file.write_at(&[1_u8], 2 * pagesize() as u64).unwrap(); file.write_at(&[1_u8], (4 * pagesize() - 1) as u64).unwrap(); - let iter = FileDataIterator::new(&file, 0); + let iter = FileDataIterator::new(&file, 0, 4 * pagesize() as u64); let result: Vec> = iter.collect(); assert_eq!(result.len(), 2); assert_eq!(result[0], 0..(pagesize() as u64)); assert_eq!(result[1], (2 * pagesize() as u64)..(4 * pagesize() as u64)); } + + #[test] + fn file_data_iterator_subrange() { + let file = tempfile::tempfile().unwrap(); + + file.write_at(&[1_u8], 0).unwrap(); + file.write_at(&[1_u8], pagesize() as u64).unwrap(); + file.write_at(&[1_u8], 2 * pagesize() as u64).unwrap(); + file.write_at(&[1_u8], 4 * pagesize() as u64).unwrap(); + + let iter = FileDataIterator::new(&file, pagesize() as u64, pagesize() as u64); + + let result: Vec> = iter.collect(); + assert_eq!(result.len(), 1); + assert_eq!(result[0], (pagesize() as u64)..(2 * pagesize() as u64)); + } } diff --git a/swap/src/file.rs b/swap/src/file.rs index 457f8497a7..1303cbacbb 100644 --- a/swap/src/file.rs +++ b/swap/src/file.rs @@ -104,7 +104,7 @@ impl SwapFile { } /// Returns the total count of managed pages. - pub fn len(&self) -> usize { + pub fn num_pages(&self) -> usize { self.state_list.len() } @@ -255,7 +255,7 @@ mod tests { let dir_path = tempfile::tempdir().unwrap(); let swap_file = SwapFile::new(dir_path.path(), 200).unwrap(); - assert_eq!(swap_file.len(), 200); + assert_eq!(swap_file.num_pages(), 200); } #[test] diff --git a/swap/src/page_handler.rs b/swap/src/page_handler.rs index a03cbe4282..4d29396300 100644 --- a/swap/src/page_handler.rs +++ b/swap/src/page_handler.rs @@ -121,7 +121,8 @@ impl PageHandler { // sequential search the corresponding page map from the list. It should be fast enough // because there are a few regions (usually only 1). self.regions.iter().position(|region| { - region.head_page_idx <= page_idx && page_idx < region.head_page_idx + region.file.len() + region.head_page_idx <= page_idx + && page_idx < region.head_page_idx + region.file.num_pages() }) } @@ -147,7 +148,7 @@ impl PageHandler { // find an overlaping region match self.regions.iter().position(|region| { if region.head_page_idx < head_page_idx { - region.head_page_idx + region.file.len() > head_page_idx + region.head_page_idx + region.file.num_pages() > head_page_idx } else { region.head_page_idx < head_page_idx + num_of_pages } @@ -158,7 +159,7 @@ impl PageHandler { Err(Error::RegionOverlap( address_range.clone(), self.page_idx_to_addr(region.head_page_idx) - ..(self.page_idx_to_addr(region.head_page_idx + region.file.len())), + ..(self.page_idx_to_addr(region.head_page_idx + region.file.num_pages())), )) } None => { @@ -312,8 +313,8 @@ impl PageHandler { if self.regions[region_position].head_page_idx != head_page_idx { return Err(Error::InvalidAddress(base_addr)); } - let region_size = self.regions[region_position].file.len() << self.pagesize_shift; - let file_data = FileDataIterator::new(memfd, base_offset); + let region_size = self.regions[region_position].file.num_pages() << self.pagesize_shift; + let file_data = FileDataIterator::new(memfd, base_offset, region_size as u64); for data_range in file_data { // assert offset is page aligned