ext2: cache sb as a regular block, avoid incoherency

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<T, F: FnOnce(&[u8]) -> Result<T, Error>>(
+        &self,
+        index: u32,
+        mapper: F,
+    ) -> Result<T, Error> {
+        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<T, F: FnOnce(&mut [u8]) -> Result<T, Error>>(
+        &self,
+        index: u32,
+        write_size: usize,
+        mapper: F,
+    ) -> Result<T, Error> {
+        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<T, F: FnOnce(&mut ExtendedSuperblock) -> Result<T, Error>>(
+        &self,
+        mapper: F,
+    ) -> Result<T, Error> {
+        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::<ExtendedSuperblock>(), |block| {
+                let sb = bytemuck::from_bytes_mut(
+                    &mut block[offset..offset + size_of::<ExtendedSuperblock>()],
+                );
+                mapper(sb)
+            })
+            .await
+    }
+
+    pub async fn with_inode_block<T, F: FnOnce(&[u8]) -> Result<T, Error>>(
+        &self,
+        inode: &Inode,
+        block: u32,
+        mapper: F,
+    ) -> Result<T, Error> {
+        let block_index = self.inode_block_index(inode, block).await?;
+        self.with_block(block_index, mapper).await
+    }
+
+    pub async fn with_inode_block_mut<T, F: FnOnce(&mut [u8]) -> Result<T, Error>>(
+        &self,
+        inode: &Inode,
+        block: u32,
+        write_size: usize,
+        mapper: F,
+    ) -> Result<T, Error> {
+        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<T, F: FnOnce(&BlockGroupDescriptor) -> Result<T, Error>>(
+        &self,
+        block_group: u32,
+        mapper: F,
+    ) -> Result<T, Error> {
+        let offset = block_group as usize * size_of::<BlockGroupDescriptor>();
+        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::<BlockGroupDescriptor>()],
+            );
+            mapper(descriptor)
+        })
+        .await
+    }
+
+    pub(crate) async fn with_bgdt_entry_mut<
+        T,
+        F: FnOnce(&mut BlockGroupDescriptor) -> Result<T, Error>,
+    >(
+        &self,
+        block_group: u32,
+        mapper: F,
+    ) -> Result<T, Error> {
+        let offset = block_group as usize * size_of::<BlockGroupDescriptor>();
+        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::<BlockGroupDescriptor>(),
+            |block| {
+                let descriptor = bytemuck::from_bytes_mut(
+                    &mut block
+                        [offset_in_block..offset_in_block + size_of::<BlockGroupDescriptor>()],
+                );
+                mapper(descriptor)
+            },
+        )
+        .await
+    }
+}

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");
 
-                {
+                self.with_superblock_mut(|sb| {
-                    let mut state = self.state.write();
+                    superblock_mapper(sb);
-                    superblock_mapper(&mut state.superblock);
+                    Ok(())
-                    state.dirty = true;
+                })
-                }
+                .await?;
-
-                if !self.cached {
-                    self.flush_superblock().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?;

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;
 

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(())

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)
             }
         }
     }

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<Arc<InodeCache>>,
-    cached: bool,
 
-    state: IrqSafeRwLock<State>,
+    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::<ExtendedSuperblock>() > 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 {
+            sb_position: data::SUPERBLOCK_OFFSET,
-                superblock,
+            // state: IrqSafeRwLock::new(State {
-                dirty: false,
+            //     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<T, F: FnOnce(&[u8]) -> Result<T, Error>>(
-        &self,
-        index: u32,
-        mapper: F,
-    ) -> Result<T, Error> {
-        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<T, F: FnOnce(&mut [u8]) -> Result<T, Error>>(
-        &self,
-        index: u32,
-        write_size: usize,
-        mapper: F,
-    ) -> Result<T, Error> {
-        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<T, F: FnOnce(&[u8]) -> Result<T, Error>>(
-        &self,
-        inode: &Inode,
-        block: u32,
-        mapper: F,
-    ) -> Result<T, Error> {
-        let block_index = self.inode_block_index(inode, block).await?;
-        self.with_block(block_index, mapper).await
-    }
-
-    pub async fn with_inode_block_mut<T, F: FnOnce(&mut [u8]) -> Result<T, Error>>(
-        &self,
-        inode: &Inode,
-        block: u32,
-        write_size: usize,
-        mapper: F,
-    ) -> Result<T, Error> {
-        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<T, F: FnOnce(&BlockGroupDescriptor) -> Result<T, Error>>(
-        &self,
-        block_group: u32,
-        mapper: F,
-    ) -> Result<T, Error> {
-        let offset = block_group as usize * size_of::<BlockGroupDescriptor>();
-        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::<BlockGroupDescriptor>()],
-            );
-            mapper(descriptor)
-        })
-        .await
-    }
-
-    async fn with_bgdt_entry_mut<T, F: FnOnce(&mut BlockGroupDescriptor) -> Result<T, Error>>(
-        &self,
-        block_group: u32,
-        mapper: F,
-    ) -> Result<T, Error> {
-        let offset = block_group as usize * size_of::<BlockGroupDescriptor>();
-        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::<BlockGroupDescriptor>(),
-            |block| {
-                let descriptor = bytemuck::from_bytes_mut(
-                    &mut block
-                        [offset_in_block..offset_in_block + size_of::<BlockGroupDescriptor>()],
-                );
-                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> {
+    // pub async fn flush_superblock(&self) -> Result<(), Error> {
-        let state = self.state.read();
+    //     let state = self.state.read();
-        if state.dirty {
+    //     if state.dirty {
-            log::info!("Flushing superblock");
+    //         log::info!("Flushing superblock");
-            log::info!(
+    //         log::info!(
-                "inodes {} blocks {}",
+    //             "inodes {} blocks {}",
-                state.superblock.total_unallocated_inodes,
+    //             state.superblock.total_unallocated_inodes,
-                state.superblock.total_unallocated_blocks
+    //             state.superblock.total_unallocated_blocks
-            );
+    //         );
-            self.mapper
+    //         self.mapper
-                .device()
+    //             .device()
-                .write_exact(
+    //             .write_exact(
-                    data::SUPERBLOCK_OFFSET,
+    //                 data::SUPERBLOCK_OFFSET,
-                    bytemuck::bytes_of(&state.superblock),
+    //                 bytemuck::bytes_of(&state.superblock),
-                )
+    //             )
-                .await?;
+    //             .await?;
-        }
+    //     }
-        Ok(())
+    //     Ok(())
-    }
+    // }
 
     async fn read_index(&self, block_index: u32, index: usize) -> Result<u32, Error> {
         self.with_block(block_index, |block| {

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(())

kernel/libk/src/device/block/cache.rs

@@ -164,6 +164,13 @@ impl<A: PhysicalMemoryAllocator<Address = PhysicalAddress>> UncachedCache<A> {
         pos: u64,
         mapper: F,
     ) -> Result<T, Error> {
+        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<A: PhysicalMemoryAllocator<Address = PhysicalAddress>> UncachedCache<A> {
         size: usize,
         mapper: F,
     ) -> Result<T, Error> {
+        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<A: PhysicalMemoryAllocator<Address = PhysicalAddress>> BlockCache<A> {
         block_position: u64,
         mapper: F,
     ) -> Result<T, Error> {
+        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<A: PhysicalMemoryAllocator<Address = PhysicalAddress>> BlockCache<A> {
         _size: usize,
         mapper: F,
     ) -> Result<T, Error> {
+        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?;

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 {
+        if let DeviceRequest::IsTerminal(value) = req {
-            DeviceRequest::IsTerminal(value) => {
+            *value = self.is_terminal();
-                *value = self.is_terminal();
+            return Ok(());
-                return Ok(());
-            }
-            _ => (),
         }
 
         match self {

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

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) {
+    matches!((res, option), (Ok(()), DeviceRequest::IsTerminal(true)))
-        (Ok(()), DeviceRequest::IsTerminal(true)) => true,
-        _ => false,
-    }
 }