ext2: Use builder pattern for creating fs

Introduce `Builder` struct for creating file system.
We define intermediate structures for each step of creating ext2,
which makes it easier to change the filesystem construction logic in
the following CL.

BUG=b:329359333
TEST=presubmit

Change-Id: Ib05f838f135636dfcc0d2d04bbcb402b4785c162
Reviewed-on: https://chromium-review.googlesource.com/c/crosvm/crosvm/+/5851191
Reviewed-by: Takaya Saeki <takayas@chromium.org>
Commit-Queue: Keiichi Watanabe <keiichiw@chromium.org>
Reviewed-by: Junichi Uekawa <uekawa@chromium.org>

ext2/examples/mkfs.rs

@@ -12,7 +12,6 @@ mod linux {
 
     use argh::FromArgs;
     use base::MappedRegion;
-    use ext2::create_ext2_region;
 
     #[derive(FromArgs)]
     /// Create ext2 filesystem.
@@ -46,12 +45,15 @@ mod linux {
     pub fn main() -> anyhow::Result<()> {
         let args: Args = argh::from_env();
         let src_dir = args.src.as_ref().map(|s| Path::new(s.as_str()));
-        let cfg = ext2::Config {
+        let builder = ext2::Builder {
             blocks_per_group: args.blocks_per_group,
             inodes_per_group: args.inodes_per_group,
             size: args.size,
         };
-        let mem = create_ext2_region(&cfg, src_dir)?;
+        let mem = builder
+            .allocate_memory()?
+            .build_mmap_info(src_dir)?
+            .do_mmap()?;
         if args.dry_run {
             println!("Done!");
             return Ok(());

ext2/src/blockgroup.rs

@@ -131,7 +131,7 @@ mod test {
     use base::MemoryMappingBuilder;
 
     use super::*;
-    use crate::superblock::Config;
+    use crate::Builder;
 
     // Check if `GroupMetaData` is correctly initialized from `SuperBlock` with one block group.
     #[test]
@@ -143,7 +143,7 @@ mod test {
         let arena = Arena::new(BLOCK_SIZE, &mut mem).unwrap();
         let sb = SuperBlock::new(
             &arena,
-            &Config {
+            &Builder {
                 inodes_per_group: 1024,
                 blocks_per_group,
                 size,
@@ -196,7 +196,7 @@ mod test {
         let arena = Arena::new(BLOCK_SIZE, &mut mem).unwrap();
         let sb = SuperBlock::new(
             &arena,
-            &Config {
+            &Builder {
                 inodes_per_group: 512,
                 blocks_per_group,
                 size: mem_size,

ext2/src/builder.rs

@@ -0,0 +1,132 @@
+// Copyright 2024 The ChromiumOS Authors
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+//! Provides structs and logic to build ext2 file system with configurations.
+
+use std::path::Path;
+
+use anyhow::bail;
+use anyhow::Context;
+use anyhow::Result;
+use base::MappedRegion;
+use base::MemoryMapping;
+use base::MemoryMappingArena;
+use base::MemoryMappingBuilder;
+use base::Protection;
+
+use crate::arena::Arena;
+use crate::arena::FileMappingInfo;
+use crate::fs::Ext2;
+use crate::BLOCK_SIZE;
+
+/// A struct to represent the configuration of an ext2 filesystem.
+pub struct Builder {
+    /// The number of blocks per group.
+    pub blocks_per_group: u32,
+    /// The number of inodes per group.
+    pub inodes_per_group: u32,
+    /// The size of the memory region.
+    pub size: u32,
+}
+
+impl Default for Builder {
+    fn default() -> Self {
+        Self {
+            blocks_per_group: 4096,
+            inodes_per_group: 4096,
+            size: 4096 * 4096,
+        }
+    }
+}
+
+impl Builder {
+    /// Validates field values and adjusts them if needed.
+    fn validate(&mut self) -> Result<()> {
+        let block_group_size = BLOCK_SIZE as u32 * self.blocks_per_group;
+        if self.size < block_group_size {
+            bail!(
+            "memory size {} is too small to have a block group: block_size={},  block_per_group={}",
+            self.size,
+            BLOCK_SIZE,
+            block_group_size
+        );
+        }
+        if self.size % block_group_size != 0 {
+            // Round down to the largest multiple of block_group_size that is smaller than self.size
+            self.size = self.size.next_multiple_of(block_group_size) - block_group_size
+        };
+        Ok(())
+    }
+
+    /// Allocates memory region with the given configuration.
+    pub fn allocate_memory(mut self) -> Result<MemRegion> {
+        self.validate()
+            .context("failed to validate the ext2 config")?;
+        let mem = MemoryMappingBuilder::new(self.size as usize)
+            .build()
+            .context("failed to allocate memory for ext2")?;
+        Ok(MemRegion { cfg: self, mem })
+    }
+}
+
+/// Memory region for ext2 with its config.
+pub struct MemRegion {
+    cfg: Builder,
+    mem: MemoryMapping,
+}
+
+impl MemRegion {
+    /// Constructs an ext2 metadata by traversing `src_dir`.
+    pub fn build_mmap_info(mut self, src_dir: Option<&Path>) -> Result<MemRegionWithMappingInfo> {
+        let arena = Arena::new(BLOCK_SIZE, &mut self.mem).context("failed to allocate arena")?;
+        let mut ext2 = Ext2::new(&self.cfg, &arena).context("failed to create Ext2 struct")?;
+        if let Some(dir) = src_dir {
+            ext2.copy_dirtree(&arena, dir)
+                .context("failed to copy directory tree")?;
+        }
+        ext2.copy_backup_metadata(&arena)
+            .context("failed to copy metadata for backup")?;
+        let mapping_info = arena.into_mapping_info();
+
+        self.mem
+            .msync()
+            .context("failed to msyn of ext2's memory region")?;
+        Ok(MemRegionWithMappingInfo {
+            mem: self.mem,
+            mapping_info,
+        })
+    }
+}
+
+/// Memory regions where ext2 metadata were written with information of mmap operations to be done.
+pub struct MemRegionWithMappingInfo {
+    mem: MemoryMapping,
+    mapping_info: Vec<FileMappingInfo>,
+}
+
+impl MemRegionWithMappingInfo {
+    /// Do mmap and returns the memory region where ext2 was created.
+    pub fn do_mmap(self) -> Result<MemoryMappingArena> {
+        let mut mmap_arena = MemoryMappingArena::from(self.mem);
+        for FileMappingInfo {
+            mem_offset,
+            file,
+            length,
+            file_offset,
+        } in self.mapping_info
+        {
+            mmap_arena
+                .add_fd_mapping(
+                    mem_offset,
+                    length,
+                    &file,
+                    file_offset as u64, /* fd_offset */
+                    Protection::read(),
+                )
+                .context("failed mmaping an fd for ext2")?;
+        }
+
+        Ok(mmap_arena)
+    }
+}

ext2/src/fs.rs

@@ -18,26 +18,21 @@ use anyhow::bail;
 use anyhow::Context;
 use anyhow::Result;
 use base::info;
-use base::MappedRegion;
-use base::MemoryMappingArena;
-use base::MemoryMappingBuilder;
-use base::Protection;
 use zerocopy::AsBytes;
 use zerocopy::FromBytes;
 use zerocopy::FromZeroes;
 
 use crate::arena::Arena;
 use crate::arena::BlockId;
-use crate::arena::FileMappingInfo;
 use crate::blockgroup::BlockGroupDescriptor;
 use crate::blockgroup::GroupMetaData;
 use crate::blockgroup::BLOCK_SIZE;
+use crate::builder::Builder;
 use crate::inode::Inode;
 use crate::inode::InodeBlock;
 use crate::inode::InodeBlocksCount;
 use crate::inode::InodeNum;
 use crate::inode::InodeType;
-use crate::superblock::Config;
 use crate::superblock::SuperBlock;
 
 #[repr(C)]
@@ -149,7 +144,7 @@ pub struct Ext2<'a> {
 
 impl<'a> Ext2<'a> {
     /// Create a new ext2 filesystem.
-    fn new(cfg: &Config, arena: &'a Arena<'a>) -> Result<Self> {
+    pub(crate) fn new(cfg: &Builder, arena: &'a Arena<'a>) -> Result<Self> {
         let sb = SuperBlock::new(arena, cfg)?;
 
         let mut group_metadata = vec![];
@@ -704,7 +699,11 @@ impl<'a> Ext2<'a> {
     }
 
     /// Walks through `src_dir` and copies directories and files to the new file system.
-    fn copy_dirtree<P: AsRef<Path>>(&mut self, arena: &'a Arena<'a>, src_dir: P) -> Result<()> {
+    pub(crate) fn copy_dirtree<P: AsRef<Path>>(
+        &mut self,
+        arena: &'a Arena<'a>,
+        src_dir: P,
+    ) -> Result<()> {
         // Update the root directory's metadata with the metadata of `src_dir`.
         let root_inode_num = InodeNum::new(2).expect("2 is a valid inode number");
         let group_id = self.group_num_for_inode(root_inode_num);
@@ -767,7 +766,7 @@ impl<'a> Ext2<'a> {
         Ok(())
     }
 
-    fn copy_backup_metadata(self, arena: &'a Arena<'a>) -> Result<()> {
+    pub(crate) fn copy_backup_metadata(self, arena: &'a Arena<'a>) -> Result<()> {
         // Copy superblock and group_metadata to every block group
         for i in 1..self.sb.num_groups() as usize {
             let super_block_id = BlockId::from(self.sb.blocks_per_group * i as u32);
@@ -783,52 +782,3 @@ impl<'a> Ext2<'a> {
         Ok(())
     }
 }
-
-/// Creates a memory mapping region where an ext2 filesystem is constructed.
-pub fn create_ext2_region(cfg: &Config, src_dir: Option<&Path>) -> Result<MemoryMappingArena> {
-    let block_group_size = BLOCK_SIZE as u32 * cfg.blocks_per_group;
-    if cfg.size < block_group_size {
-        bail!(
-            "memory size {} is too small to have a block group: block_size={},  block_per_group={}",
-            cfg.size,
-            BLOCK_SIZE,
-            block_group_size
-        );
-    }
-    let mem_size = if cfg.size % block_group_size == 0 {
-        cfg.size
-    } else {
-        // Round down to the largest multiple of block_group_size that is smaller than cfg.size
-        cfg.size.next_multiple_of(block_group_size) - block_group_size
-    };
-
-    let mut mem = MemoryMappingBuilder::new(mem_size as usize).build()?;
-
-    let arena = Arena::new(BLOCK_SIZE, &mut mem)?;
-    let mut ext2 = Ext2::new(cfg, &arena)?;
-    if let Some(dir) = src_dir {
-        ext2.copy_dirtree(&arena, dir)?;
-    }
-    ext2.copy_backup_metadata(&arena)?;
-    let file_mappings = arena.into_mapping_info();
-
-    mem.msync()?;
-    let mut mmap_arena = MemoryMappingArena::from(mem);
-    for FileMappingInfo {
-        mem_offset,
-        file,
-        length,
-        file_offset,
-    } in file_mappings
-    {
-        mmap_arena.add_fd_mapping(
-            mem_offset,
-            length,
-            &file,
-            file_offset as u64, /* fd_offset */
-            Protection::read(),
-        )?;
-    }
-
-    Ok(mmap_arena)
-}

ext2/src/lib.rs

@@ -10,10 +10,10 @@
 mod arena;
 mod bitmap;
 mod blockgroup;
+mod builder;
 mod fs;
 mod inode;
 mod superblock;
 
 pub use blockgroup::BLOCK_SIZE;
-pub use fs::create_ext2_region;
-pub use superblock::Config;
+pub use builder::Builder;

ext2/src/superblock.rs

@@ -12,27 +12,10 @@ use zerocopy_derive::FromZeroes;
 use crate::arena::Arena;
 use crate::arena::BlockId;
 use crate::blockgroup::BLOCK_SIZE;
+use crate::builder::Builder;
 use crate::inode::Inode;
 
 /// A struct to represent the configuration of an ext2 filesystem.
-pub struct Config {
-    /// The number of blocks per group.
-    pub blocks_per_group: u32,
-    /// The number of inodes per group.
-    pub inodes_per_group: u32,
-    /// The size of the memory region.
-    pub size: u32,
-}
-
-impl Default for Config {
-    fn default() -> Self {
-        Self {
-            blocks_per_group: 4096,
-            inodes_per_group: 4096,
-            size: 4096 * 4096,
-        }
-    }
-}
 
 /// The ext2 superblock.
 ///
@@ -78,7 +61,7 @@ pub(crate) struct SuperBlock {
 }
 
 impl SuperBlock {
-    pub fn new<'a>(arena: &'a Arena<'a>, cfg: &Config) -> Result<&'a mut SuperBlock> {
+    pub fn new<'a>(arena: &'a Arena<'a>, cfg: &Builder) -> Result<&'a mut SuperBlock> {
         const EXT2_MAGIC_NUMBER: u16 = 0xEF53;
         const COMPAT_EXT_ATTR: u32 = 0x8;
 

ext2/tests/tests.rs

@@ -21,8 +21,7 @@ use std::path::PathBuf;
 use std::process::Command;
 
 use base::MappedRegion;
-use ext2::create_ext2_region;
-use ext2::Config;
+use ext2::Builder;
 use tempfile::tempdir;
 use tempfile::TempDir;
 use walkdir::WalkDir;
@@ -64,9 +63,15 @@ fn run_debugfs_cmd(args: &[&str], disk: &PathBuf) -> String {
     stdout.trim_start().trim_end().to_string()
 }
 
-fn mkfs(td: &TempDir, cfg: &Config, src_dir: Option<&Path>) -> PathBuf {
+fn mkfs(td: &TempDir, builder: Builder, src_dir: Option<&Path>) -> PathBuf {
     let path = td.path().join("empty.ext2");
-    let mem = create_ext2_region(cfg, src_dir).unwrap();
+    let mem = builder
+        .allocate_memory()
+        .unwrap()
+        .build_mmap_info(src_dir)
+        .unwrap()
+        .do_mmap()
+        .unwrap();
     // SAFETY: `mem` has a valid pointer and its size.
     let buf = unsafe { std::slice::from_raw_parts(mem.as_ptr(), mem.size()) };
     let mut file = OpenOptions::new()
@@ -87,7 +92,7 @@ fn test_mkfs_empty() {
     let td = tempdir().unwrap();
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 1024,
             inodes_per_group: 1024,
             ..Default::default()
@@ -113,7 +118,7 @@ fn test_mkfs_empty_multi_block_groups() {
     let num_groups = 2;
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group,
             inodes_per_group: 4096,
             size: 4096 * blocks_per_group * num_groups,
@@ -227,7 +232,7 @@ fn test_simple_dir() {
     File::create(dir.join("dir/c.txt")).unwrap();
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -261,7 +266,7 @@ fn test_nested_dirs() {
     create_dir(dir3).unwrap();
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -290,7 +295,7 @@ fn test_file_contents() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -313,7 +318,7 @@ fn test_max_file_name() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -342,7 +347,7 @@ fn test_mkfs_indirect_block() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 4096,
             inodes_per_group: 4096,
             ..Default::default()
@@ -379,7 +384,7 @@ fn test_mkfs_symlink() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -410,7 +415,7 @@ fn test_mkfs_abs_symlink() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -436,7 +441,7 @@ fn test_mkfs_symlink_to_deleted() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -477,7 +482,7 @@ fn test_mkfs_long_symlink() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -511,7 +516,7 @@ fn test_ignore_lost_found() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -569,7 +574,7 @@ fn test_multiple_block_directory_entry() {
 
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group: 2048,
             inodes_per_group: 4096,
             ..Default::default()
@@ -608,7 +613,7 @@ fn test_multiple_bg_multi_inode_bitmap() {
     let num_groups = 2;
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group,
             inodes_per_group,
             size: BLOCK_SIZE * blocks_per_group * num_groups,
@@ -647,7 +652,7 @@ fn test_multiple_bg_multi_block_bitmap() {
     let num_groups = 4;
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group,
             inodes_per_group,
             size: BLOCK_SIZE * blocks_per_group * num_groups,
@@ -685,7 +690,7 @@ fn test_multiple_bg_big_files() {
     let num_groups = 30;
     let disk = mkfs(
         &td,
-        &Config {
+        Builder {
             blocks_per_group,
             inodes_per_group: 1024,
             size: BLOCK_SIZE * blocks_per_group * num_groups,

src/crosvm/sys/linux/device_helpers.rs

@@ -1285,12 +1285,16 @@ pub fn create_pmem_ext2_device(
     index: usize,
     pmem_device_tube: Tube,
 ) -> DeviceResult {
-    let cfg = ext2::Config {
+    let builder = ext2::Builder {
         inodes_per_group: opts.inodes_per_group,
         blocks_per_group: opts.blocks_per_group,
         size: opts.size,
     };
-    let arena = ext2::create_ext2_region(&cfg, Some(opts.path.as_path()))?;
+    let arena = builder
+        .allocate_memory()?
+        .build_mmap_info(Some(opts.path.as_path()))?
+        .do_mmap()?;
+
     let mapping_size = arena.size() as u64;
 
     let mapping_address = GuestAddress(