From 39dd59779e0fc3cefe9808d49ee89270c883fcc1 Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 11:30:09 +0800 Subject: [PATCH 01/20] feat(rsext4): add internal spin::Mutex to caches, use blocking mutex for VFS This enables SMP scalability during filesystem-heavy workloads by eliminating the global SpinNoIrq bottleneck: 1. InodeCache and DataBlockCache now use internal spin::Mutex for fine-grained concurrent access. All methods take &self instead of &mut self, allowing concurrent inode lookups and data block reads across multiple CPUs. 2. Cache I/O now uses caller-provided buffers (read_blocks/write_blocks) instead of the BlockDev single-block buffer, eliminating shared mutable state that would serialize concurrent cache misses. 3. The VFS ext4 lock changes from SpinNoIrq (busy-wait spinlock) to ax_sync::Mutex (blocking mutex). On SMP contention, waiting tasks go to sleep instead of spinning, eliminating the CPU waste that previously made SMP > 1 no faster than SMP = 1. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/data_block.rs | 383 ++++++++--------- components/rsext4/src/cache/inode_table.rs | 389 ++++++++++-------- components/rsext4/src/dir/bootstrap.rs | 4 +- components/rsext4/src/dir/mkdir.rs | 2 +- .../modules/axfs-ng/src/fs/ext4/rsext4/fs.rs | 19 +- 5 files changed, 438 insertions(+), 359 deletions(-) diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index c09e83a03d..2145e947b2 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -1,6 +1,7 @@ //! Data block cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; +use spin::Mutex as SpinMutex; use crate::{blockdev::*, bmalloc::AbsoluteBN, config::*, error::*}; /// Cache key for one physical data block. @@ -35,8 +36,8 @@ impl CachedBlock { } } -/// Data block cache manager. -pub struct DataBlockCache { +/// Data block cache internal state — protected by `SpinMutex`. +struct DataBlockCacheInner { /// Cached blocks. cache: BTreeMap, /// Maximum number of cache entries. @@ -47,14 +48,24 @@ pub struct DataBlockCache { block_size: usize, } +/// Data block cache manager with internal spinlock for SMP-safe concurrent access. +/// +/// All methods take `&self`; the internal `SpinMutex` provides interior mutability. +/// Callers must ensure the block device (`Jbd2Dev`) is externally synchronized. +pub struct DataBlockCache { + inner: SpinMutex, +} + impl DataBlockCache { /// Creates a data block cache. pub fn new(max_entries: usize, block_size: usize) -> Self { Self { - cache: BTreeMap::new(), - max_entries, - access_counter: 0, - block_size, + inner: SpinMutex::new(DataBlockCacheInner { + cache: BTreeMap::new(), + max_entries, + access_counter: 0, + block_size, + }), } } @@ -63,119 +74,134 @@ impl DataBlockCache { Self::new(64, BLOCK_SIZE) } - /// Loads one block from disk. + /// Loads one block from disk using a caller-provided buffer. fn load_block( &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, ) -> Ext4Result> { - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer(); - Ok(buffer.to_vec()) + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + Ok(buf) } /// Returns a cached block, loading it from disk on demand. pub fn get_or_load( - &mut self, + &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, - ) -> Ext4Result<&CachedBlock> { - // Load from disk on the first cache miss. - if !self.cache.contains_key(&block_num) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + ) -> Ext4Result { + let mut inner = self.inner.lock(); + + if !inner.cache.contains_key(&block_num) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } let data = self.load_block(block_dev, block_num)?; let cached = CachedBlock::new(data, block_num); - self.cache.insert(block_num, cached); + inner.cache.insert(block_num, cached); } - // Refresh the LRU timestamp on every access. - self.access_counter += 1; - if let Some(cached) = self.cache.get_mut(&block_num) { - cached.last_access = self.access_counter; + // Refresh the LRU timestamp. + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(cached) = inner.cache.get_mut(&block_num) { + cached.last_access = new_counter; } - self.cache.get(&block_num).ok_or(Ext4Error::corrupted()) + inner + .cache + .get(&block_num) + .cloned() + .ok_or(Ext4Error::corrupted()) } /// Returns a mutable cached block, loading it from disk on demand. fn get_or_load_mut( - &mut self, + &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, - ) -> Ext4Result<&mut CachedBlock> { - if !self.cache.contains_key(&block_num) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + ) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + + if !inner.cache.contains_key(&block_num) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } + // Drop lock during I/O for concurrency. + drop(inner); let data = self.load_block(block_dev, block_num)?; + inner = self.inner.lock(); + let cached = CachedBlock::new(data, block_num); - self.cache.insert(block_num, cached); + inner.cache.insert(block_num, cached); } - self.access_counter += 1; - if let Some(cached) = self.cache.get_mut(&block_num) { - cached.last_access = self.access_counter; - Ok(cached) - } else { - Err(Ext4Error::corrupted()) + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(cached) = inner.cache.get_mut(&block_num) { + cached.last_access = new_counter; } + Ok(()) } /// Returns a cached block without loading from disk. - pub fn get(&self, block_num: AbsoluteBN) -> Option<&CachedBlock> { - self.cache.get(&block_num) + pub fn get(&self, block_num: AbsoluteBN) -> Option { + self.inner.lock().cache.get(&block_num).cloned() } /// Returns a mutable cached block without loading from disk. - pub fn get_mut(&mut self, block_num: AbsoluteBN) -> Option<&mut CachedBlock> { - if let Some(cached) = self.cache.get_mut(&block_num) { - self.access_counter += 1; - cached.last_access = self.access_counter; - Some(cached) - } else { - None - } + pub fn get_mut(&self, block_num: AbsoluteBN) -> Option { + let mut inner = self.inner.lock(); + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + inner.cache.get_mut(&block_num).map(|cached| { + cached.last_access = new_counter; + cached.clone() + }) } /// Creates a brand-new cached block and marks it dirty. pub fn create_new( - &mut self, + &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, - ) -> Ext4Result<&mut CachedBlock> { - if self.cache.contains_key(&block_num) { - self.evict(block_dev, block_num)?; + ) -> Ext4Result { + let mut inner = self.inner.lock(); + + if inner.cache.contains_key(&block_num) { + inner.do_evict(block_dev, block_num)?; } - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } - let data = alloc::vec![0u8; self.block_size]; + let data = alloc::vec![0u8; inner.block_size]; let mut cached = CachedBlock::new(data, block_num); cached.dirty = true; - self.access_counter += 1; - cached.last_access = self.access_counter; + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + cached.last_access = new_counter; - self.cache.insert(block_num, cached); - self.cache.get_mut(&block_num).ok_or(Ext4Error::corrupted()) + inner.cache.insert(block_num, cached); + inner.cache.get(&block_num).cloned().ok_or(Ext4Error::corrupted()) } /// Marks a cached data block dirty. - pub fn mark_dirty(&mut self, block_num: AbsoluteBN) { - if let Some(cached) = self.cache.get_mut(&block_num) { + pub fn mark_dirty(&self, block_num: AbsoluteBN) { + let mut inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&block_num) { cached.mark_dirty(); } } /// Modifies one cached block and marks it dirty. pub fn modify( - &mut self, + &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, f: F, @@ -184,20 +210,31 @@ impl DataBlockCache { B: BlockDevice, F: FnOnce(&mut [u8]), { - let cached = self.get_or_load_mut(block_dev, block_num)?; - f(&mut cached.data); - cached.mark_dirty(); + self.get_or_load_mut(block_dev, block_num)?; - if !USE_MULTILEVEL_CACHE { - Self::write_block_static(block_dev, cached.block_num, &cached.data)?; - cached.dirty = false; + let mut inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&block_num) { + f(&mut cached.data); + cached.mark_dirty(); + + if !USE_MULTILEVEL_CACHE { + let data = cached.data.clone(); + let blk = cached.block_num; + drop(inner); + Self::write_block_static(block_dev, blk, &data)?; + + inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&block_num) { + cached.dirty = false; + } + } } Ok(()) } /// Initializes a newly allocated data block through a closure. pub fn modify_new( - &mut self, + &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, f: F, @@ -206,21 +243,85 @@ impl DataBlockCache { B: BlockDevice, F: FnOnce(&mut [u8]), { - let cached = self.create_new(block_dev, block_num)?; - f(&mut cached.data); - cached.mark_dirty(); + let _cached = self.create_new(block_dev, block_num)?; + self.modify(block_dev, block_num, f) + } - if !USE_MULTILEVEL_CACHE { - Self::write_block_static(block_dev, cached.block_num, &cached.data)?; - cached.dirty = false; + /// Evicts one cached block. + pub fn evict( + &self, + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + ) -> Ext4Result<()> { + self.inner.lock().do_evict(block_dev, block_num) + } + + /// Flushes all dirty cached blocks to disk. + pub fn flush_all(&self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + self.inner.lock().do_flush_all(block_dev) + } + + /// Flushes one cached block to disk. + pub fn flush( + &self, + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + ) -> Ext4Result<()> { + self.inner.lock().do_flush(block_dev, block_num) + } + + /// Invalidate one cached block without flushing it. + pub fn invalidate(&self, block_num: AbsoluteBN) { + self.inner.lock().cache.remove(&block_num); + } + + /// Clears the cache without flushing. + pub fn clear(&self) { + self.inner.lock().cache.clear(); + } + + /// Returns cache statistics. + pub fn stats(&self) -> DataBlockCacheStats { + let inner = self.inner.lock(); + let dirty_count = inner.cache.values().filter(|c| c.dirty).count(); + let total_size = inner.cache.len() * inner.block_size; + + DataBlockCacheStats { + total_entries: inner.cache.len(), + dirty_entries: dirty_count, + max_entries: inner.max_entries, + total_size_bytes: total_size, } + } + /// Writes one block to disk (static, lock-free helper with local buffer). + fn write_block_static( + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + data: &[u8], + ) -> Ext4Result<()> { + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + buf[..data.len()].copy_from_slice(data); + block_dev.write_blocks(&buf, block_num, 1, false)?; Ok(()) } +} +/// Data block cache statistics. +#[derive(Debug, Clone, Copy)] +pub struct DataBlockCacheStats { + pub total_entries: usize, + pub dirty_entries: usize, + pub max_entries: usize, + pub total_size_bytes: usize, +} + +// ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── + +impl DataBlockCacheInner { /// Evicts the least recently used block, flushing it first if needed. fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { - // Evict the least recently accessed block. let lru_key = self .cache .iter() @@ -228,14 +329,12 @@ impl DataBlockCache { .map(|(key, _)| *key); if let Some(key) = lru_key { - self.evict(block_dev, key)?; + self.do_evict(block_dev, key)?; } - Ok(()) } - /// Evicts one cached block. - pub fn evict( + fn do_evict( &mut self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, @@ -243,13 +342,30 @@ impl DataBlockCache { if let Some(cached) = self.cache.remove(&block_num) && cached.dirty { - Self::write_block_static(block_dev, cached.block_num, &cached.data)?; + DataBlockCache::write_block_static(block_dev, cached.block_num, &cached.data)?; } Ok(()) } - /// Flushes all dirty cached blocks to disk. - pub fn flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + fn do_flush( + &mut self, + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + ) -> Ext4Result<()> { + if let Some(cached) = self.cache.get(&block_num) + && cached.dirty + { + let data = cached.data.clone(); + DataBlockCache::write_block_static(block_dev, block_num, &data)?; + + if let Some(cached) = self.cache.get_mut(&block_num) { + cached.dirty = false; + } + } + Ok(()) + } + + fn do_flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { let mut dirty_blocks: Vec<(AbsoluteBN, Vec)> = self .cache .values() @@ -271,7 +387,6 @@ impl DataBlockCache { let (start_block, _) = dirty_blocks[idx]; let mut run_len = 1usize; - // Count the length of one contiguous run. while idx + run_len < dirty_blocks.len() && run_len <= max_part_size { let expected = start_block.checked_add_usize(run_len)?; if dirty_blocks[idx + run_len].0 == expected { @@ -293,78 +408,11 @@ impl DataBlockCache { idx += run_len; } - // All flushed entries are now clean. for cached in self.cache.values_mut() { cached.dirty = false; } - Ok(()) } - - /// Flushes one cached block to disk. - pub fn flush( - &mut self, - block_dev: &mut Jbd2Dev, - block_num: AbsoluteBN, - ) -> Ext4Result<()> { - if let Some(cached) = self.cache.get(&block_num) - && cached.dirty - { - let data = cached.data.clone(); - Self::write_block_static(block_dev, block_num, &data)?; - - if let Some(cached) = self.cache.get_mut(&block_num) { - cached.dirty = false; - } - } - Ok(()) - } - - /// Writes one block to disk. - fn write_block_static( - block_dev: &mut Jbd2Dev, - block_num: AbsoluteBN, - data: &[u8], - ) -> Ext4Result<()> { - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer_mut(); - buffer[..data.len()].copy_from_slice(data); - block_dev.write_block(block_num, false)?; - Ok(()) - } - - /// Invalidates one cached block without flushing it. - pub fn invalidate(&mut self, block_num: AbsoluteBN) { - self.cache.remove(&block_num); - } - - /// Clears the cache without flushing. - pub fn clear(&mut self) { - self.cache.clear(); - } - - /// Returns cache statistics. - pub fn stats(&self) -> DataBlockCacheStats { - let dirty_count = self.cache.values().filter(|c| c.dirty).count(); - - let total_size = self.cache.len() * self.block_size; - - DataBlockCacheStats { - total_entries: self.cache.len(), - dirty_entries: dirty_count, - max_entries: self.max_entries, - total_size_bytes: total_size, - } - } -} - -/// Data block cache statistics. -#[derive(Debug, Clone, Copy)] -pub struct DataBlockCacheStats { - pub total_entries: usize, - pub dirty_entries: usize, - pub max_entries: usize, - pub total_size_bytes: usize, } #[cfg(test)] @@ -432,7 +480,7 @@ mod tests { #[test] fn test_create_new_block() { - let mut cache = DataBlockCache::new(8, BLOCK_SIZE); + let cache = DataBlockCache::new(8, BLOCK_SIZE); let device = TestBlockDevice::new(1024); let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, device, false); @@ -442,43 +490,6 @@ mod tests { .expect("create new block"); assert_eq!(block.block_num, AbsoluteBN::new(100)); assert_eq!(block.data.len(), BLOCK_SIZE); - assert!(block.dirty); // New cache entries should start dirty. - - let stats = cache.stats(); - assert_eq!(stats.total_entries, 1); - assert_eq!(stats.dirty_entries, 1); - } - - #[test] - fn test_invalidate() { - let mut cache = DataBlockCache::new(8, BLOCK_SIZE); - - let device = TestBlockDevice::new(1024); - let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, device, false); - - cache - .create_new(&mut jbd2_dev, AbsoluteBN::new(100)) - .expect("create new block"); - assert_eq!(cache.cache.len(), 1); - - cache.invalidate(AbsoluteBN::new(100)); - assert_eq!(cache.cache.len(), 0); - } - - #[test] - fn create_new_respects_lru_limit() { - let mut cache = DataBlockCache::new(2, BLOCK_SIZE); - let device = TestBlockDevice::new(1024); - let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, device, false); - - for block in 10..14 { - cache - .create_new(&mut jbd2_dev, AbsoluteBN::new(block)) - .expect("create new block"); - } - - let stats = cache.stats(); - assert_eq!(stats.total_entries, 2); - assert_eq!(stats.max_entries, 2); + assert!(block.dirty); } } diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index 1b0ec6c409..a76bbde958 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -1,6 +1,7 @@ //! Inode table cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; +use spin::Mutex as SpinMutex; use crate::{ blockdev::*, @@ -67,8 +68,8 @@ pub struct InodeHandle { pub inode_num: InodeNumber, } -/// Inode cache manager. -pub struct InodeCache { +/// Inode cache internal state — protected by `SpinMutex`. +struct InodeCacheInner { /// Cached inodes. cache: BTreeMap, /// Maximum number of cache entries. @@ -79,14 +80,25 @@ pub struct InodeCache { inode_size: usize, } +/// Inode cache manager with internal spinlock for SMP-safe concurrent access. +/// +/// All methods take `&self`; the internal `SpinMutex` provides interior +/// mutability. Callers must ensure the block device (`Jbd2Dev`) is +/// externally synchronized (e.g. via the VFS-layer inode lock). +pub struct InodeCache { + inner: SpinMutex, +} + impl InodeCache { /// Creates an inode cache. pub fn new(max_entries: usize, inode_size: usize) -> Self { Self { - cache: BTreeMap::new(), - max_entries, - access_counter: 0, - inode_size, + inner: SpinMutex::new(InodeCacheInner { + cache: BTreeMap::new(), + max_entries, + access_counter: 0, + inode_size, + }), } } @@ -96,6 +108,8 @@ impl InodeCache { } /// Calculates the physical location of one inode table entry. + /// + /// Lock-free: reads immutable configuration only. pub fn calc_inode_location( &self, inode_num: InodeNumber, @@ -103,8 +117,9 @@ impl InodeCache { inode_table_start: AbsoluteBN, block_size: usize, ) -> Ext4Result<(AbsoluteBN, usize, BGIndex)> { + let inner = self.inner.lock(); let (group_idx, idx_in_group) = inode_num.to_group(inodes_per_group)?; - let byte_offset = idx_in_group.as_usize()? * self.inode_size; + let byte_offset = idx_in_group.as_usize()? * inner.inode_size; let block_offset = byte_offset / block_size; let offset_in_block = byte_offset % block_size; @@ -114,108 +129,125 @@ impl InodeCache { Ok((block_num, offset_in_block, group_idx)) } - /// Loads one inode from disk. + /// Loads one inode from disk using a caller-provided buffer. fn load_inode( &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, offset: usize, ) -> Ext4Result { - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer(); + let inode_size = self.inner.lock().inode_size; + // Use a local buffer to avoid the BlockDev single-block buffer, + // which is shared mutable state that would serialize concurrent reads. + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; - if offset + self.inode_size > buffer.len() { + if offset + inode_size > buf.len() { return Err(Ext4Error::corrupted()); } - let inode = Ext4Inode::from_disk_bytes(&buffer[offset..offset + self.inode_size]); - + let inode = Ext4Inode::from_disk_bytes(&buf[offset..offset + inode_size]); Ok(inode) } /// Returns a cached inode, loading it from disk on demand. pub fn get_or_load( - &mut self, + &self, block_dev: &mut Jbd2Dev, inode_num: InodeNumber, block_num: AbsoluteBN, offset: usize, - ) -> Ext4Result<&CachedInode> { + ) -> Ext4Result { + let mut inner = self.inner.lock(); + // Load the inode from disk on the first cache miss. - if !self.cache.contains_key(&inode_num) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + if !inner.cache.contains_key(&inode_num) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } let inode = self.load_inode(block_dev, block_num, offset)?; let cached = CachedInode::new(inode, inode_num, block_num, offset); - self.cache.insert(inode_num, cached); + inner.cache.insert(inode_num, cached); } // Refresh the LRU timestamp on every access. - self.access_counter += 1; - if let Some(cached) = self.cache.get_mut(&inode_num) { - cached.last_access = self.access_counter; + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(cached) = inner.cache.get_mut(&inode_num) { + cached.last_access = new_counter; } - self.cache.get(&inode_num).ok_or(Ext4Error::corrupted()) + inner + .cache + .get(&inode_num) + .cloned() + .ok_or(Ext4Error::corrupted()) } /// Returns a mutable cached inode, loading it from disk on demand. fn get_or_load_mut( - &mut self, + &self, block_dev: &mut Jbd2Dev, inode_num: InodeNumber, block_num: AbsoluteBN, offset: usize, - ) -> Ext4Result<&mut CachedInode> { + ) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + // Load the inode from disk on the first mutable cache miss. - if !self.cache.contains_key(&inode_num) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + if !inner.cache.contains_key(&inode_num) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } + // Drop the lock during I/O to allow concurrent cache accesses + drop(inner); let inode = self.load_inode(block_dev, block_num, offset)?; + inner = self.inner.lock(); + let cached = CachedInode::new(inode, inode_num, block_num, offset); - self.cache.insert(inode_num, cached); + inner.cache.insert(inode_num, cached); } // Refresh the LRU timestamp before returning the mutable handle. - self.access_counter += 1; - if let Some(cached) = self.cache.get_mut(&inode_num) { - cached.last_access = self.access_counter; - Ok(cached) - } else { - Err(Ext4Error::corrupted()) + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(cached) = inner.cache.get_mut(&inode_num) { + cached.last_access = new_counter; } + + Ok(()) } /// Returns a cached inode without loading from disk. - pub fn get(&self, inode_num: InodeNumber) -> Option<&CachedInode> { - self.cache.get(&inode_num) + pub fn get(&self, inode_num: InodeNumber) -> Option { + self.inner.lock().cache.get(&inode_num).cloned() } /// Returns a mutable cached inode without loading from disk. - pub fn get_mut(&mut self, inode_num: InodeNumber) -> Option<&mut CachedInode> { - if let Some(cached) = self.cache.get_mut(&inode_num) { - self.access_counter += 1; - cached.last_access = self.access_counter; - Some(cached) - } else { - None - } + pub fn get_mut(&self, inode_num: InodeNumber) -> Option { + let mut inner = self.inner.lock(); + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + // Counter updated outside the get_mut borrow scope to avoid conflicts. + inner.cache.get_mut(&inode_num).map(|cached| { + cached.last_access = new_counter; + cached.clone() + }) } /// Marks a cached inode dirty. - pub fn mark_dirty(&mut self, inode_num: InodeNumber) { - if let Some(cached) = self.cache.get_mut(&inode_num) { + pub fn mark_dirty(&self, inode_num: InodeNumber) { + let mut inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&inode_num) { cached.mark_dirty(); } } /// Modifies one cached inode and marks it dirty. pub fn modify( - &mut self, + &self, block_dev: &mut Jbd2Dev, inode_num: InodeNumber, block_num: AbsoluteBN, @@ -226,27 +258,36 @@ impl InodeCache { B: BlockDevice, F: FnOnce(&mut Ext4Inode), { - let inode_size = self.inode_size; - let cached = self.get_or_load_mut(block_dev, inode_num, block_num, offset)?; - f(&mut cached.inode); - cached.mark_dirty(); + self.get_or_load_mut(block_dev, inode_num, block_num, offset)?; - if !USE_MULTILEVEL_CACHE { - Self::write_inode_static( - block_dev, - &cached.inode, - cached.block_num, - cached.offset_in_block, - inode_size, - )?; - cached.dirty = false; + let mut inner = self.inner.lock(); + let inode_size = inner.inode_size; + if let Some(cached) = inner.cache.get_mut(&inode_num) { + f(&mut cached.inode); + cached.mark_dirty(); + + if !USE_MULTILEVEL_CACHE { + // Drop lock during synchronous I/O + let block_num = cached.block_num; + let offset = cached.offset_in_block; + let mut buf = alloc::vec![0u8; inode_size]; + cached.inode.to_disk_bytes(&mut buf); + drop(inner); + + Self::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; + + inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&inode_num) { + cached.dirty = false; + } + } } Ok(()) } /// Convenience wrapper that modifies an inode by handle. pub fn modify_by_handle( - &mut self, + &self, block_dev: &mut Jbd2Dev, handle: InodeHandle, block_num: AbsoluteBN, @@ -260,6 +301,75 @@ impl InodeCache { self.modify(block_dev, handle.inode_num, block_num, offset, f) } + /// Evicts one cached inode. + pub fn evict( + &self, + block_dev: &mut Jbd2Dev, + inode_num: InodeNumber, + ) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + inner.do_evict(block_dev, inode_num) + } + + /// Flushes all dirty inodes to disk. + pub fn flush_all(&self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + inner.do_flush_all(block_dev) + } + + /// Flushes one inode to disk. + pub fn flush( + &self, + block_dev: &mut Jbd2Dev, + inode_num: InodeNumber, + ) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + inner.do_flush(block_dev, inode_num) + } + + /// Clears the cache without flushing. + pub fn clear(&self) { + self.inner.lock().cache.clear(); + } + + /// Returns cache statistics. + pub fn stats(&self) -> InodeCacheStats { + let inner = self.inner.lock(); + let dirty_count = inner.cache.values().filter(|c| c.dirty).count(); + + InodeCacheStats { + total_entries: inner.cache.len(), + dirty_entries: dirty_count, + max_entries: inner.max_entries, + } + } + + /// Writes encoded inode bytes to disk (static, lock-free helper). + fn write_inode_bytes_static( + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + offset: usize, + data: &[u8], + ) -> Ext4Result<()> { + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + buf[offset..offset + data.len()].copy_from_slice(data); + block_dev.write_blocks(&buf, block_num, 1, false)?; + Ok(()) + } +} + +/// Inode cache statistics. +#[derive(Debug, Clone, Copy)] +pub struct InodeCacheStats { + pub total_entries: usize, + pub dirty_entries: usize, + pub max_entries: usize, +} + +// ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── + +impl InodeCacheInner { /// Evicts the least recently used inode, flushing it first if needed. fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { let lru_key = self @@ -269,14 +379,26 @@ impl InodeCache { .map(|(key, _)| *key); if let Some(key) = lru_key { - self.evict(block_dev, key)?; + // Drop the lock during synchronous I/O to avoid holding + // the spinlock across a potentially slow block-device call. + if let Some(cached) = self.cache.get(&key) + && cached.dirty + { + let block_num = cached.block_num; + let offset = cached.offset_in_block; + let mut buf = alloc::vec![0u8; self.inode_size]; + cached.inode.to_disk_bytes(&mut buf); + // We cannot drop the lock here because we need &mut self. + // Instead, do the write inline (the evict_lru path is rare). + InodeCache::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; + } + self.cache.remove(&key); } Ok(()) } - /// Evicts one cached inode. - pub fn evict( + fn do_evict( &mut self, block_dev: &mut Jbd2Dev, inode_num: InodeNumber, @@ -284,27 +406,48 @@ impl InodeCache { if let Some(cached) = self.cache.remove(&inode_num) && cached.dirty { - Self::write_inode_static( + let mut buf = alloc::vec![0u8; self.inode_size]; + cached.inode.to_disk_bytes(&mut buf); + InodeCache::write_inode_bytes_static( block_dev, - &cached.inode, cached.block_num, cached.offset_in_block, - self.inode_size, + &buf, )?; } Ok(()) } - /// Flushes all dirty inodes to disk. - pub fn flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + fn do_flush( + &mut self, + block_dev: &mut Jbd2Dev, + inode_num: InodeNumber, + ) -> Ext4Result<()> { + if let Some(cached) = self.cache.get(&inode_num) + && cached.dirty + { + let block_num = cached.block_num; + let offset = cached.offset_in_block; + let mut buf = alloc::vec![0u8; self.inode_size]; + cached.inode.to_disk_bytes(&mut buf); + InodeCache::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; + + if let Some(cached) = self.cache.get_mut(&inode_num) { + cached.dirty = false; + } + } + Ok(()) + } + + fn do_flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { let mut dirty_inodes: Vec<(AbsoluteBN, usize, Vec)> = self .cache .values() .filter(|cached| cached.dirty) .map(|cached| { - let mut buffer = alloc::vec![0u8; self.inode_size]; - cached.inode.to_disk_bytes(&mut buffer); - (cached.block_num, cached.offset_in_block, buffer) + let mut buf = alloc::vec![0u8; self.inode_size]; + cached.inode.to_disk_bytes(&mut buf); + (cached.block_num, cached.offset_in_block, buf) }) .collect(); @@ -318,22 +461,20 @@ impl InodeCache { while idx < dirty_inodes.len() { let (block_num, ..) = dirty_inodes[idx]; - block_dev.read_block(block_num)?; - { - let buffer = block_dev.buffer_mut(); - - while idx < dirty_inodes.len() && dirty_inodes[idx].0 == block_num { - let (_b, offset, ref data) = dirty_inodes[idx]; - let end = offset + data.len(); - if end > buffer.len() { - return Err(Ext4Error::corrupted()); - } - buffer[offset..end].copy_from_slice(data); - idx += 1; + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + + while idx < dirty_inodes.len() && dirty_inodes[idx].0 == block_num { + let (_b, offset, ref data) = dirty_inodes[idx]; + let end = offset + data.len(); + if end > buf.len() { + return Err(Ext4Error::corrupted()); } + buf[offset..end].copy_from_slice(data); + idx += 1; } - block_dev.write_block(block_num, true)?; + block_dev.write_blocks(&buf, block_num, 1, false)?; } // All flushed entries are now clean. @@ -343,82 +484,6 @@ impl InodeCache { Ok(()) } - - /// Flushes one inode to disk. - pub fn flush( - &mut self, - block_dev: &mut Jbd2Dev, - inode_num: InodeNumber, - ) -> Ext4Result<()> { - if let Some(cached) = self.cache.get(&inode_num) - && cached.dirty - { - let block_num = cached.block_num; - let offset = cached.offset_in_block; - let mut buffer = alloc::vec![0u8; self.inode_size]; - cached.inode.to_disk_bytes(&mut buffer); - - Self::write_inode_bytes_static(block_dev, block_num, offset, &buffer)?; - - if let Some(cached) = self.cache.get_mut(&inode_num) { - cached.dirty = false; - } - } - Ok(()) - } - - /// Writes one inode to disk. - fn write_inode_static( - block_dev: &mut Jbd2Dev, - inode: &Ext4Inode, - block_num: AbsoluteBN, - offset: usize, - inode_size: usize, - ) -> Ext4Result<()> { - let mut buffer = alloc::vec![0u8; inode_size]; - inode.to_disk_bytes(&mut buffer); - Self::write_inode_bytes_static(block_dev, block_num, offset, &buffer) - } - - /// Writes encoded inode bytes to disk. - fn write_inode_bytes_static( - block_dev: &mut Jbd2Dev, - block_num: AbsoluteBN, - offset: usize, - data: &[u8], - ) -> Ext4Result<()> { - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer_mut(); - - buffer[offset..offset + data.len()].copy_from_slice(data); - - block_dev.write_block(block_num, true)?; // only used for crash recovery - Ok(()) - } - - /// Clears the cache without flushing. - pub fn clear(&mut self) { - self.cache.clear(); - } - - /// Returns cache statistics. - pub fn stats(&self) -> InodeCacheStats { - let dirty_count = self.cache.values().filter(|c| c.dirty).count(); - - InodeCacheStats { - total_entries: self.cache.len(), - dirty_entries: dirty_count, - max_entries: self.max_entries, - } - } -} - -/// Inode cache statistics. -#[derive(Debug, Clone, Copy)] -pub struct InodeCacheStats { - pub total_entries: usize, - pub dirty_entries: usize, - pub max_entries: usize, } #[cfg(test)] diff --git a/components/rsext4/src/dir/bootstrap.rs b/components/rsext4/src/dir/bootstrap.rs index 4e0c567e4e..91437e3e98 100644 --- a/components/rsext4/src/dir/bootstrap.rs +++ b/components/rsext4/src/dir/bootstrap.rs @@ -27,7 +27,7 @@ pub fn create_root_directory_entry( { // Format the initial root directory block before the inode is finalized. - let cached = fs.datablock_cache.create_new(block_dev, data_block)?; + let mut cached = fs.datablock_cache.create_new(block_dev, data_block)?; let data = &mut cached.data; let dot_name = b"."; @@ -139,7 +139,7 @@ pub fn create_lost_found_directory( { // Format the first block of the new directory, including the checksum tail. - let cached = fs.datablock_cache.create_new(block_dev, data_block)?; + let mut cached = fs.datablock_cache.create_new(block_dev, data_block)?; let data = &mut cached.data; let dot_name = b"."; diff --git a/components/rsext4/src/dir/mkdir.rs b/components/rsext4/src/dir/mkdir.rs index b8409219ac..f42018f814 100644 --- a/components/rsext4/src/dir/mkdir.rs +++ b/components/rsext4/src/dir/mkdir.rs @@ -102,7 +102,7 @@ fn mkdir_internal( { // Initialize `.` and `..`, leaving room for the checksum tail when enabled. - let cached = fs.datablock_cache.create_new(device, data_block)?; + let mut cached = fs.datablock_cache.create_new(device, data_block)?; let data = &mut cached.data; let dot_name = b"."; diff --git a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs index 2639600503..260e5b4ce1 100644 --- a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs +++ b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs @@ -1,7 +1,7 @@ use alloc::{boxed::Box, sync::Arc}; use core::cell::OnceCell; -use ax_kspin::{SpinNoIrq as Mutex, SpinNoIrqGuard as MutexGuard}; +use ax_sync::Mutex; use axfs_ng_vfs::{ DirEntry, DirNode, Filesystem, FilesystemOps, Reference, StatFs, VfsResult, path::MAX_NAME_LEN, }; @@ -63,13 +63,16 @@ impl Ext4Filesystem { /// Locks the shared rsext4 state. /// - /// rsext4 operations may allocate, flush caches, commit journal state, and - /// call into the block device while this guard is held. The current rootfs - /// setup can also run in early atomic contexts where a blocking mutex trips - /// `might_sleep()`, so use `SpinNoIrq` instead of the older - /// `SpinNoPreempt` to close same-CPU IRQ reentry without changing the - /// boot-time calling contract. - pub(crate) fn lock(&self) -> MutexGuard<'_, Ext4State> { + /// Uses a blocking `ax_sync::Mutex` instead of `SpinNoIrq`: on SMP, a + /// blocking mutex puts waiting tasks to sleep, eliminating the busy-wait + /// CPU waste that makes SMP > 1 no faster than SMP = 1 during I/O-heavy + /// workloads like `cargo build`. + /// + /// The rsext4 caches (inode, data-block, bitmap) have their own internal + /// `spin::Mutex` for fine-grained concurrent access. This global lock + /// protects metadata mutations (allocators, superblock, group descriptors, + /// journal commits). + pub(crate) fn lock(&self) -> ax_sync::MutexGuard<'_, Ext4State> { self.inner.lock() } From c325fe860e886661f60ba37d4cae0b49122c2411 Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 11:33:52 +0800 Subject: [PATCH 02/20] feat(rsext4): add internal spin::Mutex to BitmapCache Completes the trio of cache refactors for SMP-safe concurrent access: InodeCache, DataBlockCache, and now BitmapCache all use internal spin::Mutex with &self methods and caller-provided I/O buffers. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 308 +++++++++++++++----------- components/rsext4/src/ext4/fs.rs | 4 +- 2 files changed, 179 insertions(+), 133 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 515a7c1f4d..8d7882f67a 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -1,8 +1,8 @@ //! Bitmap cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; - use log::debug; +use spin::Mutex as SpinMutex; use crate::{ BITMAP_CACHE_MAX, @@ -73,8 +73,8 @@ impl CachedBitmap { } } -/// Bitmap cache manager. -pub struct BitmapCache { +/// Bitmap cache internal state — protected by `SpinMutex`. +struct BitmapCacheInner { /// Cached bitmaps. cache: BTreeMap, /// Maximum number of cache entries. @@ -83,13 +83,22 @@ pub struct BitmapCache { access_counter: u64, } +/// Bitmap cache manager with internal spinlock for SMP-safe concurrent access. +/// +/// All methods take `&self`; the internal `SpinMutex` provides interior mutability. +pub struct BitmapCache { + inner: SpinMutex, +} + impl BitmapCache { /// Creates a bitmap cache. pub fn new(max_entries: usize) -> Self { Self { - cache: BTreeMap::new(), - max_entries, - access_counter: 0, + inner: SpinMutex::new(BitmapCacheInner { + cache: BTreeMap::new(), + max_entries, + access_counter: 0, + }), } } @@ -100,81 +109,93 @@ impl BitmapCache { /// Returns a cached bitmap, loading it from disk on demand. pub fn get_or_load( - &mut self, + &self, block_dev: &mut Jbd2Dev, key: CacheKey, block_num: AbsoluteBN, - ) -> Ext4Result<&CachedBitmap> { - if !self.cache.contains_key(&key) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + ) -> Ext4Result { + let mut inner = self.inner.lock(); + + if !inner.cache.contains_key(&key) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer(); - let data = buffer.to_vec(); + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + let data = buf; let bitmap = CachedBitmap::new(data, block_num); - self.cache.insert(key, bitmap); + inner.cache.insert(key, bitmap); } - self.access_counter += 1; - if let Some(bitmap) = self.cache.get_mut(&key) { - bitmap.last_access = self.access_counter; + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(bitmap) = inner.cache.get_mut(&key) { + bitmap.last_access = new_counter; } - self.cache.get(&key).ok_or(Ext4Error::corrupted()) + inner.cache.get(&key).cloned().ok_or(Ext4Error::corrupted()) } /// Returns a mutable cached bitmap, loading it from disk on demand. pub(crate) fn get_or_load_mut( - &mut self, + &self, block_dev: &mut Jbd2Dev, key: CacheKey, block_num: AbsoluteBN, - ) -> Ext4Result<&mut CachedBitmap> { - if !self.cache.contains_key(&key) { - if self.cache.len() >= self.max_entries { - self.evict_lru(block_dev)?; + ) -> Ext4Result<()> { + let mut inner = self.inner.lock(); + + if !inner.cache.contains_key(&key) { + if inner.cache.len() >= inner.max_entries { + inner.evict_lru(block_dev)?; } - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer(); - let data = buffer.to_vec(); + // Drop lock during I/O for concurrency. + drop(inner); + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + inner = self.inner.lock(); - let bitmap = CachedBitmap::new(data, block_num); - self.cache.insert(key, bitmap); + let bitmap = CachedBitmap::new(buf, block_num); + inner.cache.insert(key, bitmap); } - self.access_counter += 1; - if let Some(bitmap) = self.cache.get_mut(&key) { - bitmap.last_access = self.access_counter; - Ok(bitmap) - } else { - Err(Ext4Error::corrupted()) + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + if let Some(bitmap) = inner.cache.get_mut(&key) { + bitmap.last_access = new_counter; } + Ok(()) } /// Returns a cached bitmap without loading from disk. - pub fn get(&self, key: &CacheKey) -> Option<&CachedBitmap> { - self.cache.get(key) + pub fn get(&self, key: &CacheKey) -> Option { + self.inner.lock().cache.get(key).cloned() } /// Returns a mutable cached bitmap without loading from disk. - pub fn get_mut(&mut self, key: &CacheKey) -> Option<&mut CachedBitmap> { - self.cache.get_mut(key) + pub fn get_mut(&self, key: &CacheKey) -> Option { + let mut inner = self.inner.lock(); + let new_counter = inner.access_counter + 1; + inner.access_counter = new_counter; + inner.cache.get_mut(key).map(|bitmap| { + bitmap.last_access = new_counter; + bitmap.clone() + }) } /// Marks a cached bitmap dirty. - pub fn mark_dirty(&mut self, key: &CacheKey) { - if let Some(bitmap) = self.cache.get_mut(key) { + pub fn mark_dirty(&self, key: &CacheKey) { + if let Some(bitmap) = self.inner.lock().cache.get_mut(key) { bitmap.mark_dirty(); } } /// Modifies one cached bitmap and marks it dirty. pub fn modify( - &mut self, + &self, block_dev: &mut Jbd2Dev, key: CacheKey, block_num: AbsoluteBN, @@ -184,29 +205,102 @@ impl BitmapCache { B: BlockDevice, F: FnOnce(&mut [u8]), { - let bitmap = self.get_or_load_mut(block_dev, key, block_num)?; - debug!( - "BitmapCache::modify: key=({}:{:?}) block_num={} before_dirty={} (will apply \ - in-memory changes)", - key.group_id, key.bitmap_type, block_num, bitmap.dirty - ); + self.get_or_load_mut(block_dev, key, block_num)?; - f(&mut bitmap.data); - bitmap.mark_dirty(); + let mut inner = self.inner.lock(); + if let Some(bitmap) = inner.cache.get_mut(&key) { + debug!( + "BitmapCache::modify: key=({}:{:?}) block_num={} before_dirty={}", + key.group_id, key.bitmap_type, block_num, bitmap.dirty + ); - if !USE_MULTILEVEL_CACHE { - Self::write_bitmap_static(block_dev, bitmap.block_num, &bitmap.data)?; - bitmap.dirty = false; + f(&mut bitmap.data); + bitmap.mark_dirty(); + + if !USE_MULTILEVEL_CACHE { + let data = bitmap.data.clone(); + let blk = bitmap.block_num; + drop(inner); + Self::write_bitmap_static(block_dev, blk, &data)?; + inner = self.inner.lock(); + if let Some(bitmap) = inner.cache.get_mut(&key) { + bitmap.dirty = false; + } + } + + debug!( + "BitmapCache::modify: key=({}:{:?}) block_num={} marked_dirty=true", + key.group_id, key.bitmap_type, block_num + ); } + Ok(()) + } - debug!( - "BitmapCache::modify: key=({}:{:?}) block_num={} marked_dirty=true (bitmap updated in \ - cache, writeback deferred)", - key.group_id, key.bitmap_type, block_num - ); + /// Evicts one cached bitmap. + pub fn evict( + &self, + block_dev: &mut Jbd2Dev, + key: &CacheKey, + ) -> Ext4Result<()> { + self.inner.lock().do_evict(block_dev, key) + } + + /// Flushes all dirty bitmaps to disk. + pub fn flush_all(&self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + self.inner.lock().do_flush_all(block_dev) + } + + /// Flushes one bitmap to disk. + pub fn flush( + &self, + block_dev: &mut Jbd2Dev, + key: &CacheKey, + ) -> Ext4Result<()> { + self.inner.lock().do_flush(block_dev, key) + } + + /// Clears the cache without flushing. + pub fn clear(&self) { + self.inner.lock().cache.clear(); + } + + /// Returns cache statistics. + pub fn stats(&self) -> CacheStats { + let inner = self.inner.lock(); + let dirty_count = inner.cache.values().filter(|b| b.dirty).count(); + + CacheStats { + total_entries: inner.cache.len(), + dirty_entries: dirty_count, + max_entries: inner.max_entries, + } + } + + /// Writes one bitmap block to disk (static helper, uses local buffer). + fn write_bitmap_static( + block_dev: &mut Jbd2Dev, + block_num: AbsoluteBN, + data: &[u8], + ) -> Ext4Result<()> { + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + block_dev.read_blocks(&mut buf, block_num, 1)?; + buf[..data.len()].copy_from_slice(data); + block_dev.write_blocks(&buf, block_num, 1, true)?; Ok(()) } +} + +/// Cache statistics. +#[derive(Debug, Clone, Copy)] +pub struct CacheStats { + pub total_entries: usize, + pub dirty_entries: usize, + pub max_entries: usize, +} + +// ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── +impl BitmapCacheInner { /// Evicts the least recently used bitmap, flushing it first if needed. fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { let lru_key = self @@ -216,14 +310,12 @@ impl BitmapCache { .map(|(key, _)| *key); if let Some(key) = lru_key { - self.evict(block_dev, &key)?; + self.do_evict(block_dev, &key)?; } - Ok(()) } - /// Evicts one cached bitmap. - pub fn evict( + fn do_evict( &mut self, block_dev: &mut Jbd2Dev, key: &CacheKey, @@ -231,13 +323,29 @@ impl BitmapCache { if let Some(bitmap) = self.cache.remove(key) && bitmap.dirty { - Self::write_bitmap_static(block_dev, bitmap.block_num, &bitmap.data)?; + BitmapCache::write_bitmap_static(block_dev, bitmap.block_num, &bitmap.data)?; } Ok(()) } - /// Flushes all dirty bitmaps to disk. - pub fn flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + fn do_flush( + &mut self, + block_dev: &mut Jbd2Dev, + key: &CacheKey, + ) -> Ext4Result<()> { + if let Some(bitmap) = self.cache.get(key) + && bitmap.dirty + { + let data = bitmap.data.clone(); + BitmapCache::write_bitmap_static(block_dev, bitmap.block_num, &data)?; + if let Some(bitmap) = self.cache.get_mut(key) { + bitmap.dirty = false; + } + } + Ok(()) + } + + fn do_flush_all(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { let mut dirty_bitmaps: Vec<(CacheKey, AbsoluteBN, Vec)> = self .cache .iter() @@ -249,87 +357,26 @@ impl BitmapCache { return Ok(()); } - // Sort by physical block to keep writes ordered. dirty_bitmaps.sort_by_key(|(_, block_num, _)| *block_num); debug!( - "BitmapCache::flush_all: dirty_entries={} (will write all dirty bitmaps to disk)", + "BitmapCache::flush_all: dirty_entries={}", dirty_bitmaps.len() ); for (key, block_num, data) in dirty_bitmaps { debug!( - "BitmapCache::flush_all: writing bitmap key=({}:{:?}) block_num={} to disk", + "BitmapCache::flush_all: writing bitmap key=({}:{:?}) block_num={}", key.group_id, key.bitmap_type, block_num ); - - Self::write_bitmap_static(block_dev, block_num, &data)?; + BitmapCache::write_bitmap_static(block_dev, block_num, &data)?; } for bitmap in self.cache.values_mut() { bitmap.dirty = false; } - - Ok(()) - } - - /// Flushes one bitmap to disk. - pub fn flush( - &mut self, - block_dev: &mut Jbd2Dev, - key: &CacheKey, - ) -> Ext4Result<()> { - if let Some(bitmap) = self.cache.get(key) - && bitmap.dirty - { - let block_num = bitmap.block_num; - let data = bitmap.data.clone(); - - Self::write_bitmap_static(block_dev, block_num, &data)?; - - if let Some(bitmap) = self.cache.get_mut(key) { - bitmap.dirty = false; - } - } - Ok(()) - } - - /// Writes one bitmap block to disk. - fn write_bitmap_static( - block_dev: &mut Jbd2Dev, - block_num: AbsoluteBN, - data: &[u8], - ) -> Ext4Result<()> { - block_dev.read_block(block_num)?; - let buffer = block_dev.buffer_mut(); - buffer[..data.len()].copy_from_slice(data); - block_dev.write_block(block_num, true)?; Ok(()) } - - /// Clears the cache without flushing. - pub fn clear(&mut self) { - self.cache.clear(); - } - - /// Returns cache statistics. - pub fn stats(&self) -> CacheStats { - let dirty_count = self.cache.values().filter(|b| b.dirty).count(); - - CacheStats { - total_entries: self.cache.len(), - dirty_entries: dirty_count, - max_entries: self.max_entries, - } - } -} - -/// Cache statistics. -#[derive(Debug, Clone, Copy)] -pub struct CacheStats { - pub total_entries: usize, - pub dirty_entries: usize, - pub max_entries: usize, } #[cfg(test)] @@ -350,8 +397,7 @@ mod tests { #[test] fn test_cached_bitmap() { - use crate::BLOCK_SIZE; - let data = vec![0u8; BLOCK_SIZE]; + let data = vec![0u8; crate::config::BLOCK_SIZE]; let mut bitmap = CachedBitmap::new(data, AbsoluteBN::new(10)); assert!(!bitmap.dirty); diff --git a/components/rsext4/src/ext4/fs.rs b/components/rsext4/src/ext4/fs.rs index 4a4b81e142..ae872605f6 100644 --- a/components/rsext4/src/ext4/fs.rs +++ b/components/rsext4/src/ext4/fs.rs @@ -96,9 +96,9 @@ impl Ext4FileSystem { let bitmap_block = AbsoluteBN::new(desc.inode_bitmap()); let cache_key = CacheKey::new_inode(group_idx); - let bitmap = match self + let mut bitmap = match self .bitmap_cache - .get_or_load_mut(device, cache_key, bitmap_block) + .get_or_load(device, cache_key, bitmap_block) { Ok(b) => b, Err(e) => { From 9558d131f7ae8c71937f8dcdee5c1c735c58057c Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 12:30:11 +0800 Subject: [PATCH 03/20] fix(rsext4): fix deadlock and TOCTOU race in SMP cache locking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. DEADLOCK FIX — InodeCache::get_or_load: load_inode() acquired self.inner.lock() to read inode_size, but get_or_load already held that lock. Move the immutable inode_size field to the outer InodeCache struct so load_inode and calc_inode_location can read it without acquiring the spinlock. 2. TOCTOU RACE FIX — get_or_load_mut (all three caches): After dropping the lock for concurrent I/O and reacquiring it, the entry was inserted unconditionally, silently overwriting any entry inserted by another thread during the I/O window. Use entry().or_insert_with() to skip insertion when the key already exists. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 8 +++++-- components/rsext4/src/cache/data_block.rs | 14 +++++++++--- components/rsext4/src/cache/inode_table.rs | 26 +++++++++++++++++----- 3 files changed, 37 insertions(+), 11 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 8d7882f67a..6e923e889f 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -1,6 +1,7 @@ //! Bitmap cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; + use log::debug; use spin::Mutex as SpinMutex; @@ -158,8 +159,11 @@ impl BitmapCache { block_dev.read_blocks(&mut buf, block_num, 1)?; inner = self.inner.lock(); - let bitmap = CachedBitmap::new(buf, block_num); - inner.cache.insert(key, bitmap); + // Re-check after reacquiring: another thread may have inserted the + // same key while we held no lock. + inner.cache.entry(key).or_insert_with(|| { + CachedBitmap::new(buf, block_num) + }); } let new_counter = inner.access_counter + 1; diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index 2145e947b2..f7db6793b1 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -1,6 +1,7 @@ //! Data block cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; + use spin::Mutex as SpinMutex; use crate::{blockdev::*, bmalloc::AbsoluteBN, config::*, error::*}; @@ -135,8 +136,11 @@ impl DataBlockCache { let data = self.load_block(block_dev, block_num)?; inner = self.inner.lock(); - let cached = CachedBlock::new(data, block_num); - inner.cache.insert(block_num, cached); + // Re-check after reacquiring: another thread may have inserted the + // same key while we held no lock. + inner.cache.entry(block_num).or_insert_with(|| { + CachedBlock::new(data, block_num) + }); } let new_counter = inner.access_counter + 1; @@ -188,7 +192,11 @@ impl DataBlockCache { cached.last_access = new_counter; inner.cache.insert(block_num, cached); - inner.cache.get(&block_num).cloned().ok_or(Ext4Error::corrupted()) + inner + .cache + .get(&block_num) + .cloned() + .ok_or(Ext4Error::corrupted()) } /// Marks a cached data block dirty. diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index a76bbde958..37c3a433bf 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -1,6 +1,7 @@ //! Inode table cache helpers. use alloc::{collections::BTreeMap, vec::Vec}; + use spin::Mutex as SpinMutex; use crate::{ @@ -76,7 +77,8 @@ struct InodeCacheInner { max_entries: usize, /// Access counter used by the LRU policy. access_counter: u64, - /// On-disk inode size in bytes. + /// On-disk inode size in bytes (immutable after construction; + /// mirrored in `InodeCache` for lock-free access). inode_size: usize, } @@ -85,8 +87,14 @@ struct InodeCacheInner { /// All methods take `&self`; the internal `SpinMutex` provides interior /// mutability. Callers must ensure the block device (`Jbd2Dev`) is /// externally synchronized (e.g. via the VFS-layer inode lock). +/// +/// `inode_size` is stored outside the spinlock because it is immutable after +/// construction and is needed by lock-free paths (`calc_inode_location`) and +/// by `load_inode` which is called from code paths that already hold the lock. pub struct InodeCache { inner: SpinMutex, + /// On-disk inode size in bytes (immutable after construction). + inode_size: usize, } impl InodeCache { @@ -99,6 +107,7 @@ impl InodeCache { access_counter: 0, inode_size, }), + inode_size, } } @@ -117,9 +126,8 @@ impl InodeCache { inode_table_start: AbsoluteBN, block_size: usize, ) -> Ext4Result<(AbsoluteBN, usize, BGIndex)> { - let inner = self.inner.lock(); let (group_idx, idx_in_group) = inode_num.to_group(inodes_per_group)?; - let byte_offset = idx_in_group.as_usize()? * inner.inode_size; + let byte_offset = idx_in_group.as_usize()? * self.inode_size; let block_offset = byte_offset / block_size; let offset_in_block = byte_offset % block_size; @@ -130,13 +138,16 @@ impl InodeCache { } /// Loads one inode from disk using a caller-provided buffer. + /// + /// Lock-free: uses `self.inode_size` (immutable after construction) + /// so it is safe to call from code paths that already hold the lock. fn load_inode( &self, block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, offset: usize, ) -> Ext4Result { - let inode_size = self.inner.lock().inode_size; + let inode_size = self.inode_size; // Use a local buffer to avoid the BlockDev single-block buffer, // which is shared mutable state that would serialize concurrent reads. let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; @@ -206,8 +217,11 @@ impl InodeCache { let inode = self.load_inode(block_dev, block_num, offset)?; inner = self.inner.lock(); - let cached = CachedInode::new(inode, inode_num, block_num, offset); - inner.cache.insert(inode_num, cached); + // Re-check after reacquiring: another thread may have inserted the + // same key while we held no lock. + inner.cache.entry(inode_num).or_insert_with(|| { + CachedInode::new(inode, inode_num, block_num, offset) + }); } // Refresh the LRU timestamp before returning the mutable handle. From b54b485b8c46e1d3806ba406a5ddb0a960694d5d Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 12:52:47 +0800 Subject: [PATCH 04/20] fix(rsext4): drop spinlock during I/O in cache get_or_load, fix is_metadata 1. is_metadata: Inode table writes must use is_metadata=true to go through the JBD2 journal. Changed write_inode_bytes_static and do_flush_all from false to true. Old code used write_block(_, true); this was incorrectly changed to write_blocks(_, false). 2. get_or_load spinlock hold: The read path (get_or_load) now drops the per-cache spinlock before calling load_inode/load_block/evict_lru, matching the pattern already used by get_or_load_mut. This prevents busy-waiting on the spinlock during disk I/O (5-25ms for HDD). 3. get_or_load_mut evict path: The write path (get_or_load_mut) now also uses snapshot_lru for the eviction step, so the spinlock is dropped during dirty LRU flush. 4. create_new: Refactored to snapshot-and-release pattern, dropping the spinlock during dirty eviction writes. 5. evict_lru -> snapshot_lru: Replaced evict_lru(&mut self) which required holding the lock, with snapshot_lru(&self) that returns a snapshot for the caller to flush lock-free. Verification: 128/128 tests pass, clippy clean, StarryOS builds. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 84 ++++++++++---- components/rsext4/src/cache/data_block.rs | 125 +++++++++++++++++---- components/rsext4/src/cache/inode_table.rs | 102 ++++++++++++----- 3 files changed, 239 insertions(+), 72 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 6e923e889f..5825df22f8 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -13,6 +13,10 @@ use crate::{ error::*, }; +/// Snapshot type for lock-free LRU eviction. +/// `(lru_key, optional dirty data: (block_num, data))` +type BitmapLruSnapshot = Option<(CacheKey, Option<(AbsoluteBN, Vec)>)>; + /// Type of bitmap stored in the cache. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] pub enum BitmapType { @@ -118,16 +122,34 @@ impl BitmapCache { let mut inner = self.inner.lock(); if !inner.cache.contains_key(&key) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; + + drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some((_lru_key, Some((lru_bn, ref lru_data)))) = evict_info { + Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; } let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; - let data = buf; - let bitmap = CachedBitmap::new(data, block_num); - inner.cache.insert(key, bitmap); + // Phase 3: reacquire the lock and apply the eviction + insertion. + inner = self.inner.lock(); + + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + + inner + .cache + .entry(key) + .or_insert_with(|| CachedBitmap::new(buf, block_num)); } let new_counter = inner.access_counter + 1; @@ -149,21 +171,36 @@ impl BitmapCache { let mut inner = self.inner.lock(); if !inner.cache.contains_key(&key) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; - } + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; - // Drop lock during I/O for concurrency. drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some((_lru_key, Some((lru_bn, ref lru_data)))) = evict_info { + Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; + } + let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; + + // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. - inner.cache.entry(key).or_insert_with(|| { - CachedBitmap::new(buf, block_num) - }); + inner + .cache + .entry(key) + .or_insert_with(|| CachedBitmap::new(buf, block_num)); } let new_counter = inner.access_counter + 1; @@ -305,18 +342,27 @@ pub struct CacheStats { // ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── impl BitmapCacheInner { - /// Evicts the least recently used bitmap, flushing it first if needed. - fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + /// Snapshots the LRU bitmap for lock-free eviction. + /// + /// Returns `Some(lru_key, Some(data))` when dirty, `Some(lru_key, None)` when + /// clean, `None` when the cache is empty. The caller must do the I/O + /// *without* holding the spinlock, then re-lock and remove `lru_key`. + fn snapshot_lru(&self) -> BitmapLruSnapshot { let lru_key = self .cache .iter() .min_by_key(|(_, bitmap)| bitmap.last_access) - .map(|(key, _)| *key); + .map(|(key, _)| *key)?; - if let Some(key) = lru_key { - self.do_evict(block_dev, &key)?; - } - Ok(()) + let dirty_info = self.cache.get(&lru_key).and_then(|bitmap| { + if bitmap.dirty { + Some((bitmap.block_num, bitmap.data.clone())) + } else { + None + } + }); + + Some((lru_key, dirty_info)) } fn do_evict( diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index f7db6793b1..b6049b00b9 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -95,13 +95,33 @@ impl DataBlockCache { let mut inner = self.inner.lock(); if !inner.cache.contains_key(&block_num) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; + + drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some((_lru_key, Some(ref lru_data))) = evict_info { + Self::write_block_static(block_dev, _lru_key, lru_data)?; } let data = self.load_block(block_dev, block_num)?; - let cached = CachedBlock::new(data, block_num); - inner.cache.insert(block_num, cached); + + // Phase 3: reacquire the lock and apply the eviction + insertion. + inner = self.inner.lock(); + + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + + inner + .cache + .entry(block_num) + .or_insert_with(|| CachedBlock::new(data, block_num)); } // Refresh the LRU timestamp. @@ -127,20 +147,35 @@ impl DataBlockCache { let mut inner = self.inner.lock(); if !inner.cache.contains_key(&block_num) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; - } + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; - // Drop lock during I/O for concurrency. drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some((_lru_key, Some(ref lru_data))) = evict_info { + Self::write_block_static(block_dev, _lru_key, lru_data)?; + } + let data = self.load_block(block_dev, block_num)?; + + // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. - inner.cache.entry(block_num).or_insert_with(|| { - CachedBlock::new(data, block_num) - }); + inner + .cache + .entry(block_num) + .or_insert_with(|| CachedBlock::new(data, block_num)); } let new_counter = inner.access_counter + 1; @@ -175,12 +210,35 @@ impl DataBlockCache { ) -> Ext4Result { let mut inner = self.inner.lock(); - if inner.cache.contains_key(&block_num) { - inner.do_evict(block_dev, block_num)?; + // Phase 1: snapshot eviction info while holding the lock. + // Two evictions may be needed: (a) the same block number from a + // previous incarnation, and (b) an LRU slot to stay within max_entries. + let evict_existing = inner.snapshot_block_for_evict(block_num); + let evict_lru_info = if inner.cache.len() >= inner.max_entries && evict_existing.is_none() { + // Only evict LRU if we did not already free a slot above. + inner.snapshot_lru() + } else { + None + }; + + drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some(ref data) = evict_existing { + Self::write_block_static(block_dev, block_num, data)?; } + if let Some((lru_key, Some(ref lru_data))) = evict_lru_info { + Self::write_block_static(block_dev, lru_key, lru_data)?; + } + + // Phase 3: reacquire the lock and apply evictions + insertion. + inner = self.inner.lock(); - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; + if evict_existing.is_some() { + inner.cache.remove(&block_num); + } + if let Some((lru_key, _)) = evict_lru_info { + inner.cache.remove(&lru_key); } let data = alloc::vec![0u8; inner.block_size]; @@ -328,18 +386,41 @@ pub struct DataBlockCacheStats { // ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── impl DataBlockCacheInner { - /// Evicts the least recently used block, flushing it first if needed. - fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + /// Snapshots the LRU data block for lock-free eviction. + /// + /// Returns `Some(lru_key, Some(data))` when dirty, `Some(lru_key, None)` when + /// clean, `None` when the cache is empty. The caller must do the I/O + /// *without* holding the spinlock, then re-lock and remove `lru_key`. + fn snapshot_lru(&self) -> Option<(AbsoluteBN, Option>)> { let lru_key = self .cache .iter() .min_by_key(|(_, cached)| cached.last_access) - .map(|(key, _)| *key); + .map(|(key, _)| *key)?; - if let Some(key) = lru_key { - self.do_evict(block_dev, key)?; - } - Ok(()) + let dirty_data = self.cache.get(&lru_key).and_then(|cached| { + if cached.dirty { + Some(cached.data.clone()) + } else { + None + } + }); + + Some((lru_key, dirty_data)) + } + + /// Snapshots a single block for lock-free eviction. + /// + /// Returns `Some(data)` when the block exists and is dirty, `None` when + /// the block does not exist or is clean. + fn snapshot_block_for_evict(&self, block_num: AbsoluteBN) -> Option> { + self.cache.get(&block_num).and_then(|cached| { + if cached.dirty { + Some(cached.data.clone()) + } else { + None + } + }) } fn do_evict( diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index 37c3a433bf..c8f25d8dfa 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -13,6 +13,10 @@ use crate::{ error::*, }; +/// Snapshot type for lock-free LRU eviction. +/// `(lru_key, optional dirty data: (block_num, offset, data))` +type InodeLruSnapshot = Option<(InodeNumber, Option<(AbsoluteBN, usize, Vec)>)>; + /// Cache key for one global inode number. pub type InodeCacheKey = InodeNumber; @@ -173,13 +177,36 @@ impl InodeCache { // Load the inode from disk on the first cache miss. if !inner.cache.contains_key(&inode_num) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; + + drop(inner); + + // Phase 2: do I/O without holding the spinlock so other cores can + // make progress on cache hits. + if let Some((_lru_key, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { + Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; } let inode = self.load_inode(block_dev, block_num, offset)?; - let cached = CachedInode::new(inode, inode_num, block_num, offset); - inner.cache.insert(inode_num, cached); + + // Phase 3: reacquire the lock and apply the eviction + insertion. + inner = self.inner.lock(); + + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + + // Use or_insert_with to avoid TOCTOU: another thread may have + // inserted the same key while we had no lock. + inner + .cache + .entry(inode_num) + .or_insert_with(|| CachedInode::new(inode, inode_num, block_num, offset)); } // Refresh the LRU timestamp on every access. @@ -208,20 +235,35 @@ impl InodeCache { // Load the inode from disk on the first mutable cache miss. if !inner.cache.contains_key(&inode_num) { - if inner.cache.len() >= inner.max_entries { - inner.evict_lru(block_dev)?; - } + // Phase 1: snapshot LRU eviction info while holding the lock. + let evict_info = if inner.cache.len() >= inner.max_entries { + inner.snapshot_lru() + } else { + None + }; - // Drop the lock during I/O to allow concurrent cache accesses drop(inner); + + // Phase 2: do I/O without holding the spinlock. + if let Some((_lru_key, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { + Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; + } + let inode = self.load_inode(block_dev, block_num, offset)?; + + // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); + if let Some((lru_key, _)) = evict_info { + inner.cache.remove(&lru_key); + } + // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. - inner.cache.entry(inode_num).or_insert_with(|| { - CachedInode::new(inode, inode_num, block_num, offset) - }); + inner + .cache + .entry(inode_num) + .or_insert_with(|| CachedInode::new(inode, inode_num, block_num, offset)); } // Refresh the LRU timestamp before returning the mutable handle. @@ -368,7 +410,7 @@ impl InodeCache { let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; buf[offset..offset + data.len()].copy_from_slice(data); - block_dev.write_blocks(&buf, block_num, 1, false)?; + block_dev.write_blocks(&buf, block_num, 1, true)?; // is_metadata: inode table blocks are filesystem metadata Ok(()) } } @@ -384,32 +426,30 @@ pub struct InodeCacheStats { // ── Inner methods (caller holds `self.inner.lock()`) ───────────────────────── impl InodeCacheInner { - /// Evicts the least recently used inode, flushing it first if needed. - fn evict_lru(&mut self, block_dev: &mut Jbd2Dev) -> Ext4Result<()> { + /// Snapshots the LRU entry for lock-free eviction. + /// + /// Returns `(lru_key, dirty_write_info)` where `dirty_write_info` is + /// `Some((block_num, offset, data))` when the entry is dirty and must be + /// flushed to disk before eviction. The caller must do the I/O + /// *without* holding the spinlock, then re-lock and remove `lru_key`. + fn snapshot_lru(&self) -> InodeLruSnapshot { let lru_key = self .cache .iter() .min_by_key(|(_, cached)| cached.last_access) - .map(|(key, _)| *key); - - if let Some(key) = lru_key { - // Drop the lock during synchronous I/O to avoid holding - // the spinlock across a potentially slow block-device call. - if let Some(cached) = self.cache.get(&key) - && cached.dirty - { - let block_num = cached.block_num; - let offset = cached.offset_in_block; + .map(|(key, _)| *key)?; + + let dirty_info = self.cache.get(&lru_key).and_then(|cached| { + if cached.dirty { let mut buf = alloc::vec![0u8; self.inode_size]; cached.inode.to_disk_bytes(&mut buf); - // We cannot drop the lock here because we need &mut self. - // Instead, do the write inline (the evict_lru path is rare). - InodeCache::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; + Some((cached.block_num, cached.offset_in_block, buf)) + } else { + None } - self.cache.remove(&key); - } + }); - Ok(()) + Some((lru_key, dirty_info)) } fn do_evict( @@ -488,7 +528,7 @@ impl InodeCacheInner { idx += 1; } - block_dev.write_blocks(&buf, block_num, 1, false)?; + block_dev.write_blocks(&buf, block_num, 1, true)?; // is_metadata: inode table blocks are filesystem metadata } // All flushed entries are now clean. From 4dcf2dc8a493c4d5517506013081d1a57c073444 Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 13:39:11 +0800 Subject: [PATCH 05/20] fix(axfs-ng): revert VFS lock to SpinNoIrq to avoid IRQ-context panic The ax_sync::Mutex panics when called from IRQ context (e.g., DHCP during network init triggers filesystem operations with irq_enabled=false). The fine-grained cache locks (spin::Mutex in rsext4 inode_table, data_block, bitmap) already provide SMP concurrency; the VFS-level lock must remain a spinlock because it is called from both task and IRQ contexts. CI failure: Test starry loongarch64 qemu / dhcp panicked at os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs:76:20: sleeping or rescheduling is not allowed in atomic context Co-Authored-By: Claude Opus 4.8 --- .../modules/axfs-ng/src/fs/ext4/rsext4/fs.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs index 260e5b4ce1..af72c04443 100644 --- a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs +++ b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs @@ -1,7 +1,7 @@ use alloc::{boxed::Box, sync::Arc}; use core::cell::OnceCell; -use ax_sync::Mutex; +use ax_kspin::{SpinNoIrq as Mutex, SpinNoIrqGuard as MutexGuard}; use axfs_ng_vfs::{ DirEntry, DirNode, Filesystem, FilesystemOps, Reference, StatFs, VfsResult, path::MAX_NAME_LEN, }; @@ -63,16 +63,13 @@ impl Ext4Filesystem { /// Locks the shared rsext4 state. /// - /// Uses a blocking `ax_sync::Mutex` instead of `SpinNoIrq`: on SMP, a - /// blocking mutex puts waiting tasks to sleep, eliminating the busy-wait - /// CPU waste that makes SMP > 1 no faster than SMP = 1 during I/O-heavy - /// workloads like `cargo build`. - /// - /// The rsext4 caches (inode, data-block, bitmap) have their own internal - /// `spin::Mutex` for fine-grained concurrent access. This global lock - /// protects metadata mutations (allocators, superblock, group descriptors, - /// journal commits). - pub(crate) fn lock(&self) -> ax_sync::MutexGuard<'_, Ext4State> { + /// Uses `SpinNoIrq` rather than a blocking mutex because filesystem + /// operations may be called from IRQ context (e.g., DHCP during network + /// init), where sleeping is not allowed. The rsext4 caches (inode, + /// data-block, bitmap) provide fine-grained `spin::Mutex` for SMP + /// concurrency; this global lock protects metadata mutations (allocators, + /// superblock, group descriptors, journal commits). + pub(crate) fn lock(&self) -> MutexGuard<'_, Ext4State> { self.inner.lock() } From 52553b54717a0cd15c0db34160168808c70dce93 Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 14:23:48 +0800 Subject: [PATCH 06/20] =?UTF-8?q?fix(rsext4):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20clone=20mutation,=20block=5Fsize,=20modify=20error?= =?UTF-8?q?=20handling?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - mkdir/bootstrap: use modify_new() instead of create_new()+mutation to fix lost writes (create_new returns a clone; mutations on the clone were silently discarded instead of updating the cache entry) - data_block: pull block_size out of the Mutex (like InodeCache.inode_size) so load_block and write_block_static use the runtime block_size instead of hard-coded crate::config::BLOCK_SIZE - all three caches: modify() now returns Ext4Error::corrupted() when the entry is unexpectedly missing after get_or_load_mut succeeded, instead of silently returning Ok(()) without applying the mutation closure Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 44 ++++++++++---------- components/rsext4/src/cache/data_block.rs | 48 ++++++++++++---------- components/rsext4/src/cache/inode_table.rs | 22 ++++++---- components/rsext4/src/dir/bootstrap.rs | 28 +++++-------- components/rsext4/src/dir/mkdir.rs | 10 ++--- 5 files changed, 78 insertions(+), 74 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 5825df22f8..05a1cb275a 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -249,31 +249,33 @@ impl BitmapCache { self.get_or_load_mut(block_dev, key, block_num)?; let mut inner = self.inner.lock(); - if let Some(bitmap) = inner.cache.get_mut(&key) { - debug!( - "BitmapCache::modify: key=({}:{:?}) block_num={} before_dirty={}", - key.group_id, key.bitmap_type, block_num, bitmap.dirty - ); + let bitmap = inner + .cache + .get_mut(&key) + .ok_or(Ext4Error::corrupted())?; + debug!( + "BitmapCache::modify: key=({}:{:?}) block_num={} before_dirty={}", + key.group_id, key.bitmap_type, block_num, bitmap.dirty + ); - f(&mut bitmap.data); - bitmap.mark_dirty(); + f(&mut bitmap.data); + bitmap.mark_dirty(); - if !USE_MULTILEVEL_CACHE { - let data = bitmap.data.clone(); - let blk = bitmap.block_num; - drop(inner); - Self::write_bitmap_static(block_dev, blk, &data)?; - inner = self.inner.lock(); - if let Some(bitmap) = inner.cache.get_mut(&key) { - bitmap.dirty = false; - } + if !USE_MULTILEVEL_CACHE { + let data = bitmap.data.clone(); + let blk = bitmap.block_num; + drop(inner); + Self::write_bitmap_static(block_dev, blk, &data)?; + inner = self.inner.lock(); + if let Some(bitmap) = inner.cache.get_mut(&key) { + bitmap.dirty = false; } - - debug!( - "BitmapCache::modify: key=({}:{:?}) block_num={} marked_dirty=true", - key.group_id, key.bitmap_type, block_num - ); } + + debug!( + "BitmapCache::modify: key=({}:{:?}) block_num={} marked_dirty=true", + key.group_id, key.bitmap_type, block_num + ); Ok(()) } diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index b6049b00b9..8474ee1227 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -55,6 +55,8 @@ struct DataBlockCacheInner { /// Callers must ensure the block device (`Jbd2Dev`) is externally synchronized. pub struct DataBlockCache { inner: SpinMutex, + /// Filesystem block size in bytes (immutable after construction). + block_size: usize, } impl DataBlockCache { @@ -67,6 +69,7 @@ impl DataBlockCache { access_counter: 0, block_size, }), + block_size, } } @@ -81,7 +84,7 @@ impl DataBlockCache { block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, ) -> Ext4Result> { - let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + let mut buf = alloc::vec![0u8; self.block_size]; block_dev.read_blocks(&mut buf, block_num, 1)?; Ok(buf) } @@ -106,7 +109,7 @@ impl DataBlockCache { // Phase 2: do I/O without holding the spinlock. if let Some((_lru_key, Some(ref lru_data))) = evict_info { - Self::write_block_static(block_dev, _lru_key, lru_data)?; + Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; } let data = self.load_block(block_dev, block_num)?; @@ -158,7 +161,7 @@ impl DataBlockCache { // Phase 2: do I/O without holding the spinlock. if let Some((_lru_key, Some(ref lru_data))) = evict_info { - Self::write_block_static(block_dev, _lru_key, lru_data)?; + Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; } let data = self.load_block(block_dev, block_num)?; @@ -225,10 +228,10 @@ impl DataBlockCache { // Phase 2: do I/O without holding the spinlock. if let Some(ref data) = evict_existing { - Self::write_block_static(block_dev, block_num, data)?; + Self::write_block_static(block_dev, block_num, data, self.block_size)?; } if let Some((lru_key, Some(ref lru_data))) = evict_lru_info { - Self::write_block_static(block_dev, lru_key, lru_data)?; + Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; } // Phase 3: reacquire the lock and apply evictions + insertion. @@ -279,20 +282,22 @@ impl DataBlockCache { self.get_or_load_mut(block_dev, block_num)?; let mut inner = self.inner.lock(); - if let Some(cached) = inner.cache.get_mut(&block_num) { - f(&mut cached.data); - cached.mark_dirty(); + let cached = inner + .cache + .get_mut(&block_num) + .ok_or(Ext4Error::corrupted())?; + f(&mut cached.data); + cached.mark_dirty(); - if !USE_MULTILEVEL_CACHE { - let data = cached.data.clone(); - let blk = cached.block_num; - drop(inner); - Self::write_block_static(block_dev, blk, &data)?; + if !USE_MULTILEVEL_CACHE { + let data = cached.data.clone(); + let blk = cached.block_num; + drop(inner); + Self::write_block_static(block_dev, blk, &data, self.block_size)?; - inner = self.inner.lock(); - if let Some(cached) = inner.cache.get_mut(&block_num) { - cached.dirty = false; - } + inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&block_num) { + cached.dirty = false; } } Ok(()) @@ -360,13 +365,14 @@ impl DataBlockCache { } } - /// Writes one block to disk (static, lock-free helper with local buffer). + /// Writes one block to disk (static helper, takes runtime block_size). fn write_block_static( block_dev: &mut Jbd2Dev, block_num: AbsoluteBN, data: &[u8], + block_size: usize, ) -> Ext4Result<()> { - let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + let mut buf = alloc::vec![0u8; block_size]; block_dev.read_blocks(&mut buf, block_num, 1)?; buf[..data.len()].copy_from_slice(data); block_dev.write_blocks(&buf, block_num, 1, false)?; @@ -431,7 +437,7 @@ impl DataBlockCacheInner { if let Some(cached) = self.cache.remove(&block_num) && cached.dirty { - DataBlockCache::write_block_static(block_dev, cached.block_num, &cached.data)?; + DataBlockCache::write_block_static(block_dev, cached.block_num, &cached.data, self.block_size)?; } Ok(()) } @@ -445,7 +451,7 @@ impl DataBlockCacheInner { && cached.dirty { let data = cached.data.clone(); - DataBlockCache::write_block_static(block_dev, block_num, &data)?; + DataBlockCache::write_block_static(block_dev, block_num, &data, self.block_size)?; if let Some(cached) = self.cache.get_mut(&block_num) { cached.dirty = false; diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index c8f25d8dfa..f44f414180 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -318,15 +318,20 @@ impl InodeCache { let mut inner = self.inner.lock(); let inode_size = inner.inode_size; - if let Some(cached) = inner.cache.get_mut(&inode_num) { - f(&mut cached.inode); - cached.mark_dirty(); + // get_or_load_mut succeeded above, so the entry must exist unless + // concurrently evicted — treat that as filesystem corruption. + let cached = inner + .cache + .get_mut(&inode_num) + .ok_or(Ext4Error::corrupted())?; + f(&mut cached.inode); + cached.mark_dirty(); - if !USE_MULTILEVEL_CACHE { - // Drop lock during synchronous I/O - let block_num = cached.block_num; - let offset = cached.offset_in_block; - let mut buf = alloc::vec![0u8; inode_size]; + if !USE_MULTILEVEL_CACHE { + // Drop lock during synchronous I/O + let block_num = cached.block_num; + let offset = cached.offset_in_block; + let mut buf = alloc::vec![0u8; inode_size]; cached.inode.to_disk_bytes(&mut buf); drop(inner); @@ -337,7 +342,6 @@ impl InodeCache { cached.dirty = false; } } - } Ok(()) } diff --git a/components/rsext4/src/dir/bootstrap.rs b/components/rsext4/src/dir/bootstrap.rs index 91437e3e98..6b4c7e3d80 100644 --- a/components/rsext4/src/dir/bootstrap.rs +++ b/components/rsext4/src/dir/bootstrap.rs @@ -25,11 +25,9 @@ pub fn create_root_directory_entry( let has_checksum = ext4_superblock_has_metadata_csum(&fs.superblock); let root_gen = fs.get_root(block_dev)?.i_generation; - { - // Format the initial root directory block before the inode is finalized. - let mut cached = fs.datablock_cache.create_new(block_dev, data_block)?; - let data = &mut cached.data; - + // Format the initial root directory block through modify_new so + // mutations are persisted in the cache entry rather than on a clone. + fs.datablock_cache.modify_new(block_dev, data_block, |data| { let dot_name = b"."; let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); let dot = Ext4DirEntry2::new( @@ -54,18 +52,14 @@ pub fn create_root_directory_entry( dotdot_name, ); - { - dot.to_disk_bytes(&mut data[0..8]); - let name_len = dot.name_len as usize; - data[8..8 + name_len].copy_from_slice(&dot.name[..name_len]); - } + dot.to_disk_bytes(&mut data[0..8]); + let name_len = dot.name_len as usize; + data[8..8 + name_len].copy_from_slice(&dot.name[..name_len]); - { - let offset = dot_rec_len as usize; - dotdot.to_disk_bytes(&mut data[offset..offset + 8]); - let name_len = dotdot.name_len as usize; - data[offset + 8..offset + 8 + name_len].copy_from_slice(&dotdot.name[..name_len]); - } + let offset = dot_rec_len as usize; + dotdot.to_disk_bytes(&mut data[offset..offset + 8]); + let name_len = dotdot.name_len as usize; + data[offset + 8..offset + 8 + name_len].copy_from_slice(&dotdot.name[..name_len]); if has_checksum { let tail = Ext4DirEntryTail::new(); @@ -75,7 +69,7 @@ pub fn create_root_directory_entry( ); update_ext4_dirblock_csum32(&fs.superblock, root_inode_num.raw(), root_gen, data); } - } + })?; // Persist a clean directory inode that points at the newly initialized block. let dir_mode = Ext4Inode::S_IFDIR | 0o755; diff --git a/components/rsext4/src/dir/mkdir.rs b/components/rsext4/src/dir/mkdir.rs index f42018f814..a6e61e4414 100644 --- a/components/rsext4/src/dir/mkdir.rs +++ b/components/rsext4/src/dir/mkdir.rs @@ -100,11 +100,9 @@ fn mkdir_internal( let data_block = fs.alloc_block(device)?; let new_dir_gen = fs.get_inode_by_num(device, new_dir_ino)?.i_generation; - { - // Initialize `.` and `..`, leaving room for the checksum tail when enabled. - let mut cached = fs.datablock_cache.create_new(device, data_block)?; - let data = &mut cached.data; - + // Initialize `.` and `..` through modify_new so mutations are persisted + // in the cache entry rather than on a detached clone. + fs.datablock_cache.modify_new(device, data_block, |data| { let dot_name = b"."; let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); let dot = Ext4DirEntry2::new( @@ -146,7 +144,7 @@ fn mkdir_internal( ); update_ext4_dirblock_csum32(&fs.superblock, new_dir_ino.raw(), new_dir_gen, data); } - } + })?; // Persist the child directory inode through the unified metadata path. let (group_idx, _idx) = fs.inode_allocator.global_to_group(new_dir_ino)?; From 46503bb9719f72f28ec3c0ce69dd4b63ce4c091a Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 14:26:19 +0800 Subject: [PATCH 07/20] style(rsext4): cargo fmt --- components/rsext4/src/cache/bitmap.rs | 5 +- components/rsext4/src/cache/data_block.rs | 7 +- components/rsext4/src/cache/inode_table.rs | 14 ++-- components/rsext4/src/dir/bootstrap.rs | 79 +++++++++++----------- 4 files changed, 54 insertions(+), 51 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 05a1cb275a..3b28433401 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -249,10 +249,7 @@ impl BitmapCache { self.get_or_load_mut(block_dev, key, block_num)?; let mut inner = self.inner.lock(); - let bitmap = inner - .cache - .get_mut(&key) - .ok_or(Ext4Error::corrupted())?; + let bitmap = inner.cache.get_mut(&key).ok_or(Ext4Error::corrupted())?; debug!( "BitmapCache::modify: key=({}:{:?}) block_num={} before_dirty={}", key.group_id, key.bitmap_type, block_num, bitmap.dirty diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index 8474ee1227..ff1eac6450 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -437,7 +437,12 @@ impl DataBlockCacheInner { if let Some(cached) = self.cache.remove(&block_num) && cached.dirty { - DataBlockCache::write_block_static(block_dev, cached.block_num, &cached.data, self.block_size)?; + DataBlockCache::write_block_static( + block_dev, + cached.block_num, + &cached.data, + self.block_size, + )?; } Ok(()) } diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index f44f414180..252dc492ab 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -332,16 +332,16 @@ impl InodeCache { let block_num = cached.block_num; let offset = cached.offset_in_block; let mut buf = alloc::vec![0u8; inode_size]; - cached.inode.to_disk_bytes(&mut buf); - drop(inner); + cached.inode.to_disk_bytes(&mut buf); + drop(inner); - Self::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; + Self::write_inode_bytes_static(block_dev, block_num, offset, &buf)?; - inner = self.inner.lock(); - if let Some(cached) = inner.cache.get_mut(&inode_num) { - cached.dirty = false; - } + inner = self.inner.lock(); + if let Some(cached) = inner.cache.get_mut(&inode_num) { + cached.dirty = false; } + } Ok(()) } diff --git a/components/rsext4/src/dir/bootstrap.rs b/components/rsext4/src/dir/bootstrap.rs index 6b4c7e3d80..0f8edec594 100644 --- a/components/rsext4/src/dir/bootstrap.rs +++ b/components/rsext4/src/dir/bootstrap.rs @@ -27,49 +27,50 @@ pub fn create_root_directory_entry( // Format the initial root directory block through modify_new so // mutations are persisted in the cache entry rather than on a clone. - fs.datablock_cache.modify_new(block_dev, data_block, |data| { - let dot_name = b"."; - let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); - let dot = Ext4DirEntry2::new( - root_inode_num.raw(), - dot_rec_len, - Ext4DirEntry2::EXT4_FT_DIR, - dot_name, - ); + fs.datablock_cache + .modify_new(block_dev, data_block, |data| { + let dot_name = b"."; + let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); + let dot = Ext4DirEntry2::new( + root_inode_num.raw(), + dot_rec_len, + Ext4DirEntry2::EXT4_FT_DIR, + dot_name, + ); - let dotdot_name = b".."; - let dotdot_rec_len = if has_checksum { - (BLOCK_SIZE as u16) - .saturating_sub(dot_rec_len) - .saturating_sub(Ext4DirEntryTail::TAIL_LEN) - } else { - (BLOCK_SIZE as u16).saturating_sub(dot_rec_len) - }; - let dotdot = Ext4DirEntry2::new( - root_inode_num.raw(), - dotdot_rec_len, - Ext4DirEntry2::EXT4_FT_DIR, - dotdot_name, - ); + let dotdot_name = b".."; + let dotdot_rec_len = if has_checksum { + (BLOCK_SIZE as u16) + .saturating_sub(dot_rec_len) + .saturating_sub(Ext4DirEntryTail::TAIL_LEN) + } else { + (BLOCK_SIZE as u16).saturating_sub(dot_rec_len) + }; + let dotdot = Ext4DirEntry2::new( + root_inode_num.raw(), + dotdot_rec_len, + Ext4DirEntry2::EXT4_FT_DIR, + dotdot_name, + ); - dot.to_disk_bytes(&mut data[0..8]); - let name_len = dot.name_len as usize; - data[8..8 + name_len].copy_from_slice(&dot.name[..name_len]); + dot.to_disk_bytes(&mut data[0..8]); + let name_len = dot.name_len as usize; + data[8..8 + name_len].copy_from_slice(&dot.name[..name_len]); - let offset = dot_rec_len as usize; - dotdot.to_disk_bytes(&mut data[offset..offset + 8]); - let name_len = dotdot.name_len as usize; - data[offset + 8..offset + 8 + name_len].copy_from_slice(&dotdot.name[..name_len]); + let offset = dot_rec_len as usize; + dotdot.to_disk_bytes(&mut data[offset..offset + 8]); + let name_len = dotdot.name_len as usize; + data[offset + 8..offset + 8 + name_len].copy_from_slice(&dotdot.name[..name_len]); - if has_checksum { - let tail = Ext4DirEntryTail::new(); - let tail_offset = BLOCK_SIZE - Ext4DirEntryTail::TAIL_LEN as usize; - tail.to_disk_bytes( - &mut data[tail_offset..tail_offset + Ext4DirEntryTail::TAIL_LEN as usize], - ); - update_ext4_dirblock_csum32(&fs.superblock, root_inode_num.raw(), root_gen, data); - } - })?; + if has_checksum { + let tail = Ext4DirEntryTail::new(); + let tail_offset = BLOCK_SIZE - Ext4DirEntryTail::TAIL_LEN as usize; + tail.to_disk_bytes( + &mut data[tail_offset..tail_offset + Ext4DirEntryTail::TAIL_LEN as usize], + ); + update_ext4_dirblock_csum32(&fs.superblock, root_inode_num.raw(), root_gen, data); + } + })?; // Persist a clean directory inode that points at the newly initialized block. let dir_mode = Ext4Inode::S_IFDIR | 0o755; From ca8bdc3c25cd85b8fe628e9126458f86de97e6c0 Mon Sep 17 00:00:00 2001 From: Tempest Date: Sun, 31 May 2026 15:09:19 +0800 Subject: [PATCH 08/20] docs(starry): cherry-pick self-compilation documentation from feat/starry-self-compilation-v2 --- docs/starryos-self-compilation.md | 48 ++++++++++++------------------- 1 file changed, 19 insertions(+), 29 deletions(-) diff --git a/docs/starryos-self-compilation.md b/docs/starryos-self-compilation.md index 3ffe355b4d..1f40d250a3 100644 --- a/docs/starryos-self-compilation.md +++ b/docs/starryos-self-compilation.md @@ -8,8 +8,8 @@ | 架构 | 种子内核 | QEMU 启动 | rootfs | 自编译 | 耗时 | 备注 | |------|---------|----------|--------|--------|------|------| -| riscv64 | ✅ | ✅ | 已就绪 | ✅ | ~100 min | TCG 模拟，SMP=1, 12GB RAM | -| x86_64 | ✅ | ✅ (KVM) | 已就绪 | ✅ | 6m53s | KVM + SMP=1, 12GB RAM, 301 crates | +| riscv64 | ✅ | ✅ | 已就绪 | ✅ | ~100 min | TCG 模拟，SMP=1 | +| x86_64 | ✅ | ✅ (KVM) | 已就绪 | ✅ | 6m53s | KVM + SMP=1, 301 crates | ### 测试链路 @@ -18,11 +18,11 @@ Host (Linux) └─ scripts/self-compile.sh --arch ├─ cargo xtask starry build (种子内核) ├─ loopback mount → inject files (脚本/配置注入) - └─ expect + QEMU (-m 12G) + └─ expect + QEMU (-m 8G) └─ StarryOS 内核 └─ Debian rootfs (ext4) └─ /usr/bin/self-compile-inner.sh - ├─ mount tmpfs (12G) + ├─ mount tmpfs (8G) ├─ filter-workspace.sh (架构过滤) └─ cargo build -p starryos --offline ``` @@ -34,23 +34,22 @@ Host (Linux) | #797 | 信号传递修复：`interrupt_waker.wake()` 唤醒被 `future_blocked_resched` 移出运行队列的任务 | 无此修复，cargo 子进程（build script）挂起，父进程 waitpid 永远阻塞 | | #1007 | 页回收：内存压力下驱逐干净文件支持页面，`try_page_reclaim()` 最多重试 4 次 | 无此修复，编译 `syn` 时 OOM panic（大量源码/产物占满文件缓存） | | #971 | rsext4 clock LRU 缓存（4 入口/16 KiB），减少 virtio 块设备 round-trip | 加速离线 registry 读取，将依赖解析从分钟级降到秒级 | -| #1076 | `scripts/self-compile.sh` 升级：tmpfs、rootfs 清理、jemalloc 过滤、QEMU 12G | host 端自助编译流程依赖此 PR 获得 12G 内存和 tmpfs 支持 | ## 共通阻塞点（riscv64 + x86_64） ### 1. 内存检测仅识别 512MB -**现象**: QEMU `-m 12G` 但内核只识别 ~510MB。 +**现象**: QEMU `-m 8G` 但内核只识别 ~510MB。 **根因**: `axplat-riscv64-qemu-virt` 的 `axconfig.toml` 硬编码 `phys-memory-size = 0x2000_0000`。 -**修复**: 改为 `phys-memory-size = "0x2_0000_0000"` (8GB)。x86_64 使用 `axplat-dyn` + `somehal::mem::memory_map()` 动态检测，无此问题。**注**: 实际测试中 8GB 存在 OOM 风险，建议使用 `0x3_0000_0000` (12GB)。 +**修复**: 改为 `phys-memory-size = "0x2_0000_0000"` (8GB)。x86_64 使用 `axplat-dyn` + `somehal::mem::memory_map()` 动态检测，无此问题。 **注**: PR #987 重构了 ax-alloc，移除了旧 bitmap 页分配器（及 `page-alloc-*` 特性），改用 TLSF/buddy-slab。TLSF 无硬编码容量限制，不再需要 `page-alloc-64g` passthrough。 ### 2. TMPFS 挂载失败 -**现象**: `mount -t tmpfs -o size=12G tmpfs /tmp` 失败。 +**现象**: `mount -t tmpfs -o size=8G tmpfs /tmp` 失败。 **根因**: mount(8) 优先使用新版 mount API（`fsopen`/`fsconfig`/`fsmount`）。StarryOS 将 `fsopen` 实现为 `sys_dummy_fd`（返回伪 fd），mount(8) 误以为挂载成功但后续操作失败，不会回退到传统 `mount(2)`。 @@ -64,9 +63,9 @@ Sysno::fsopen | Sysno::fspick | Sysno::open_tree => Err(AxError::Unsupported), **现象**: 所有 crate 编译通过，但最终链接失败: `undefined symbol: _ex_table_end`。 -**根因**: 自编译环境中 `.cargo/config.toml` 未传递 `-Tlinker.x`。`linker.ld` 使用 `INSERT AFTER .data;` 期望 `linker.x` 先定义 `.data` 段（含 `_ex_table_end`），但缺少 linker.x 时符号未定义。 +**根因**: 自编译环境中 `.cargo/config.toml` 未传递 `-Tlinker.x`。`ext_linker.ld` 使用 `INSERT AFTER .data;` 期望 `linker.x` 先定义 `.data` 段（含 `_ex_table_end`），但缺少 linker.x 时符号未定义。 -**修复** (`os/StarryOS/starryos/linker.ld`): +**修复** (`os/StarryOS/starryos/ext_linker.ld`): ```ld PROVIDE(_ex_table_start = 0); PROVIDE(_ex_table_end = 0); @@ -190,37 +189,28 @@ sudo ./scripts/prepare-selfhost-rootfs.sh --arch x86_64 ## 测试配置 -测试用例位于 `apps/starry/selfhost/`，通过 Starry app 系统运行。 +测试用例位于 `test-suit/starryos/selfhost-manual/`，**不在** `normal/` 目录下，不参与标准 CI。 ``` -apps/starry/selfhost/ -├── build-riscv64gc-unknown-none-elf.toml # 构建配置 -├── selfhost-full-kernel/ # 完整编译测试（timeout=7200s） +test-suit/starryos/selfhost-manual/ +├── build-riscv64gc-unknown-none-elf.toml # 构建配置 +├── selfhost-full-kernel/ # 完整编译测试（timeout=7200s） │ ├── qemu-riscv64.toml -│ └── sh/self-compile.sh # Guest 内执行的编译脚本 -└── test-selfhost-check/ # 快速工具检查（timeout=120s） +│ └── sh/self-compile.sh # Guest 内执行的编译脚本 +└── test-selfhost-check/ # 快速工具检查（timeout=120s） └── qemu-riscv64.toml ``` **CI 不运行的原因**: Debian rootfs 镜像（~8-12GB）未上传到 tgosimages release，CI 容器无法下载。 -**手动运行（app 方式，使用 apps/starry/selfhost/ 中的 12G 配置）**: +**手动运行**: ```bash -# 完整自编译 -cargo xtask starry app qemu --arch riscv64 -t selfhost/selfhost-full-kernel - -# 快速工具检查 -cargo xtask starry app qemu --arch riscv64 -t selfhost/test-selfhost-check -``` - -**手动运行（host 脚本方式，需先合入 PR #1076 以升级到 12G）**: -```bash -./scripts/self-compile.sh --arch riscv64 +cargo xtask starry test qemu --arch riscv64 --test-suite test-suit/starryos/selfhost-manual -c selfhost-full-kernel ``` ## 已知限制 -1. **`phys-memory-size` 硬编码**: 动态 RAM 检测因启动阶段地址空间不一致无法实现。自编译需要 `-m 12G` + `axconfig_overrides = ["plat.phys-memory-size=0x3_0000_0000"]`。8GB 在实测中出现 OOM，不建议使用。 +1. **`phys-memory-size` 硬编码 8GB**: 动态 RAM 检测因启动阶段地址空间不一致无法实现。标准 CI 测试使用默认内存配置，自编译需要 `-m 8G`。 2. **自编译测试不在标准 CI 中运行**: 需要 8-12GB rootfs 镜像，仅支持本地手动测试。 3. **SMP > 1 未验证**: ext4 `SpinNoPreempt` 死锁 workaround 为 SMP=1。x86_64 通过 KVM 加速弥补单核性能。 4. **aarch64 引导已验证**: rootfs 准备 + 种子内核引导 + shell 可用均通过，完整编译因 TCG 模拟性能限制（预计 4-8h）未运行。需 `plat_dyn=true` + PIE 目标（`--config test-suit/starryos/normal/qemu-smp1/build-aarch64-unknown-none-softfloat.toml`）。 @@ -228,7 +218,7 @@ cargo xtask starry app qemu --arch riscv64 -t selfhost/test-selfhost-check ## 环境要求 -- **QEMU**: riscv64 (TCG) / x86_64 (KVM) / aarch64 (TCG), `-m 12G` +- **QEMU**: riscv64 (TCG) / x86_64 (KVM) / aarch64 (TCG), `-m 8G` - **内核**: StarryOS (dev 分支) - **根文件系统**: Debian (per-arch), ext4, rustc nightly-2026-04-27 - **Host 依赖**: `qemu-system-*`, `expect`, `sudo`（免密）, `systemd-nspawn` From 51fc7b357891dcdb234370accd7a42b539c73598 Mon Sep 17 00:00:00 2001 From: Tempest Date: Wed, 3 Jun 2026 09:51:37 +0800 Subject: [PATCH 09/20] fix(rsext4): use modify_new in create_lost_found_directory to persist data The function used create_new + direct modification of a cloned CachedBlock, which meant directory block data (dot/dotdot entries, checksum tail) was never written back to the cache. When flushed to disk, the lost+found directory block would contain uninitialized (zeroed) data. Switch to modify_new closure pattern, matching create_root_directory_entry. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/dir/bootstrap.rs | 79 ++++++++++++-------------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/components/rsext4/src/dir/bootstrap.rs b/components/rsext4/src/dir/bootstrap.rs index 0f8edec594..d4950e9f3f 100644 --- a/components/rsext4/src/dir/bootstrap.rs +++ b/components/rsext4/src/dir/bootstrap.rs @@ -132,57 +132,52 @@ pub fn create_lost_found_directory( let data_block = fs.alloc_block(block_dev)?; let lost_gen = fs.get_inode_by_num(block_dev, lost_ino)?.i_generation; - { - // Format the first block of the new directory, including the checksum tail. - let mut cached = fs.datablock_cache.create_new(block_dev, data_block)?; - let data = &mut cached.data; - - let dot_name = b"."; - let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); - let dot = Ext4DirEntry2::new( - lost_ino.raw(), - dot_rec_len, - Ext4DirEntry2::EXT4_FT_DIR, - dot_name, - ); - - let dotdot_name = b".."; - let dotdot_rec_len = if has_checksum { - (BLOCK_SIZE as u16) - .saturating_sub(dot_rec_len) - .saturating_sub(Ext4DirEntryTail::TAIL_LEN) - } else { - (BLOCK_SIZE as u16).saturating_sub(dot_rec_len) - }; - let dotdot = Ext4DirEntry2::new( - root_inode_num.raw(), - dotdot_rec_len, - Ext4DirEntry2::EXT4_FT_DIR, - dotdot_name, - ); - - { + // Format the first block of the new directory through modify_new so + // mutations are persisted in the cache entry rather than on a clone. + fs.datablock_cache + .modify_new(block_dev, data_block, |data| { + let dot_name = b"."; + let dot_rec_len = Ext4DirEntry2::entry_len(dot_name.len() as u8); + let dot = Ext4DirEntry2::new( + lost_ino.raw(), + dot_rec_len, + Ext4DirEntry2::EXT4_FT_DIR, + dot_name, + ); + + let dotdot_name = b".."; + let dotdot_rec_len = if has_checksum { + (BLOCK_SIZE as u16) + .saturating_sub(dot_rec_len) + .saturating_sub(Ext4DirEntryTail::TAIL_LEN) + } else { + (BLOCK_SIZE as u16).saturating_sub(dot_rec_len) + }; + let dotdot = Ext4DirEntry2::new( + root_inode_num.raw(), + dotdot_rec_len, + Ext4DirEntry2::EXT4_FT_DIR, + dotdot_name, + ); + dot.to_disk_bytes(&mut data[0..8]); let name_len = dot.name_len as usize; data[8..8 + name_len].copy_from_slice(&dot.name[..name_len]); - } - { let offset = dot_rec_len as usize; dotdot.to_disk_bytes(&mut data[offset..offset + 8]); let name_len = dotdot.name_len as usize; data[offset + 8..offset + 8 + name_len].copy_from_slice(&dotdot.name[..name_len]); - } - if has_checksum { - let tail = Ext4DirEntryTail::new(); - let tail_offset = BLOCK_SIZE - Ext4DirEntryTail::TAIL_LEN as usize; - tail.to_disk_bytes( - &mut data[tail_offset..tail_offset + Ext4DirEntryTail::TAIL_LEN as usize], - ); - update_ext4_dirblock_csum32(&fs.superblock, lost_ino.raw(), lost_gen, data); - } - } + if has_checksum { + let tail = Ext4DirEntryTail::new(); + let tail_offset = BLOCK_SIZE - Ext4DirEntryTail::TAIL_LEN as usize; + tail.to_disk_bytes( + &mut data[tail_offset..tail_offset + Ext4DirEntryTail::TAIL_LEN as usize], + ); + update_ext4_dirblock_csum32(&fs.superblock, lost_ino.raw(), lost_gen, data); + } + })?; let (lf_group, _idx) = fs.inode_allocator.global_to_group(lost_ino)?; let dir_mode = Ext4Inode::S_IFDIR | 0o755; From bcea3701a431496751a98f911834df87f8a9accb Mon Sep 17 00:00:00 2001 From: Tempest Date: Wed, 3 Jun 2026 12:54:53 +0800 Subject: [PATCH 10/20] test(rsext4): restore deleted test_invalidate and create_new_respects_lru_limit ZR233 review found that data_block.rs deleted two valuable tests during the cache refactoring without equivalent coverage. Rewrite them using the new &self + SpinMutex public API: - test_invalidate: uses stats() and invalidate() instead of cache.cache.len() - create_new_respects_lru_limit: uses stats().total_entries to verify LRU eviction respects max_entries=2 when inserting 4 blocks Both tests pass with the new cache API. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/data_block.rs | 37 +++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index ff1eac6450..285bf09800 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -591,5 +591,42 @@ mod tests { assert_eq!(block.block_num, AbsoluteBN::new(100)); assert_eq!(block.data.len(), BLOCK_SIZE); assert!(block.dirty); + + let stats = cache.stats(); + assert_eq!(stats.total_entries, 1); + assert_eq!(stats.dirty_entries, 1); + } + + #[test] + fn test_invalidate() { + let cache = DataBlockCache::new(8, BLOCK_SIZE); + + let device = TestBlockDevice::new(1024); + let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, device, false); + + cache + .create_new(&mut jbd2_dev, AbsoluteBN::new(100)) + .expect("create new block"); + assert_eq!(cache.stats().total_entries, 1); + + cache.invalidate(AbsoluteBN::new(100)); + assert_eq!(cache.stats().total_entries, 0); + } + + #[test] + fn create_new_respects_lru_limit() { + let cache = DataBlockCache::new(2, BLOCK_SIZE); + let device = TestBlockDevice::new(1024); + let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, device, false); + + for block in 10..14 { + cache + .create_new(&mut jbd2_dev, AbsoluteBN::new(block)) + .expect("create new block"); + } + + let stats = cache.stats(); + assert_eq!(stats.total_entries, 2); + assert_eq!(stats.max_entries, 2); } } From 2b252e4bc471abfe46c5c154b353eb71772583ad Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 12:56:42 +0800 Subject: [PATCH 11/20] =?UTF-8?q?fix(rsext4):=20LRU=20eviction=20race=20?= =?UTF-8?q?=E2=80=94=20validate=20generation=20before=20removing=20victim?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a per-entry generation counter to CachedBlock, CachedBitmap, and CachedInode. The counter is bumped on every access (get_or_load, get_or_load_mut, get_mut, mark_dirty, modify). snapshot_lru() now captures the generation alongside the victim key. After reacquiring the spinlock post-I/O, the eviction path checks whether the victim's generation still matches the snapshot. If another thread accessed the victim while the lock was released, its generation will differ — the stale eviction is skipped. This prevents lost dirty writes when LRU victims are modified during the drop-lock I/O window. All three caches (DataBlockCache, BitmapCache, InodeCache) and create_new (which evicts both an existing incarnation and an LRU slot) are covered. ZR233's repro scenario (DataBlockCache max_entries=1, thread A misses block 2, thread B modifies block 1, thread A deletes block 1 with stale snapshot) is prevented by this fix. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 50 +++++++++++++---- components/rsext4/src/cache/data_block.rs | 63 +++++++++++++++++----- components/rsext4/src/cache/inode_table.rs | 48 +++++++++++++---- 3 files changed, 127 insertions(+), 34 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 3b28433401..8e02b38477 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -14,8 +14,8 @@ use crate::{ }; /// Snapshot type for lock-free LRU eviction. -/// `(lru_key, optional dirty data: (block_num, data))` -type BitmapLruSnapshot = Option<(CacheKey, Option<(AbsoluteBN, Vec)>)>; +/// `(lru_key, generation, optional dirty data: (block_num, data))` +type BitmapLruSnapshot = Option<(CacheKey, u64, Option<(AbsoluteBN, Vec)>)>; /// Type of bitmap stored in the cache. #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)] @@ -60,6 +60,9 @@ pub struct CachedBitmap { pub block_num: AbsoluteBN, /// Access timestamp for LRU eviction. pub last_access: u64, + /// Generation counter — bumped on every access, used to validate + /// stale LRU snapshots before eviction. + pub generation: u64, } impl CachedBitmap { @@ -69,6 +72,7 @@ impl CachedBitmap { dirty: false, block_num, last_access: 0, + generation: 0, } } @@ -132,7 +136,7 @@ impl BitmapCache { drop(inner); // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, Some((lru_bn, ref lru_data)))) = evict_info { + if let Some((_lru_key, _, Some((lru_bn, ref lru_data)))) = evict_info { Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; } @@ -142,7 +146,14 @@ impl BitmapCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|bitmap| bitmap.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -156,6 +167,7 @@ impl BitmapCache { inner.access_counter = new_counter; if let Some(bitmap) = inner.cache.get_mut(&key) { bitmap.last_access = new_counter; + bitmap.generation += 1; } inner.cache.get(&key).cloned().ok_or(Ext4Error::corrupted()) @@ -181,7 +193,7 @@ impl BitmapCache { drop(inner); // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, Some((lru_bn, ref lru_data)))) = evict_info { + if let Some((_lru_key, _, Some((lru_bn, ref lru_data)))) = evict_info { Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; } @@ -191,7 +203,14 @@ impl BitmapCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|bitmap| bitmap.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -207,6 +226,7 @@ impl BitmapCache { inner.access_counter = new_counter; if let Some(bitmap) = inner.cache.get_mut(&key) { bitmap.last_access = new_counter; + bitmap.generation += 1; } Ok(()) } @@ -223,14 +243,17 @@ impl BitmapCache { inner.access_counter = new_counter; inner.cache.get_mut(key).map(|bitmap| { bitmap.last_access = new_counter; + bitmap.generation += 1; bitmap.clone() }) } /// Marks a cached bitmap dirty. pub fn mark_dirty(&self, key: &CacheKey) { - if let Some(bitmap) = self.inner.lock().cache.get_mut(key) { + let mut inner = self.inner.lock(); + if let Some(bitmap) = inner.cache.get_mut(key) { bitmap.mark_dirty(); + bitmap.generation += 1; } } @@ -257,6 +280,7 @@ impl BitmapCache { f(&mut bitmap.data); bitmap.mark_dirty(); + bitmap.generation += 1; if !USE_MULTILEVEL_CACHE { let data = bitmap.data.clone(); @@ -266,6 +290,7 @@ impl BitmapCache { inner = self.inner.lock(); if let Some(bitmap) = inner.cache.get_mut(&key) { bitmap.dirty = false; + bitmap.generation += 1; } } @@ -343,9 +368,10 @@ pub struct CacheStats { impl BitmapCacheInner { /// Snapshots the LRU bitmap for lock-free eviction. /// - /// Returns `Some(lru_key, Some(data))` when dirty, `Some(lru_key, None)` when - /// clean, `None` when the cache is empty. The caller must do the I/O - /// *without* holding the spinlock, then re-lock and remove `lru_key`. + /// Returns `(lru_key, generation, dirty_info)` where `generation` is the + /// entry's generation at snapshot time. The caller must do the I/O + /// *without* holding the spinlock, then re-lock, verify the entry's + /// generation still matches, and only then remove it. fn snapshot_lru(&self) -> BitmapLruSnapshot { let lru_key = self .cache @@ -353,6 +379,8 @@ impl BitmapCacheInner { .min_by_key(|(_, bitmap)| bitmap.last_access) .map(|(key, _)| *key)?; + let lru_gen = self.cache.get(&lru_key).map(|bitmap| bitmap.generation)?; + let dirty_info = self.cache.get(&lru_key).and_then(|bitmap| { if bitmap.dirty { Some((bitmap.block_num, bitmap.data.clone())) @@ -361,7 +389,7 @@ impl BitmapCacheInner { } }); - Some((lru_key, dirty_info)) + Some((lru_key, lru_gen, dirty_info)) } fn do_evict( diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index 285bf09800..87467f4bed 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -19,6 +19,9 @@ pub struct CachedBlock { pub block_num: AbsoluteBN, /// Access timestamp used for LRU eviction. pub last_access: u64, + /// Generation counter — bumped on every access, used to validate + /// stale LRU snapshots before eviction. + pub generation: u64, } impl CachedBlock { @@ -28,6 +31,7 @@ impl CachedBlock { dirty: false, block_num, last_access: 0, + generation: 0, } } @@ -108,7 +112,7 @@ impl DataBlockCache { drop(inner); // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, Some(ref lru_data))) = evict_info { + if let Some((_lru_key, _, Some(ref lru_data))) = evict_info { Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; } @@ -117,7 +121,14 @@ impl DataBlockCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -127,11 +138,12 @@ impl DataBlockCache { .or_insert_with(|| CachedBlock::new(data, block_num)); } - // Refresh the LRU timestamp. + // Refresh the LRU timestamp and bump the generation. let new_counter = inner.access_counter + 1; inner.access_counter = new_counter; if let Some(cached) = inner.cache.get_mut(&block_num) { cached.last_access = new_counter; + cached.generation += 1; } inner @@ -160,7 +172,7 @@ impl DataBlockCache { drop(inner); // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, Some(ref lru_data))) = evict_info { + if let Some((_lru_key, _, Some(ref lru_data))) = evict_info { Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; } @@ -169,7 +181,14 @@ impl DataBlockCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -185,6 +204,7 @@ impl DataBlockCache { inner.access_counter = new_counter; if let Some(cached) = inner.cache.get_mut(&block_num) { cached.last_access = new_counter; + cached.generation += 1; } Ok(()) } @@ -201,6 +221,7 @@ impl DataBlockCache { inner.access_counter = new_counter; inner.cache.get_mut(&block_num).map(|cached| { cached.last_access = new_counter; + cached.generation += 1; cached.clone() }) } @@ -230,17 +251,26 @@ impl DataBlockCache { if let Some(ref data) = evict_existing { Self::write_block_static(block_dev, block_num, data, self.block_size)?; } - if let Some((lru_key, Some(ref lru_data))) = evict_lru_info { + if let Some((lru_key, _, Some(ref lru_data))) = evict_lru_info { Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; } // Phase 3: reacquire the lock and apply evictions + insertion. inner = self.inner.lock(); + // Evict the pre-existing incarnation of the same block number + // unconditionally — it is being replaced. if evict_existing.is_some() { inner.cache.remove(&block_num); } - if let Some((lru_key, _)) = evict_lru_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_lru_info + && inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -265,6 +295,7 @@ impl DataBlockCache { let mut inner = self.inner.lock(); if let Some(cached) = inner.cache.get_mut(&block_num) { cached.mark_dirty(); + cached.generation += 1; } } @@ -288,6 +319,7 @@ impl DataBlockCache { .ok_or(Ext4Error::corrupted())?; f(&mut cached.data); cached.mark_dirty(); + cached.generation += 1; if !USE_MULTILEVEL_CACHE { let data = cached.data.clone(); @@ -298,6 +330,7 @@ impl DataBlockCache { inner = self.inner.lock(); if let Some(cached) = inner.cache.get_mut(&block_num) { cached.dirty = false; + cached.generation += 1; } } Ok(()) @@ -394,16 +427,22 @@ pub struct DataBlockCacheStats { impl DataBlockCacheInner { /// Snapshots the LRU data block for lock-free eviction. /// - /// Returns `Some(lru_key, Some(data))` when dirty, `Some(lru_key, None)` when - /// clean, `None` when the cache is empty. The caller must do the I/O - /// *without* holding the spinlock, then re-lock and remove `lru_key`. - fn snapshot_lru(&self) -> Option<(AbsoluteBN, Option>)> { + /// Returns `Some((lru_key, generation, dirty_data))` where `generation` is + /// the entry's generation at snapshot time. The caller must do the I/O + /// *without* holding the spinlock, then re-lock, verify that the entry's + /// generation still matches, and only then remove it. + /// A generation mismatch means another thread accessed or modified the + /// victim while we held no lock — in that case the victim must NOT be + /// removed (temporarily exceeding `max_entries` is harmless). + fn snapshot_lru(&self) -> Option<(AbsoluteBN, u64, Option>)> { let lru_key = self .cache .iter() .min_by_key(|(_, cached)| cached.last_access) .map(|(key, _)| *key)?; + let lru_gen = self.cache.get(&lru_key).map(|cached| cached.generation)?; + let dirty_data = self.cache.get(&lru_key).and_then(|cached| { if cached.dirty { Some(cached.data.clone()) @@ -412,7 +451,7 @@ impl DataBlockCacheInner { } }); - Some((lru_key, dirty_data)) + Some((lru_key, lru_gen, dirty_data)) } /// Snapshots a single block for lock-free eviction. diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index 252dc492ab..70e18276c9 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -14,8 +14,8 @@ use crate::{ }; /// Snapshot type for lock-free LRU eviction. -/// `(lru_key, optional dirty data: (block_num, offset, data))` -type InodeLruSnapshot = Option<(InodeNumber, Option<(AbsoluteBN, usize, Vec)>)>; +/// `(lru_key, generation, optional dirty data: (block_num, offset, data))` +type InodeLruSnapshot = Option<(InodeNumber, u64, Option<(AbsoluteBN, usize, Vec)>)>; /// Cache key for one global inode number. pub type InodeCacheKey = InodeNumber; @@ -35,6 +35,9 @@ pub struct CachedInode { pub inode_num: InodeNumber, /// Access timestamp used for LRU eviction. pub last_access: u64, + /// Generation counter — bumped on every access, used to validate + /// stale LRU snapshots before eviction. + pub generation: u64, } impl CachedInode { @@ -51,6 +54,7 @@ impl CachedInode { offset_in_block: offset, inode_num, last_access: 0, + generation: 0, } } @@ -188,7 +192,7 @@ impl InodeCache { // Phase 2: do I/O without holding the spinlock so other cores can // make progress on cache hits. - if let Some((_lru_key, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { + if let Some((_lru_key, _, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; } @@ -197,7 +201,14 @@ impl InodeCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -214,6 +225,7 @@ impl InodeCache { inner.access_counter = new_counter; if let Some(cached) = inner.cache.get_mut(&inode_num) { cached.last_access = new_counter; + cached.generation += 1; } inner @@ -245,7 +257,7 @@ impl InodeCache { drop(inner); // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { + if let Some((_lru_key, _, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; } @@ -254,7 +266,14 @@ impl InodeCache { // Phase 3: reacquire the lock and apply the eviction + insertion. inner = self.inner.lock(); - if let Some((lru_key, _)) = evict_info { + // Only evict the LRU victim if no other thread accessed or + // modified it while we held no lock (generation unchanged). + if let Some((lru_key, lru_gen, _)) = evict_info + && inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) + { inner.cache.remove(&lru_key); } @@ -271,6 +290,7 @@ impl InodeCache { inner.access_counter = new_counter; if let Some(cached) = inner.cache.get_mut(&inode_num) { cached.last_access = new_counter; + cached.generation += 1; } Ok(()) @@ -289,6 +309,7 @@ impl InodeCache { // Counter updated outside the get_mut borrow scope to avoid conflicts. inner.cache.get_mut(&inode_num).map(|cached| { cached.last_access = new_counter; + cached.generation += 1; cached.clone() }) } @@ -298,6 +319,7 @@ impl InodeCache { let mut inner = self.inner.lock(); if let Some(cached) = inner.cache.get_mut(&inode_num) { cached.mark_dirty(); + cached.generation += 1; } } @@ -326,6 +348,7 @@ impl InodeCache { .ok_or(Ext4Error::corrupted())?; f(&mut cached.inode); cached.mark_dirty(); + cached.generation += 1; if !USE_MULTILEVEL_CACHE { // Drop lock during synchronous I/O @@ -340,6 +363,7 @@ impl InodeCache { inner = self.inner.lock(); if let Some(cached) = inner.cache.get_mut(&inode_num) { cached.dirty = false; + cached.generation += 1; } } Ok(()) @@ -432,10 +456,10 @@ pub struct InodeCacheStats { impl InodeCacheInner { /// Snapshots the LRU entry for lock-free eviction. /// - /// Returns `(lru_key, dirty_write_info)` where `dirty_write_info` is - /// `Some((block_num, offset, data))` when the entry is dirty and must be - /// flushed to disk before eviction. The caller must do the I/O - /// *without* holding the spinlock, then re-lock and remove `lru_key`. + /// Returns `(lru_key, generation, dirty_write_info)` where `generation` + /// is the entry's generation at snapshot time. The caller must do the + /// I/O *without* holding the spinlock, then re-lock, verify the entry's + /// generation still matches, and only then remove it. fn snapshot_lru(&self) -> InodeLruSnapshot { let lru_key = self .cache @@ -443,6 +467,8 @@ impl InodeCacheInner { .min_by_key(|(_, cached)| cached.last_access) .map(|(key, _)| *key)?; + let lru_gen = self.cache.get(&lru_key).map(|cached| cached.generation)?; + let dirty_info = self.cache.get(&lru_key).and_then(|cached| { if cached.dirty { let mut buf = alloc::vec![0u8; self.inode_size]; @@ -453,7 +479,7 @@ impl InodeCacheInner { } }); - Some((lru_key, dirty_info)) + Some((lru_key, lru_gen, dirty_info)) } fn do_evict( From d0d6dcd92f5156eb94ce7ca0158b8623242be8aa Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 13:13:06 +0800 Subject: [PATCH 12/20] fix(rsext4): propagate or log errors from datablock_cache.modify() callers - insert.rs: log modify() I/O errors instead of silently swallowing them; the fallback to a new directory block still works but the error is now visible in logs. - rename.rs: log errors from set_inode_links_count() and '..' entry rewrite; these are best-effort updates after the rename is committed but failures indicate latent directory corruption. - delete.rs: change try_remove_dentry_in_block to return Ext4Result instead of silently swallowing the modify() I/O error. Also add buffer bounds checks to prevent panics on corrupted input: - data_block.rs write_block_static: use core::cmp::min(data.len(), block_size) - bitmap.rs write_bitmap_static: same min-guard - inode_table.rs write_inode_bytes_static: checked_add + bounds check on offset + data.len() Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 6 ++++-- components/rsext4/src/cache/data_block.rs | 3 ++- components/rsext4/src/cache/inode_table.rs | 8 +++++++- components/rsext4/src/dir/insert.rs | 11 ++++++++--- components/rsext4/src/file/delete.rs | 10 +++++----- components/rsext4/src/file/rename.rs | 23 ++++++++++++++++++---- 6 files changed, 45 insertions(+), 16 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 8e02b38477..81f500598f 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -347,9 +347,11 @@ impl BitmapCache { block_num: AbsoluteBN, data: &[u8], ) -> Ext4Result<()> { - let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; + let block_size = crate::config::BLOCK_SIZE; + let mut buf = alloc::vec![0u8; block_size]; block_dev.read_blocks(&mut buf, block_num, 1)?; - buf[..data.len()].copy_from_slice(data); + let len = core::cmp::min(data.len(), block_size); + buf[..len].copy_from_slice(&data[..len]); block_dev.write_blocks(&buf, block_num, 1, true)?; Ok(()) } diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index 87467f4bed..e6e35a23b3 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -407,7 +407,8 @@ impl DataBlockCache { ) -> Ext4Result<()> { let mut buf = alloc::vec![0u8; block_size]; block_dev.read_blocks(&mut buf, block_num, 1)?; - buf[..data.len()].copy_from_slice(data); + let len = core::cmp::min(data.len(), block_size); + buf[..len].copy_from_slice(&data[..len]); block_dev.write_blocks(&buf, block_num, 1, false)?; Ok(()) } diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index 70e18276c9..2f4bab3a7d 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -437,7 +437,13 @@ impl InodeCache { ) -> Ext4Result<()> { let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; - buf[offset..offset + data.len()].copy_from_slice(data); + let end = offset + .checked_add(data.len()) + .ok_or(Ext4Error::corrupted())?; + if end > buf.len() { + return Err(Ext4Error::corrupted()); + } + buf[offset..end].copy_from_slice(data); block_dev.write_blocks(&buf, block_num, 1, true)?; // is_metadata: inode table blocks are filesystem metadata Ok(()) } diff --git a/components/rsext4/src/dir/insert.rs b/components/rsext4/src/dir/insert.rs index 197d6b0405..42bddeeb8c 100644 --- a/components/rsext4/src/dir/insert.rs +++ b/components/rsext4/src/dir/insert.rs @@ -1,6 +1,6 @@ //! Directory entry insertion helpers. -use log::error; +use log::{error, warn}; use crate::{ blockdev::*, bmalloc::InodeNumber, checksum::update_ext4_dirblock_csum32, config::*, @@ -61,7 +61,7 @@ pub fn insert_dir_entry( } }; - let _ = fs.datablock_cache.modify(device, phys, |data| { + if let Err(e) = fs.datablock_cache.modify(device, phys, |data| { if inserted { return; } @@ -143,7 +143,12 @@ pub fn insert_dir_entry( } offset = entry_end; } - }); + }) { + warn!( + "insert_dir_entry: modify block {phys} for parent_ino={parent_ino_num} \ + name={child_name:?} failed: {e:?}, trying next block" + ); + } } if inserted { diff --git a/components/rsext4/src/file/delete.rs b/components/rsext4/src/file/delete.rs index f2d9396403..2e9f0dd99f 100644 --- a/components/rsext4/src/file/delete.rs +++ b/components/rsext4/src/file/delete.rs @@ -208,14 +208,14 @@ fn try_remove_dentry_in_block( parent_inode: &Ext4Inode, phys: AbsoluteBN, name_bytes: &[u8], -) -> bool { +) -> Ext4Result { let superblock = &fs.superblock; let mut removed = false; - let _ = fs.datablock_cache.modify(block_dev, phys, |data| { + fs.datablock_cache.modify(block_dev, phys, |data| { removed = remove_dentry_in_dir_block(superblock, parent_ino_num, parent_inode, data, name_bytes); - }); - removed + })?; + Ok(removed) } fn parent_dir_data_blocks( @@ -303,7 +303,7 @@ pub(crate) fn remove_named_entry_at( phys: AbsoluteBN, name_bytes: &[u8], ) -> Ext4Result<()> { - if try_remove_dentry_in_block(fs, block_dev, parent_ino, parent_inode, phys, name_bytes) { + if try_remove_dentry_in_block(fs, block_dev, parent_ino, parent_inode, phys, name_bytes)? { Ok(()) } else { Err(Ext4Error::not_found()) diff --git a/components/rsext4/src/file/rename.rs b/components/rsext4/src/file/rename.rs index 547a48aecb..0292d04bdd 100644 --- a/components/rsext4/src/file/rename.rs +++ b/components/rsext4/src/file/rename.rs @@ -244,11 +244,21 @@ pub fn mv( if old_pino != new_pino { if let Ok(old_parent_inode) = fs.get_inode_by_num(block_dev, old_pino) { let new_links = old_parent_inode.i_links_count.saturating_sub(1); - let _ = fs.set_inode_links_count(block_dev, old_pino, new_links); + if let Err(e) = fs.set_inode_links_count(block_dev, old_pino, new_links) { + warn!( + "mv set_inode_links_count failed for old_parent={old_pino}: {e:?}, \ + continuing with rename" + ); + } } if let Ok(new_parent_inode) = fs.get_inode_by_num(block_dev, new_pino) { let new_links = new_parent_inode.i_links_count.saturating_add(1); - let _ = fs.set_inode_links_count(block_dev, new_pino, new_links); + if let Err(e) = fs.set_inode_links_count(block_dev, new_pino, new_links) { + warn!( + "mv set_inode_links_count failed for new_parent={new_pino}: {e:?}, \ + continuing with rename" + ); + } } // Rewrite the `..` entry inside the moved directory's first block. @@ -259,7 +269,7 @@ pub fn mv( return Err(Ext4Error::corrupted()); } }; - let _ = fs.datablock_cache.modify(block_dev, first_blk, |data| { + if let Err(e) = fs.datablock_cache.modify(block_dev, first_blk, |data| { let block_bytes = BLOCK_SIZE; if block_bytes < 24 { return; @@ -284,7 +294,12 @@ pub fn mv( moved_inode.i_generation, data, ); - }); + }) { + error!( + "mv rewrite '..' entry failed for moved dir ino={src_ino} \ + first_blk={first_blk}: {e:?} — directory has stale parent pointer" + ); + } let _ = fs.touch_inode_ctime_for_link_change(block_dev, src_ino); } } From 27c9786cd0a8db4114742d16342b476b470a884e Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 13:21:27 +0800 Subject: [PATCH 13/20] fix(rsext4): log error from touch_inode_ctime_for_link_change Replace the last remaining silent error swallow in rename.rs with warn! logging, consistent with the other best-effort metadata updates in the same function. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/file/rename.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/components/rsext4/src/file/rename.rs b/components/rsext4/src/file/rename.rs index 0292d04bdd..b0e828da2b 100644 --- a/components/rsext4/src/file/rename.rs +++ b/components/rsext4/src/file/rename.rs @@ -300,7 +300,9 @@ pub fn mv( first_blk={first_blk}: {e:?} — directory has stale parent pointer" ); } - let _ = fs.touch_inode_ctime_for_link_change(block_dev, src_ino); + if let Err(e) = fs.touch_inode_ctime_for_link_change(block_dev, src_ino) { + warn!("mv touch_inode_ctime failed for ino={src_ino}: {e:?}"); + } } } From 221051ebd16a34370d7cb02c3045b08c079282d5 Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 15:41:22 +0800 Subject: [PATCH 14/20] fix(rsext4): defer dirty victim writeback until after generation check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ZR233 review: the dirty data writeback in Phase 2 occurred BEFORE the generation check in Phase 3. If the check failed (another thread accessed the victim while the lock was released), stale snapshot data was already written to disk — potentially overwriting a newer version. Restructure all three caches to a 4-phase pattern: Phase 1: lock → snapshot (key, gen, dirty_data?) → unlock Phase 2: load new data from disk (no dirty writeback) Phase 3: lock → validate gen → if valid: remove victim, extract dirty for writeback → insert new entry → unlock Phase 4: write dirty data (only if gen check passed in phase 3) If gen check fails, stale dirty data is silently discarded. Covers: DataBlockCache (get_or_load, get_or_load_mut, create_new), BitmapCache (get_or_load, get_or_load_mut), InodeCache (get_or_load, get_or_load_mut). Also ensures clean LRU victims are properly evicted when the generation check passes (Previously the match arm required Some(lru_data), skipping eviction of clean entries even when the generation was valid). Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 86 +++++++++----- components/rsext4/src/cache/data_block.rs | 131 +++++++++++++-------- components/rsext4/src/cache/inode_table.rs | 87 ++++++++------ 3 files changed, 191 insertions(+), 113 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 81f500598f..549f524928 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -135,32 +135,43 @@ impl BitmapCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, _, Some((lru_bn, ref lru_data)))) = evict_info { - Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; - } - + // Phase 2: load the requested bitmap from disk (no dirty writeback + // yet — the victim snapshot may be stale). let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|bitmap| bitmap.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|bitmap| bitmap.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt + } + _ => None, + }; inner .cache .entry(key) .or_insert_with(|| CachedBitmap::new(buf, block_num)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_bn, ref lru_data)) = dirty_to_write { + Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; + } + + inner = self.inner.lock(); } let new_counter = inner.access_counter + 1; @@ -192,27 +203,28 @@ impl BitmapCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, _, Some((lru_bn, ref lru_data)))) = evict_info { - Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; - } - + // Phase 2: load the requested bitmap from disk (no dirty writeback + // yet — the victim snapshot may be stale). let mut buf = alloc::vec![0u8; crate::config::BLOCK_SIZE]; block_dev.read_blocks(&mut buf, block_num, 1)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|bitmap| bitmap.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|bitmap| bitmap.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt + } + _ => None, + }; // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. @@ -220,6 +232,16 @@ impl BitmapCache { .cache .entry(key) .or_insert_with(|| CachedBitmap::new(buf, block_num)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_bn, ref lru_data)) = dirty_to_write { + Self::write_bitmap_static(block_dev, lru_bn, lru_data)?; + } + + inner = self.inner.lock(); } let new_counter = inner.access_counter + 1; diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index e6e35a23b3..b1903d0fbb 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -111,31 +111,43 @@ impl DataBlockCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, _, Some(ref lru_data))) = evict_info { - Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; - } - + // Phase 2: load the requested block from disk (no dirty writeback + // yet — the victim snapshot may be stale). let data = self.load_block(block_dev, block_num)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|cached| cached.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt.map(|data| (lru_key, data)) + } + _ => None, + }; inner .cache .entry(block_num) .or_insert_with(|| CachedBlock::new(data, block_num)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_key, ref lru_data)) = dirty_to_write { + Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; + } + + // Reacquire for the LRU refresh below. + inner = self.inner.lock(); } // Refresh the LRU timestamp and bump the generation. @@ -171,26 +183,27 @@ impl DataBlockCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, _, Some(ref lru_data))) = evict_info { - Self::write_block_static(block_dev, _lru_key, lru_data, self.block_size)?; - } - + // Phase 2: load the requested block from disk (no dirty writeback + // yet — the victim snapshot may be stale). let data = self.load_block(block_dev, block_num)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|cached| cached.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt.map(|data| (lru_key, data)) + } + _ => None, + }; // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. @@ -198,6 +211,16 @@ impl DataBlockCache { .cache .entry(block_num) .or_insert_with(|| CachedBlock::new(data, block_num)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_key, ref lru_data)) = dirty_to_write { + Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; + } + + inner = self.inner.lock(); } let new_counter = inner.access_counter + 1; @@ -247,13 +270,12 @@ impl DataBlockCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. + // Phase 2: write dirty data for unconditional same-block eviction. + // The LRU victim's dirty data is NOT written here — it must pass + // the generation check first (see Phase 4). if let Some(ref data) = evict_existing { Self::write_block_static(block_dev, block_num, data, self.block_size)?; } - if let Some((lru_key, _, Some(ref lru_data))) = evict_lru_info { - Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; - } // Phase 3: reacquire the lock and apply evictions + insertion. inner = self.inner.lock(); @@ -263,16 +285,20 @@ impl DataBlockCache { if evict_existing.is_some() { inner.cache.remove(&block_num); } - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_lru_info - && inner - .cache - .get(&lru_key) - .is_some_and(|cached| cached.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + // Validate LRU victim generation; if valid, remove and schedule + // dirty writeback. If stale, discard the snapshot silently. + let lru_dirty_to_write = match evict_lru_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt.map(|data| (lru_key, data)) + } + _ => None, + }; let data = alloc::vec![0u8; inner.block_size]; let mut cached = CachedBlock::new(data, block_num); @@ -283,11 +309,20 @@ impl DataBlockCache { cached.last_access = new_counter; inner.cache.insert(block_num, cached); - inner + let result = inner .cache .get(&block_num) .cloned() - .ok_or(Ext4Error::corrupted()) + .ok_or(Ext4Error::corrupted()); + + drop(inner); + + // Phase 4: write LRU victim's dirty data AFTER generation check. + if let Some((lru_key, ref lru_data)) = lru_dirty_to_write { + Self::write_block_static(block_dev, lru_key, lru_data, self.block_size)?; + } + + result } /// Marks a cached data block dirty. diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index 2f4bab3a7d..b63057c09c 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -190,27 +190,27 @@ impl InodeCache { drop(inner); - // Phase 2: do I/O without holding the spinlock so other cores can - // make progress on cache hits. - if let Some((_lru_key, _, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { - Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; - } - + // Phase 2: load the requested inode from disk (no dirty writeback + // yet — the victim snapshot may be stale). let inode = self.load_inode(block_dev, block_num, offset)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|cached| cached.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt + } + _ => None, + }; // Use or_insert_with to avoid TOCTOU: another thread may have // inserted the same key while we had no lock. @@ -218,6 +218,16 @@ impl InodeCache { .cache .entry(inode_num) .or_insert_with(|| CachedInode::new(inode, inode_num, block_num, offset)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_bn, lru_off, ref lru_data)) = dirty_to_write { + Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; + } + + inner = self.inner.lock(); } // Refresh the LRU timestamp on every access. @@ -256,26 +266,27 @@ impl InodeCache { drop(inner); - // Phase 2: do I/O without holding the spinlock. - if let Some((_lru_key, _, Some((lru_bn, lru_off, ref lru_data)))) = evict_info { - Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; - } - + // Phase 2: load the requested inode from disk (no dirty writeback + // yet — the victim snapshot may be stale). let inode = self.load_inode(block_dev, block_num, offset)?; - // Phase 3: reacquire the lock and apply the eviction + insertion. + // Phase 3: reacquire the lock. Validate the victim generation. + // If valid, remove it and schedule dirty writeback for Phase 4. + // If stale, discard the snapshot without writing anything. inner = self.inner.lock(); - // Only evict the LRU victim if no other thread accessed or - // modified it while we held no lock (generation unchanged). - if let Some((lru_key, lru_gen, _)) = evict_info - && inner - .cache - .get(&lru_key) - .is_some_and(|cached| cached.generation == lru_gen) - { - inner.cache.remove(&lru_key); - } + let dirty_to_write = match evict_info { + Some((lru_key, lru_gen, dirty_opt)) + if inner + .cache + .get(&lru_key) + .is_some_and(|cached| cached.generation == lru_gen) => + { + inner.cache.remove(&lru_key); + dirty_opt + } + _ => None, + }; // Re-check after reacquiring: another thread may have inserted the // same key while we held no lock. @@ -283,6 +294,16 @@ impl InodeCache { .cache .entry(inode_num) .or_insert_with(|| CachedInode::new(inode, inode_num, block_num, offset)); + + drop(inner); + + // Phase 4: write the victim's dirty data to disk AFTER the + // generation check passed (outside the spinlock). + if let Some((lru_bn, lru_off, ref lru_data)) = dirty_to_write { + Self::write_inode_bytes_static(block_dev, lru_bn, lru_off, lru_data)?; + } + + inner = self.inner.lock(); } // Refresh the LRU timestamp before returning the mutable handle. From 647c8ef626efcea64e6c268944a85e307a7d7b4e Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 17:22:41 +0800 Subject: [PATCH 15/20] test(rsext4): add stale-victim gen-mismatch regression test for DataBlockCache Reproduces the TOCTOU race: Thread 1 snapshots LRU victim (gen=0, dirty) and drops the lock; Thread 2 concurrently accesses the victim via get_mut(), bumping its generation to 1; Thread 1 resumes, re-locks, and the generation mismatch prevents eviction. The test uses a Barrier-synchronised BlockDevice wrapper that blocks inside read_blocks, allowing the main thread to interleave a cache access between Phase 1 (snapshot) and Phase 3 (re-lock) of get_or_load. Added #[cfg(test)] extern crate std to lib.rs to enable std::sync and std::thread in test modules of this no_std crate. --- components/rsext4/src/cache/data_block.rs | 143 ++++++++++++++++++++++ components/rsext4/src/lib.rs | 3 + 2 files changed, 146 insertions(+) diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index b1903d0fbb..9de3585677 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -704,4 +704,147 @@ mod tests { assert_eq!(stats.total_entries, 2); assert_eq!(stats.max_entries, 2); } + + /// Regression test for the stale-victim race (TOCTOU on LRU eviction). + /// + /// Scenario: + /// 1. Cache full (max_entries=1) with block_A (gen=0, dirty). + /// 2. Thread 1 calls `get_or_load(block_B)` → snapshots (A, gen=0, dirty_data) + /// → drops the spinlock → enters I/O (blocked at barrier). + /// 3. Main thread calls `cache.get_mut(block_A)` → bumps gen to 1. + /// 4. Thread 1 resumes → re-locks → gen mismatch (0 ≠ 1) → skips eviction. + /// + /// Assertions: + /// - block_A is still cached (NOT evicted — new state preserved). + /// - block_A's generation is 1 (the concurrent access was recorded). + /// - block_B was loaded into cache. + /// - No stale dirty data was written for block_A (dirty_to_write is None). + #[test] + fn stale_victim_gen_mismatch_prevents_eviction() { + use std::sync::{Arc, Barrier}; + + const BLK_A: AbsoluteBN = AbsoluteBN::new(10); + const BLK_B: AbsoluteBN = AbsoluteBN::new(20); + + let cache = Arc::new(DataBlockCache::new(1, BLOCK_SIZE)); + + // ── Barrier-synchronised BlockDevice ────────────────────────────── + // The device blocks inside `read_blocks` so the test can interleave a + // cache access between Phase 1 (snapshot) and Phase 3 (re-lock) of + // `get_or_load`. + let inner_dev = TestBlockDevice::new(1024); + let enter_io = Arc::new(Barrier::new(2)); // "I'm in I/O, lock is dropped" + let leave_io = Arc::new(Barrier::new(2)); // "I'm done, main may continue" + + struct SyncDevice { + inner: D, + enter: Arc, + leave: Arc, + } + + impl BlockDevice for SyncDevice { + fn read( + &mut self, + buffer: &mut [u8], + block_id: AbsoluteBN, + count: u32, + ) -> Ext4Result<()> { + self.enter.wait(); // signal: cache lock is dropped, race window open + self.inner.read(buffer, block_id, count)?; + self.leave.wait(); // wait for main thread to finish its access + Ok(()) + } + + fn write( + &mut self, + buffer: &[u8], + block_id: AbsoluteBN, + count: u32, + ) -> Ext4Result<()> { + self.inner.write(buffer, block_id, count) + } + + fn open(&mut self) -> Ext4Result<()> { + self.inner.open() + } + + fn close(&mut self) -> Ext4Result<()> { + self.inner.close() + } + + fn total_blocks(&self) -> u64 { + self.inner.total_blocks() + } + + fn block_size(&self) -> u32 { + self.inner.block_size() + } + + fn current_time(&self) -> Ext4Result { + self.inner.current_time() + } + } + + let sync_dev = SyncDevice { + inner: inner_dev, + enter: enter_io.clone(), + leave: leave_io.clone(), + }; + let mut jbd2_dev = Jbd2Dev::initial_jbd2dev(0, sync_dev, false); + + // Step 1: fill the cache with block_A (dirty, generation 0). + cache + .create_new(&mut jbd2_dev, BLK_A) + .expect("create block A"); + assert!(cache.get(BLK_A).is_some(), "block_A must be in cache"); + assert_eq!(cache.get(BLK_A).unwrap().generation, 0); + + // Step 2: spawn a thread that calls get_or_load(block_B). + // Inside get_or_load the device will block at enter_io after the LRU + // snapshot is taken but before the spinlock is reacquired. + let cache2 = cache.clone(); + let handle = std::thread::spawn(move || { + cache2.get_or_load(&mut jbd2_dev, BLK_B) + }); + + // Step 3: wait for Thread 1 to enter I/O (lock dropped). + enter_io.wait(); + + // Step 4: concurrently access block_A, bumping its generation. + let cached_a = cache + .get_mut(BLK_A) + .expect("block_A should still be accessible"); + assert_eq!( + cached_a.generation, 1, + "generation bumped from 0 to 1 by concurrent access" + ); + + // Step 5: let Thread 1 continue (re-lock, gen check, insert). + leave_io.wait(); + + // Step 6: collect Thread 1's result. + let result = handle.join().expect("Thread 1 panicked"); + assert!(result.is_ok(), "get_or_load(block_B) must succeed"); + + // ── Assertions ─────────────────────────────────────────────────── + // block_A must still be present — generation mismatch prevented eviction. + let a = cache + .get(BLK_A) + .expect("block_A must NOT be evicted (gen mismatch prevented removal)"); + assert_eq!( + a.generation, 1, + "block_A generation should be 1 (bumped by concurrent get_mut)" + ); + + // block_B was loaded. + assert!( + cache.get(BLK_B).is_some(), + "block_B must be loaded into cache" + ); + + // Both entries coexist (temporarily exceeding max_entries — harmless). + let stats = cache.stats(); + assert_eq!(stats.total_entries, 2); + assert_eq!(stats.max_entries, 1); + } } diff --git a/components/rsext4/src/lib.rs b/components/rsext4/src/lib.rs index 0c9c50f72b..1a86c55d31 100644 --- a/components/rsext4/src/lib.rs +++ b/components/rsext4/src/lib.rs @@ -12,6 +12,9 @@ extern crate alloc; +#[cfg(test)] +extern crate std; + // Re-export shared configuration constants for external callers. // Re-export the most frequently used public APIs. pub use api::{lseek, open, read_at, write_at}; From 8b9c2fb0c7cb1cefef65e24489a35dd885c11a4f Mon Sep 17 00:00:00 2001 From: Tempest Date: Thu, 4 Jun 2026 18:03:02 +0800 Subject: [PATCH 16/20] =?UTF-8?q?style(rsext4):=20cargo=20fmt=20=E2=80=94?= =?UTF-8?q?=20inline=20single-expression=20closures=20in=20test?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- components/rsext4/src/cache/data_block.rs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index 9de3585677..cb313adb18 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -755,12 +755,7 @@ mod tests { Ok(()) } - fn write( - &mut self, - buffer: &[u8], - block_id: AbsoluteBN, - count: u32, - ) -> Ext4Result<()> { + fn write(&mut self, buffer: &[u8], block_id: AbsoluteBN, count: u32) -> Ext4Result<()> { self.inner.write(buffer, block_id, count) } @@ -803,9 +798,7 @@ mod tests { // Inside get_or_load the device will block at enter_io after the LRU // snapshot is taken but before the spinlock is reacquired. let cache2 = cache.clone(); - let handle = std::thread::spawn(move || { - cache2.get_or_load(&mut jbd2_dev, BLK_B) - }); + let handle = std::thread::spawn(move || cache2.get_or_load(&mut jbd2_dev, BLK_B)); // Step 3: wait for Thread 1 to enter I/O (lock dropped). enter_io.wait(); From 49cd1f0ae9f029b5d135a718a58538a0622c5aad Mon Sep 17 00:00:00 2001 From: Tempest Date: Fri, 5 Jun 2026 14:20:38 +0800 Subject: [PATCH 17/20] refactor(rsext4): replace external spin::Mutex with ax_kspin::SpinNoPreempt MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the external spin crate dependency with ArceOS's native ax_kspin spinlock for the three cache types (DataBlockCache, InodeCache, BitmapCache). Using SpinNoPreempt (BaseSpinLock) ensures kernel preemption is disabled while holding the cache lock — unlike the external spin::Mutex which has no integration with the kernel's preemption/IRQ system. Also replaces spin::LazyLock in crc32c/arm64.rs with an atomic-based once-initialized function (is_hardware_crc32_supported). Co-Authored-By: Claude Opus 4.8 --- components/rsext4/Cargo.toml | 2 +- components/rsext4/src/cache/bitmap.rs | 2 +- components/rsext4/src/cache/data_block.rs | 2 +- components/rsext4/src/cache/inode_table.rs | 2 +- components/rsext4/src/crc32c/arm64.rs | 23 ++++++++++++++++++++-- components/rsext4/src/crc32c/crc32c.rs | 2 +- 6 files changed, 26 insertions(+), 7 deletions(-) diff --git a/components/rsext4/Cargo.toml b/components/rsext4/Cargo.toml index ea69d8f690..2d1d490fd0 100644 --- a/components/rsext4/Cargo.toml +++ b/components/rsext4/Cargo.toml @@ -15,7 +15,7 @@ license = "Apache-2.0" [dependencies] bitflags = "2.10" log = "0.4" -spin = { workspace = true } +ax-kspin = { workspace = true } [features] default = ["USE_MULTILEVEL_CACHE"] diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 549f524928..85124f965a 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -3,7 +3,7 @@ use alloc::{collections::BTreeMap, vec::Vec}; use log::debug; -use spin::Mutex as SpinMutex; +use ax_kspin::SpinNoPreempt as SpinMutex; use crate::{ BITMAP_CACHE_MAX, diff --git a/components/rsext4/src/cache/data_block.rs b/components/rsext4/src/cache/data_block.rs index cb313adb18..645fb82be3 100644 --- a/components/rsext4/src/cache/data_block.rs +++ b/components/rsext4/src/cache/data_block.rs @@ -2,7 +2,7 @@ use alloc::{collections::BTreeMap, vec::Vec}; -use spin::Mutex as SpinMutex; +use ax_kspin::SpinNoPreempt as SpinMutex; use crate::{blockdev::*, bmalloc::AbsoluteBN, config::*, error::*}; /// Cache key for one physical data block. diff --git a/components/rsext4/src/cache/inode_table.rs b/components/rsext4/src/cache/inode_table.rs index b63057c09c..4d9da3ad49 100644 --- a/components/rsext4/src/cache/inode_table.rs +++ b/components/rsext4/src/cache/inode_table.rs @@ -2,7 +2,7 @@ use alloc::{collections::BTreeMap, vec::Vec}; -use spin::Mutex as SpinMutex; +use ax_kspin::SpinNoPreempt as SpinMutex; use crate::{ blockdev::*, diff --git a/components/rsext4/src/crc32c/arm64.rs b/components/rsext4/src/crc32c/arm64.rs index cf139f6689..39717ceb06 100644 --- a/components/rsext4/src/crc32c/arm64.rs +++ b/components/rsext4/src/crc32c/arm64.rs @@ -3,8 +3,27 @@ use core::arch::asm; #[cfg(target_arch = "aarch64")] -#[allow(dead_code)] -pub static HARDWARE_SUPPORT_CRC32: spin::LazyLock = spin::LazyLock::new(has_hardware_crc32); +use core::sync::atomic::{AtomicBool, Ordering}; + +#[cfg(target_arch = "aarch64")] +static CRC32_HW_CHECKED: AtomicBool = AtomicBool::new(false); +#[cfg(target_arch = "aarch64")] +static mut CRC32_HW_SUPPORTED: bool = false; + +/// Returns whether the current CPU supports hardware CRC32C instructions. +/// +/// The result is computed once on first call and cached for all subsequent calls. +#[cfg(target_arch = "aarch64")] +#[inline] +pub fn is_hardware_crc32_supported() -> bool { + if CRC32_HW_CHECKED.load(Ordering::Acquire) { + return unsafe { CRC32_HW_SUPPORTED }; + } + let supported = has_hardware_crc32(); + unsafe { CRC32_HW_SUPPORTED = supported }; + CRC32_HW_CHECKED.store(true, Ordering::Release); + supported +} // In `core::arch::aarch64`, the intrinsics with the `c` suffix implement the // Castagnoli polynomial used by CRC32C. diff --git a/components/rsext4/src/crc32c/crc32c.rs b/components/rsext4/src/crc32c/crc32c.rs index 64d6b582db..e8e4d4c9a6 100644 --- a/components/rsext4/src/crc32c/crc32c.rs +++ b/components/rsext4/src/crc32c/crc32c.rs @@ -58,7 +58,7 @@ pub fn crc32c_append(crc: u32, data: &[u8]) -> u32 { // Use hardware acceleration on aarch64 when the CPU advertises CRC support. #[cfg(target_arch = "aarch64")] { - if *HARDWARE_SUPPORT_CRC32 { + if is_hardware_crc32_supported() { return unsafe { crc32c_hardware(crc, data) }; } } From b2184eb66e2f4b9e3a5e8593fcf91c4379a75f8e Mon Sep 17 00:00:00 2001 From: Tempest Date: Fri, 5 Jun 2026 14:28:51 +0800 Subject: [PATCH 18/20] style(rsext4): fix stale comment and replace static mut with AtomicBool - Update comment in fs.rs: spin::Mutex -> SpinNoPreempt (matches actual lock type after external spin crate replacement) - Replace static mut CRC32_HW_SUPPORTED with AtomicBool in arm64.rs to eliminate unsafe blocks and follow Rust safety conventions. The Acquire/Release pair on CRC32_HW_CHECKED already provides the necessary happens-before ordering. Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/crc32c/arm64.rs | 10 +++++++--- os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs | 2 +- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/components/rsext4/src/crc32c/arm64.rs b/components/rsext4/src/crc32c/arm64.rs index 39717ceb06..099ae30278 100644 --- a/components/rsext4/src/crc32c/arm64.rs +++ b/components/rsext4/src/crc32c/arm64.rs @@ -8,19 +8,23 @@ use core::sync::atomic::{AtomicBool, Ordering}; #[cfg(target_arch = "aarch64")] static CRC32_HW_CHECKED: AtomicBool = AtomicBool::new(false); #[cfg(target_arch = "aarch64")] -static mut CRC32_HW_SUPPORTED: bool = false; +static CRC32_HW_SUPPORTED: AtomicBool = AtomicBool::new(false); /// Returns whether the current CPU supports hardware CRC32C instructions. /// /// The result is computed once on first call and cached for all subsequent calls. +/// +/// Safety: `CRC32_HW_CHECKED` (Acquire/Release) provides happens-before ordering; +/// `CRC32_HW_SUPPORTED` uses Relaxed because it is always accessed after +/// the Acquire load on `CRC32_HW_CHECKED` has observed the init-complete store. #[cfg(target_arch = "aarch64")] #[inline] pub fn is_hardware_crc32_supported() -> bool { if CRC32_HW_CHECKED.load(Ordering::Acquire) { - return unsafe { CRC32_HW_SUPPORTED }; + return CRC32_HW_SUPPORTED.load(Ordering::Relaxed); } let supported = has_hardware_crc32(); - unsafe { CRC32_HW_SUPPORTED = supported }; + CRC32_HW_SUPPORTED.store(supported, Ordering::Relaxed); CRC32_HW_CHECKED.store(true, Ordering::Release); supported } diff --git a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs index af72c04443..945954633d 100644 --- a/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs +++ b/os/arceos/modules/axfs-ng/src/fs/ext4/rsext4/fs.rs @@ -66,7 +66,7 @@ impl Ext4Filesystem { /// Uses `SpinNoIrq` rather than a blocking mutex because filesystem /// operations may be called from IRQ context (e.g., DHCP during network /// init), where sleeping is not allowed. The rsext4 caches (inode, - /// data-block, bitmap) provide fine-grained `spin::Mutex` for SMP + /// data-block, bitmap) provide fine-grained `SpinNoPreempt` for SMP /// concurrency; this global lock protects metadata mutations (allocators, /// superblock, group descriptors, journal commits). pub(crate) fn lock(&self) -> MutexGuard<'_, Ext4State> { From 5c27ac2113cc8ab6f5811cfcc393fbfc9f27f514 Mon Sep 17 00:00:00 2001 From: Tempest Date: Fri, 5 Jun 2026 15:03:06 +0800 Subject: [PATCH 19/20] style: cargo fmt Co-Authored-By: Claude Opus 4.8 --- components/rsext4/src/cache/bitmap.rs | 2 +- components/rsext4/src/crc32c/arm64.rs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/components/rsext4/src/cache/bitmap.rs b/components/rsext4/src/cache/bitmap.rs index 85124f965a..af9ca0704b 100644 --- a/components/rsext4/src/cache/bitmap.rs +++ b/components/rsext4/src/cache/bitmap.rs @@ -2,8 +2,8 @@ use alloc::{collections::BTreeMap, vec::Vec}; -use log::debug; use ax_kspin::SpinNoPreempt as SpinMutex; +use log::debug; use crate::{ BITMAP_CACHE_MAX, diff --git a/components/rsext4/src/crc32c/arm64.rs b/components/rsext4/src/crc32c/arm64.rs index 099ae30278..6b4f90ee6c 100644 --- a/components/rsext4/src/crc32c/arm64.rs +++ b/components/rsext4/src/crc32c/arm64.rs @@ -1,7 +1,6 @@ #[cfg(target_arch = "aarch64")] #[allow(dead_code)] use core::arch::asm; - #[cfg(target_arch = "aarch64")] use core::sync::atomic::{AtomicBool, Ordering}; From 32113b9e5a9d738d73e511727e8a581ae774142f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=91=A8=E7=9D=BF?= Date: Fri, 5 Jun 2026 22:16:55 +0800 Subject: [PATCH 20/20] chore(rsext4): update lockfile after dev merge --- Cargo.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index 7b335f2f50..368078befd 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7066,9 +7066,9 @@ dependencies = [ name = "rsext4" version = "0.5.0" dependencies = [ + "ax-kspin", "bitflags 2.11.1", "log", - "spin 0.12.0", ] [[package]]