From 3aea206cadcd3390d98308fd11e27321c674c569 Mon Sep 17 00:00:00 2001 From: Mark Poliakov Date: Tue, 31 Dec 2024 12:10:30 +0200 Subject: [PATCH] ext2: cache sb as a regular block, avoid incoherency --- kernel/driver/fs/ext2/src/access.rs | 122 +++++++++++++++++ kernel/driver/fs/ext2/src/allocation.rs | 24 +--- kernel/driver/fs/ext2/src/dir.rs | 12 +- kernel/driver/fs/ext2/src/file.rs | 2 +- kernel/driver/fs/ext2/src/inode.rs | 6 +- kernel/driver/fs/ext2/src/lib.rs | 166 +++++------------------- kernel/driver/fs/ext2/src/symlink.rs | 4 +- kernel/libk/src/device/block/cache.rs | 28 ++++ kernel/libk/src/vfs/file/mod.rs | 9 +- kernel/libk/src/vfs/node/ops.rs | 4 +- lib/runtime/src/io.rs | 5 +- 11 files changed, 206 insertions(+), 176 deletions(-) create mode 100644 kernel/driver/fs/ext2/src/access.rs diff --git a/kernel/driver/fs/ext2/src/access.rs b/kernel/driver/fs/ext2/src/access.rs new file mode 100644 index 00000000..e07dbd56 --- /dev/null +++ b/kernel/driver/fs/ext2/src/access.rs @@ -0,0 +1,122 @@ +use libk::error::Error; + +use crate::{BlockGroupDescriptor, Ext2Fs, ExtendedSuperblock, Inode}; + +impl Ext2Fs { + pub async fn with_block Result>( + &self, + index: u32, + mapper: F, + ) -> Result { + if index < 1 || index >= self.total_blocks { + return Err(Error::InvalidFile); + } + self.mapper + .try_with(self.block_address(index), mapper) + .await + } + + pub async fn with_block_mut Result>( + &self, + index: u32, + write_size: usize, + mapper: F, + ) -> Result { + if index < 1 || index >= self.total_blocks { + return Err(Error::InvalidFile); + } + self.mapper + .try_with_mut(self.block_address(index), write_size, mapper) + .await + } + + pub async fn with_superblock_mut Result>( + &self, + mapper: F, + ) -> Result { + let position = self.sb_position & !(self.block_size as u64 - 1); + let offset = (self.sb_position & (self.block_size as u64 - 1)) as usize; + + self.mapper + .try_with_mut(position, size_of::(), |block| { + let sb = bytemuck::from_bytes_mut( + &mut block[offset..offset + size_of::()], + ); + mapper(sb) + }) + .await + } + + pub async fn with_inode_block Result>( + &self, + inode: &Inode, + block: u32, + mapper: F, + ) -> Result { + let block_index = self.inode_block_index(inode, block).await?; + self.with_block(block_index, mapper).await + } + + pub async fn with_inode_block_mut Result>( + &self, + inode: &Inode, + block: u32, + write_size: usize, + mapper: F, + ) -> Result { + let block_index = self.inode_block_index(inode, block).await?; + self.with_block_mut(block_index, write_size, mapper).await + } + + pub(crate) async fn with_bgdt_entry Result>( + &self, + block_group: u32, + mapper: F, + ) -> Result { + let offset = block_group as usize * size_of::(); + let block = offset / self.block_size; + if block >= self.bgdt.block_count { + log::warn!("ext2: bgdt block out of bounds: {block}"); + return Err(Error::InvalidArgument); + } + let offset_in_block = offset % self.block_size; + + self.with_block(block as u32 + self.bgdt.base, |block| { + let descriptor = bytemuck::from_bytes( + &block[offset_in_block..offset_in_block + size_of::()], + ); + mapper(descriptor) + }) + .await + } + + pub(crate) async fn with_bgdt_entry_mut< + T, + F: FnOnce(&mut BlockGroupDescriptor) -> Result, + >( + &self, + block_group: u32, + mapper: F, + ) -> Result { + let offset = block_group as usize * size_of::(); + let block = offset / self.block_size; + if block >= self.bgdt.block_count { + log::warn!("ext2: bgdt block out of bounds: {block}"); + return Err(Error::InvalidArgument); + } + let offset_in_block = offset % self.block_size; + + self.with_block_mut( + block as u32 + self.bgdt.base, + size_of::(), + |block| { + let descriptor = bytemuck::from_bytes_mut( + &mut block + [offset_in_block..offset_in_block + size_of::()], + ); + mapper(descriptor) + }, + ) + .await + } +} diff --git a/kernel/driver/fs/ext2/src/allocation.rs b/kernel/driver/fs/ext2/src/allocation.rs index fecb9264..bc061473 100644 --- a/kernel/driver/fs/ext2/src/allocation.rs +++ b/kernel/driver/fs/ext2/src/allocation.rs @@ -39,15 +39,11 @@ impl Ext2Fs { .await? .expect("TODO: bgdt says there're things, but bitmap says there aren't"); - { - let mut state = self.state.write(); - superblock_mapper(&mut state.superblock); - state.dirty = true; - } - - if !self.cached { - self.flush_superblock().await?; - } + self.with_superblock_mut(|sb| { + superblock_mapper(sb); + Ok(()) + }) + .await?; return Ok(group_index * group_item_count + no); } @@ -69,16 +65,11 @@ impl Ext2Fs { descriptor.directories += 1; } descriptor.unallocated_inodes -= 1; - log::info!("bg unallocated_inodes = {}", descriptor.unallocated_inodes); Some((descriptor.inode_usage_bitmap, self.block_group_inode_count)) } }, |superblock| { superblock.total_unallocated_inodes -= 1; - log::info!( - "total_unallocated_inodes = {}", - superblock.total_unallocated_inodes - ); }, ) .await?; @@ -97,16 +88,11 @@ impl Ext2Fs { None } else { descriptor.unallocated_blocks -= 1; - log::info!("bg unallocated_blocks = {}", descriptor.unallocated_blocks); Some((descriptor.block_usage_bitmap, self.block_group_block_count)) } }, |superblock| { superblock.total_unallocated_blocks -= 1; - log::info!( - "total_unallocated_blocks = {}", - superblock.total_unallocated_blocks - ); }, ) .await?; diff --git a/kernel/driver/fs/ext2/src/dir.rs b/kernel/driver/fs/ext2/src/dir.rs index 2fa7fd62..35f68333 100644 --- a/kernel/driver/fs/ext2/src/dir.rs +++ b/kernel/driver/fs/ext2/src/dir.rs @@ -317,7 +317,7 @@ impl DirectoryNode { for i in 0..n { let fit_block = self .fs - .with_inode_block_mut(&inode, i, 0, |block| { + .with_inode_block_mut(inode, i, 0, |block| { let mut iter = DirentIterMut::new(&self.fs, &mut block[..], 0); if iter.try_fit(name, ino) { Ok(true) @@ -346,7 +346,7 @@ impl DirectoryNode { .await?; self.fs - .with_inode_block_mut(&inode, block_index, self.fs.block_size, |block| { + .with_inode_block_mut(inode, block_index, self.fs.block_size, |block| { block.fill(0); // Place dirent @@ -391,7 +391,7 @@ impl DirectoryNode { for i in 0..n { self.fs - .with_inode_block(&inode, i as _, |block| { + .with_inode_block(inode, i as _, |block| { let iter = DirentIter::new(&self.fs, block, 0); for (_, name, _) in iter { @@ -439,7 +439,7 @@ impl DirectoryNode { for i in 0..n { let ino = self .fs - .with_inode_block_mut(&inode, i as _, 0, |block| { + .with_inode_block_mut(inode, i as _, 0, |block| { let mut iter = DirentIterMut::new(&self.fs, block, 0); Ok(iter.remove(name)) }) @@ -482,7 +482,7 @@ impl DirectoryNode { for i in 0..n { let ino = self .fs - .with_inode_block(&inode, i, |block| { + .with_inode_block(inode, i, |block| { let iter = DirentIter::new(&self.fs, block, 0); for (dirent, name, _) in iter { @@ -531,7 +531,7 @@ impl DirectoryNode { let (entry_count, new_pos) = self .fs - .with_inode_block(&inode, index as u32, |block| { + .with_inode_block(inode, index as u32, |block| { let mut pos = pos; let mut entry_count = 0; diff --git a/kernel/driver/fs/ext2/src/file.rs b/kernel/driver/fs/ext2/src/file.rs index 9d5f1aa3..864b8f26 100644 --- a/kernel/driver/fs/ext2/src/file.rs +++ b/kernel/driver/fs/ext2/src/file.rs @@ -58,7 +58,7 @@ impl RegularNode { let amount = remaining.min(self.fs.block_size - block_offset); self.fs - .with_inode_block_mut(&inode, block_index as u32, amount, |block| { + .with_inode_block_mut(inode, block_index as u32, amount, |block| { block[block_offset..block_offset + amount] .copy_from_slice(&buffer[offset..offset + amount]); Ok(()) diff --git a/kernel/driver/fs/ext2/src/inode.rs b/kernel/driver/fs/ext2/src/inode.rs index 814a36c8..a6e1dc8b 100644 --- a/kernel/driver/fs/ext2/src/inode.rs +++ b/kernel/driver/fs/ext2/src/inode.rs @@ -89,7 +89,7 @@ impl InodeAccess { let inode = self.inode_cache.get(self.ino).await?; let result = { let lock = inode.read(); - mapper(&*lock) + mapper(&lock) }; result } @@ -101,7 +101,7 @@ impl InodeAccess { let mut inode = self.inode_cache.get_mut(self.ino).await?; let result = { let mut lock = inode.write(); - mapper(&mut *lock) + mapper(&mut lock) }; inode.put().await?; result @@ -227,7 +227,7 @@ impl InodeAccess { Err(error) => { log::warn!("ext2: couldn't set up inode #{ino}: {error:?}"); // TODO free the inode and flush it from the cache - return Err(error); + Err(error) } } } diff --git a/kernel/driver/fs/ext2/src/lib.rs b/kernel/driver/fs/ext2/src/lib.rs index bf9c94b9..18b067db 100644 --- a/kernel/driver/fs/ext2/src/lib.rs +++ b/kernel/driver/fs/ext2/src/lib.rs @@ -16,8 +16,9 @@ use libk::{ error::Error, vfs::{Filesystem, FilesystemMountOption, NodeRef}, }; -use libk_util::{sync::spin_rwlock::IrqSafeRwLock, OneTimeInit}; +use libk_util::OneTimeInit; +pub mod access; mod allocation; mod data; pub mod inode; @@ -30,11 +31,6 @@ pub use data::{BlockGroupDescriptor, Dirent, ExtendedSuperblock, Inode}; use symlink::SymlinkNode; use yggdrasil_abi::io::FileType; -struct State { - superblock: ExtendedSuperblock, - dirty: bool, -} - struct Bgdt { base: u32, entry_count: usize, @@ -44,9 +40,8 @@ struct Bgdt { pub struct Ext2Fs { mapper: DeviceMapper, inode_cache: OneTimeInit>, - cached: bool, - state: IrqSafeRwLock, + sb_position: u64, bgdt: Bgdt, total_inodes: u32, @@ -73,10 +68,6 @@ impl Filesystem for Ext2Fs { log::info!("ext2: flushing caches"); let mut last_err = None; - if let Err(error) = self.flush_superblock().await { - log::error!("ext2: superblock flush error: {error:?}"); - last_err = Some(error); - } if let Err(error) = self.flush_inode_cache().await { log::error!("ext2: inode cache flush error {error:?}"); last_err = Some(error); @@ -215,6 +206,12 @@ impl Ext2Fs { } let block_size = 1024usize << superblock.block_size_log2; + + if data::SUPERBLOCK_OFFSET as usize + size_of::() > block_size { + log::error!("ext2: superblock struct crosses block boundary, cannot mount"); + return Err(Error::InvalidArgument); + } + let bgdt_block_index = (data::SUPERBLOCK_OFFSET as usize).div_ceil(block_size) as u32; let bgdt_entry_count = superblock .total_blocks @@ -277,12 +274,12 @@ impl Ext2Fs { mapper, inode_cache: OneTimeInit::new(), - cached, - state: IrqSafeRwLock::new(State { - superblock, - dirty: false, - }), + sb_position: data::SUPERBLOCK_OFFSET, + // state: IrqSafeRwLock::new(State { + // superblock, + // dirty: false, + // }), bgdt, required_features, @@ -313,103 +310,6 @@ impl Ext2Fs { index as u64 * self.block_size as u64 } - pub async fn with_block Result>( - &self, - index: u32, - mapper: F, - ) -> Result { - if index < 1 || index >= self.total_blocks { - return Err(Error::InvalidFile); - } - self.mapper - .try_with(self.block_address(index), mapper) - .await - } - - pub async fn with_block_mut Result>( - &self, - index: u32, - write_size: usize, - mapper: F, - ) -> Result { - if index < 1 || index >= self.total_blocks { - return Err(Error::InvalidFile); - } - self.mapper - .try_with_mut(self.block_address(index), write_size, mapper) - .await - } - - pub async fn with_inode_block Result>( - &self, - inode: &Inode, - block: u32, - mapper: F, - ) -> Result { - let block_index = self.inode_block_index(inode, block).await?; - self.with_block(block_index, mapper).await - } - - pub async fn with_inode_block_mut Result>( - &self, - inode: &Inode, - block: u32, - write_size: usize, - mapper: F, - ) -> Result { - let block_index = self.inode_block_index(inode, block).await?; - self.with_block_mut(block_index, write_size, mapper).await - } - - async fn with_bgdt_entry Result>( - &self, - block_group: u32, - mapper: F, - ) -> Result { - let offset = block_group as usize * size_of::(); - let block = offset / self.block_size; - if block >= self.bgdt.block_count { - log::warn!("ext2: bgdt block out of bounds: {block}"); - return Err(Error::InvalidArgument); - } - let offset_in_block = offset % self.block_size; - - self.with_block(block as u32 + self.bgdt.base, |block| { - let descriptor = bytemuck::from_bytes( - &block[offset_in_block..offset_in_block + size_of::()], - ); - mapper(descriptor) - }) - .await - } - - async fn with_bgdt_entry_mut Result>( - &self, - block_group: u32, - mapper: F, - ) -> Result { - let offset = block_group as usize * size_of::(); - let block = offset / self.block_size; - if block >= self.bgdt.block_count { - log::warn!("ext2: bgdt block out of bounds: {block}"); - return Err(Error::InvalidArgument); - } - let offset_in_block = offset % self.block_size; - - self.with_block_mut( - block as u32 + self.bgdt.base, - size_of::(), - |block| { - let descriptor = bytemuck::from_bytes_mut( - &mut block - [offset_in_block..offset_in_block + size_of::()], - ); - mapper(descriptor) - }, - ) - .await - } - async fn inode(&self, ino: u32) -> Result<(u32, usize), Error> { if ino < 1 || ino >= self.total_inodes { return Err(Error::InvalidFile); @@ -491,25 +391,25 @@ impl Ext2Fs { self.inode_cache.get().flush().await } - pub async fn flush_superblock(&self) -> Result<(), Error> { - let state = self.state.read(); - if state.dirty { - log::info!("Flushing superblock"); - log::info!( - "inodes {} blocks {}", - state.superblock.total_unallocated_inodes, - state.superblock.total_unallocated_blocks - ); - self.mapper - .device() - .write_exact( - data::SUPERBLOCK_OFFSET, - bytemuck::bytes_of(&state.superblock), - ) - .await?; - } - Ok(()) - } + // pub async fn flush_superblock(&self) -> Result<(), Error> { + // let state = self.state.read(); + // if state.dirty { + // log::info!("Flushing superblock"); + // log::info!( + // "inodes {} blocks {}", + // state.superblock.total_unallocated_inodes, + // state.superblock.total_unallocated_blocks + // ); + // self.mapper + // .device() + // .write_exact( + // data::SUPERBLOCK_OFFSET, + // bytemuck::bytes_of(&state.superblock), + // ) + // .await?; + // } + // Ok(()) + // } async fn read_index(&self, block_index: u32, index: usize) -> Result { self.with_block(block_index, |block| { diff --git a/kernel/driver/fs/ext2/src/symlink.rs b/kernel/driver/fs/ext2/src/symlink.rs index 42e416b5..7262a59f 100644 --- a/kernel/driver/fs/ext2/src/symlink.rs +++ b/kernel/driver/fs/ext2/src/symlink.rs @@ -48,12 +48,12 @@ impl SymlinkNode { // If length of symlink is lower than 60, data is stored directly in "block address" // section of the inode if len < 60 { - let bytes = unsafe { Self::link_from_inode_blocks(&inode, len) }; + let bytes = unsafe { Self::link_from_inode_blocks(inode, len) }; write.extend_from_slice(bytes); buf[..len].copy_from_slice(bytes); } else { self.fs - .with_inode_block(&inode, 0, |block| { + .with_inode_block(inode, 0, |block| { write.extend_from_slice(&block[..len]); buf[..len].copy_from_slice(&block[..len]); Ok(()) diff --git a/kernel/libk/src/device/block/cache.rs b/kernel/libk/src/device/block/cache.rs index 5125bbcb..0191776d 100644 --- a/kernel/libk/src/device/block/cache.rs +++ b/kernel/libk/src/device/block/cache.rs @@ -164,6 +164,13 @@ impl> UncachedCache { pos: u64, mapper: F, ) -> Result { + if pos % self.block_size as u64 != 0 { + log::warn!( + "uncached: position {pos} is not a multiple of block size {}", + self.block_size + ); + return Err(Error::InvalidArgument); + } let mut data = PageBox::<_, A>::new_uninit_slice_in(self.block_size)?; self.device.read_aligned(pos, data.as_slice_mut()).await?; let result = mapper(unsafe { data.assume_init_slice_ref() })?; @@ -176,6 +183,13 @@ impl> UncachedCache { size: usize, mapper: F, ) -> Result { + if pos % self.block_size as u64 != 0 { + log::warn!( + "uncached: position {pos} is not a multiple of block size {}", + self.block_size + ); + return Err(Error::InvalidArgument); + } let mut data = PageBox::<_, A>::new_uninit_slice_in(self.block_size)?; // No need to read a block only to then fully rewrite it if size != self.block_size { @@ -292,6 +306,13 @@ impl> BlockCache { block_position: u64, mapper: F, ) -> Result { + if block_position % self.block_size as u64 != 0 { + log::warn!( + "mapper: position {block_position} is not a multiple of block size {}", + self.block_size + ); + return Err(Error::InvalidArgument); + } let segment_position = block_position & !(self.segment_size as u64 - 1); let segment_offset = (block_position & (self.segment_size as u64 - 1)) as usize; let block = self.entry(segment_position).await?; @@ -305,6 +326,13 @@ impl> BlockCache { _size: usize, mapper: F, ) -> Result { + if block_position % self.block_size as u64 != 0 { + log::warn!( + "mapper: position {block_position} is not a multiple of block size {}", + self.block_size + ); + return Err(Error::InvalidArgument); + } let segment_position = block_position & !(self.segment_size as u64 - 1); let segment_offset = (block_position & (self.segment_size as u64 - 1)) as usize; let block = self.entry(segment_position).await?; diff --git a/kernel/libk/src/vfs/file/mod.rs b/kernel/libk/src/vfs/file/mod.rs index e6fd287c..6e43fdf0 100644 --- a/kernel/libk/src/vfs/file/mod.rs +++ b/kernel/libk/src/vfs/file/mod.rs @@ -343,12 +343,9 @@ impl File { /// Performs a device-specific request pub fn device_request(&self, req: &mut DeviceRequest) -> Result<(), Error> { - match req { - DeviceRequest::IsTerminal(value) => { - *value = self.is_terminal(); - return Ok(()); - } - _ => (), + if let DeviceRequest::IsTerminal(value) = req { + *value = self.is_terminal(); + return Ok(()); } match self { diff --git a/kernel/libk/src/vfs/node/ops.rs b/kernel/libk/src/vfs/node/ops.rs index b089e8dc..e698ec21 100644 --- a/kernel/libk/src/vfs/node/ops.rs +++ b/kernel/libk/src/vfs/node/ops.rs @@ -241,7 +241,7 @@ impl Node { if !self.flags.contains(NodeFlags::IN_MEMORY_PROPS) { // Update permissions in the real node - common.set_metadata(self, &metadata)?; + common.set_metadata(self, metadata)?; } Ok(()) @@ -255,7 +255,7 @@ impl Node { .metadata .get_or_try_insert_with(|| self.data_as_common().metadata(self))?; - Ok(metadata.clone()) + Ok(*metadata) } // TODO clarify directory size diff --git a/lib/runtime/src/io.rs b/lib/runtime/src/io.rs index 8976ca1e..eb3fa3df 100644 --- a/lib/runtime/src/io.rs +++ b/lib/runtime/src/io.rs @@ -54,8 +54,5 @@ pub fn set_current_directory(path: &str) -> Result<(), Error> { pub fn is_terminal(f: RawFd) -> bool { let mut option = DeviceRequest::IsTerminal(false); let res = unsafe { crate::sys::device_request(f, &mut option) }; - match (res, option) { - (Ok(()), DeviceRequest::IsTerminal(true)) => true, - _ => false, - } + matches!((res, option), (Ok(()), DeviceRequest::IsTerminal(true))) }