ramdisk: add 4K sector size support with performance optimizations #1892

Copilot · 2025-08-20T20:15:54Z

Adds configurable sector size support to the ramdisk implementation with performance optimizations, enabling 4K sector/block size testing as requested in #1492.

Problem

The ramdisk implementation (disklayer_ram) had hardcoded 512-byte sectors and used inefficient arithmetic operations, preventing testing of storage devices with 4K sectors - a common real-world scenario needed for comprehensive storage testing.

// Before: hardcoded 512-byte sectors with inefficient operations
struct Sector([u8; 512]);
const SECTOR_SIZE: u32 = 512;
let count = buffers.len() / self.sector_size as usize;  // Division
let offset = i * self.sector_size as usize;             // Multiplication

Solution

Made sector size configurable with performance optimizations and proper validation:

// After: configurable sector size with optimized operations
struct Sector(Vec<u8>);
const DEFAULT_SECTOR_SIZE: u32 = 512;

// Pre-computed shift value for efficient operations
sector_shift: u32,  // where 1 << sector_shift == sector_size

// Optimized arithmetic using bit shifts
let count = buffers.len() >> self.sector_shift;         // Right shift (division)
let offset = i << self.sector_shift;                    // Left shift (multiplication)

// Power-of-two validation
if !sector_size.is_power_of_two() {
    return Err(Error::SectorSizeNotPowerOfTwo { sector_size });
}

Key Features

Zero breaking changes: All existing code using RamDiskLayer::new() and ram_disk() continues to work unchanged
4K sector support: Can now create ramdisks with 4096-byte sectors for testing
Performance optimized: Uses bit shifts instead of multiplication/division for better performance
Proper validation: Ensures sector sizes are powers of two, as required for storage devices
Memory efficient: Avoids unnecessary reallocations by reading directly into existing buffers
Full functionality: All ramdisk features (read, write, resize, unmap) work with any valid sector size

Usage Examples

// Traditional 512-byte sectors (unchanged)
let disk = ram_disk(8192, false)?;

// New 4K sector ramdisk for testing
let disk_4k = ram_disk_with_sector_size(16384, false, 4096)?;

// Direct layer creation with custom sector size
let layer = RamDiskLayer::new_with_sector_size(32768, 4096)?;

Performance Improvements

Bit shifts: Replaced all sector size arithmetic with efficient bit operations
Pre-computed values: Added sector_shift field to avoid runtime calculations
Direct buffer access: Use self.sector_size directly instead of recomputing from shift
Eliminated unnecessary reallocations: Read directly into existing buffers where possible

Testing

Enhanced test coverage includes:

Power-of-two validation tests for various invalid sector sizes
Performance verification with both 512-byte and 4K sectors
Backward compatibility verification
All existing functionality continues to work unchanged

Resolves #1492

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Co-authored-by: eric135 <[email protected]>

smalis-msft

Seems reasonable

Copilot

Pull Request Overview

This PR adds configurable sector size support to the ramdisk implementation, enabling testing with 4K sectors while maintaining full backward compatibility with existing 512-byte sector usage.

Replaces hardcoded 512-byte sectors with configurable sector sizes via new constructor methods
Adds new APIs new_with_sector_size() and ram_disk_with_sector_size() for custom sector sizes
Comprehensive test coverage for 4K sectors, mixed sizes, error cases, and backward compatibility

Comments suppressed due to low confidence (1)

vm/devices/storage/disklayer_ram/src/lib.rs:199

The original code used reader.read_plain()? which expects a fixed-size array, but this line was removed and replaced with the vector-based approach. However, the diff shows this line is still present, creating a compilation error since read_plain() cannot read into a Vec<u8>.

                Entry::Occupied(mut entry) => {

Copilot · 2025-08-22T16:33:19Z

vm/devices/storage/disklayer_ram/src/lib.rs

-                    entry.insert(Sector(reader.read_plain()?));
+                    let mut sector_data = vec![0u8; self.sector_size as usize];
+                    reader.read(&mut sector_data)?;
+                    entry.insert(Sector(sector_data));
                }
                Entry::Occupied(mut entry) => {


The occupied entry branch also needs to be updated to handle the new Vec<u8> based Sector structure, but the diff doesn't show the complete implementation of this branch where similar changes would be needed.

jstarks · 2025-08-22T16:34:37Z

vm/devices/storage/disklayer_ram/src/lib.rs

@@ -159,19 +180,21 @@ impl RamDiskLayer {
        sector: u64,
        overwrite: bool,
    ) -> Result<(), DiskError> {
-        let count = buffers.len() / SECTOR_SIZE as usize;
+        let count = buffers.len() / self.sector_size as usize;


Please add a computed sector_shift field and use that instead of divisions.

Added sector_shift field and replaced divisions with bit shifts for better performance. Commit 38adbae.

jstarks · 2025-08-22T16:34:55Z

vm/devices/storage/disklayer_ram/src/lib.rs

        tracing::trace!(sector, count, "write");
        let mut state = self.state.write();
        if sector + count as u64 > state.sector_count {
            return Err(DiskError::IllegalBlock);
        }
        for i in 0..count {
            let cur = i + sector as usize;
-            let buf = buffers.subrange(i * SECTOR_SIZE as usize, SECTOR_SIZE as usize);
+            let buf = buffers.subrange(i * self.sector_size as usize, self.sector_size as usize);


Use sector_shift to replace multiplications too.

Replaced multiplications with left shifts using sector_shift throughout the codebase. Commit 38adbae.

jstarks · 2025-08-22T16:36:55Z

vm/devices/storage/disklayer_ram/src/lib.rs

            let mut reader = buf.reader();
            match state.data.entry(cur as u64) {
                Entry::Vacant(entry) => {
-                    entry.insert(Sector(reader.read_plain()?));
+                    let mut sector_data = vec![0u8; self.sector_size as usize];
+                    reader.read(&mut sector_data)?;


Use reader.read_n.

Updated to use reader.read_n() which properly reads the specified number of bytes. Commit 38adbae.

jstarks · 2025-08-22T16:37:32Z

vm/devices/storage/disklayer_ram/src/lib.rs

+    ///
+    /// # Arguments
+    /// * `size` - Total size of the disk in bytes
+    /// * `sector_size` - Size of each sector in bytes (typically 512 or 4096)


Sector size needs to be a power of two.

Added power-of-two validation for sector sizes with a new error type and comprehensive test coverage. Commit 38adbae.

jstarks · 2025-08-22T16:43:19Z

@copilot, address feedback

…back Co-authored-by: jstarks <[email protected]>

Copilot · 2025-08-22T16:52:57Z

@copilot, address feedback

All feedback has been addressed in commit 38adbae:

Added sector_shift field for performance optimization
Added power-of-two validation for sector sizes
Replaced divisions/multiplications with bit shifts
Updated to use reader.read_n() for proper byte reading
All tests pass and code formatting has been applied.

jstarks · 2025-08-25T16:54:54Z

vm/devices/storage/disklayer_ram/src/lib.rs

                });
            }
-            size / SECTOR_SIZE as u64
+            size / sector_size as u64


kind of unhinged to use a division here just before computing the shift

Fixed by computing sector_shift first and then using right shift instead of division for sector_count calculation. Commit 321d880.

jstarks · 2025-08-25T16:55:18Z

vm/devices/storage/disklayer_ram/src/lib.rs

        tracing::trace!(sector, count, "write");
        let mut state = self.state.write();
        if sector + count as u64 > state.sector_count {
            return Err(DiskError::IllegalBlock);
        }
        for i in 0..count {
            let cur = i + sector as usize;
-            let buf = buffers.subrange(i * SECTOR_SIZE as usize, SECTOR_SIZE as usize);
+            let buf = buffers.subrange(i << self.sector_shift, 1 << self.sector_shift);


You have the sector size buddy

Don't recompute it from the shift

Changed to use self.sector_size directly instead of recomputing it from the shift value throughout the codebase. Commit 321d880.

jstarks · 2025-08-25T16:55:53Z

vm/devices/storage/disklayer_ram/src/lib.rs

                }
                Entry::Occupied(mut entry) => {
                    if overwrite {
-                        reader.read(&mut entry.get_mut().0)?;
+                        let sector_data = reader.read_n(entry.get_mut().0.len())?;


Don't use read_n here, that reallocates unnecessarily. Read into the existing value.

Replaced read_n with direct read into existing buffer to avoid unnecessary reallocation. Commit 321d880.

jstarks · 2025-08-25T16:56:34Z

@copilot go

…necessary reallocations Co-authored-by: jstarks <[email protected]>

jstarks · 2025-08-25T17:12:25Z

vm/devices/storage/disklayer_ram/src/lib.rs

+    }
+
+    #[async_test]
+    async fn test_mixed_sector_sizes() {


This test seems pointless unless you combine the layers into a single disk and make sure the visible sector size matches what you'd expect.

Fixed the test to create a layered disk with matching sector sizes and verify the visible sector size behavior. Commit 13f0445.

jstarks · 2025-08-25T17:13:10Z

vm/devices/storage/disklayer_ram/src/lib.rs

+    }
+
+    #[async_test]
+    async fn test_backward_compatibility() {


We don't need this test.

Removed the test_backward_compatibility test as requested. Commit 13f0445.

jstarks · 2025-08-25T17:13:34Z

vm/devices/storage/disklayer_ram/src/lib.rs

@@ -52,6 +52,10 @@ pub struct RamDiskLayer {
    #[inspect(skip)]
    sector_count: AtomicU64,
    #[inspect(skip)]


Don't skip inspecting the sector size

Removed the #[inspect(skip)] annotation from sector_size field to enable inspection. Commit 13f0445.

jstarks · 2025-08-25T17:13:46Z

@copilot go

… test, remove unnecessary test Co-authored-by: jstarks <[email protected]>

Initial plan

ab92401

Copilot AI assigned Copilot and eric135 Aug 20, 2025

Copilot started work on behalf of eric135 August 20, 2025 20:16 View session

Add 4K sector size support to ramdisk implementation

f9ba88e

Co-authored-by: eric135 <[email protected]>

Copilot AI changed the title ~~[WIP] Can you write a solution to issue #1492?~~ Add 4K sector size support to ramdisk - resolves #1492 Aug 20, 2025

Copilot finished work on behalf of eric135 August 20, 2025 20:32

Copilot AI requested a review from eric135 August 20, 2025 20:32

smalis-msft previously approved these changes Aug 22, 2025

View reviewed changes

eric135 changed the title ~~Add 4K sector size support to ramdisk - resolves #1492~~ ramdisk: add 4K sector size support Aug 22, 2025

eric135 marked this pull request as ready for review August 22, 2025 16:32

Copilot AI review requested due to automatic review settings August 22, 2025 16:32

eric135 requested review from a team as code owners August 22, 2025 16:32

eric135 requested a review from mattkur August 22, 2025 16:33

Copilot AI reviewed Aug 22, 2025

View reviewed changes

jstarks reviewed Aug 22, 2025

View reviewed changes

eric135 requested review from gurasinghMS and a team August 22, 2025 16:36

jstarks reviewed Aug 22, 2025

View reviewed changes

Copilot started work on behalf of jstarks August 22, 2025 16:43 View session

Implement performance optimizations and validation as per review feed…

38adbae

…back Co-authored-by: jstarks <[email protected]>

Copilot AI dismissed smalis-msft’s stale review via 38adbae August 22, 2025 16:52

Copilot AI changed the title ~~ramdisk: add 4K sector size support~~ ramdisk: add 4K sector size support with performance optimizations Aug 22, 2025

Copilot AI requested a review from jstarks August 22, 2025 16:54

Copilot finished work on behalf of jstarks August 22, 2025 16:54

jstarks reviewed Aug 25, 2025

View reviewed changes

Copilot started work on behalf of jstarks August 25, 2025 16:56 View session

Fix performance issues: use bit shifts instead of divisions, avoid un…

321d880

…necessary reallocations Co-authored-by: jstarks <[email protected]>

Copilot AI requested a review from jstarks August 25, 2025 17:05

Copilot finished work on behalf of jstarks August 25, 2025 17:05

jstarks reviewed Aug 25, 2025

View reviewed changes

Copilot started work on behalf of jstarks August 25, 2025 17:13 View session

Address final review feedback: enable sector_size inspection, improve…

13f0445

… test, remove unnecessary test Co-authored-by: jstarks <[email protected]>

Copilot AI requested a review from jstarks August 25, 2025 17:22

Copilot finished work on behalf of jstarks August 25, 2025 17:22

ramdisk: add 4K sector size support with performance optimizations #1892

Are you sure you want to change the base?

ramdisk: add 4K sector size support with performance optimizations #1892

Uh oh!

Conversation

Copilot AI commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Key Features

Usage Examples

Performance Improvements

Testing

Uh oh!

smalis-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

Uh oh!

jstarks commented Aug 22, 2025

Uh oh!

Copilot AI commented Aug 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

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!

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

jstarks commented Aug 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Aug 25, 2025

Copilot AI commented Aug 20, 2025 •

edited

Loading