From b025a30d271458386769e8721231e1e219481f57 Mon Sep 17 00:00:00 2001 From: Florian Klink Date: Mon, 15 Apr 2024 15:14:03 +0300 Subject: refactor(tvix/castore/fs): remove From for InodeData MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit These were copying unnecessarily. Instead, have a InodeData::from_node(), which *consumes* the Node entirely, returns `InodeData` and the split-off name (which is not part of InodeData). Callers can then use the result in various helper functions, like: - InodeData::as_fuse_type - InodeData::as_fuse_file_attr - InodeData::as_fuse_entry … to prepare their replies to the kernel. This removes not only a bunch of clones, but also a lot of copy-pasted code. Change-Id: Idbca5f25cc29e96c1f4c614b33dff2becb0a8738 Reviewed-on: https://cl.tvl.fyi/c/depot/+/11435 Tested-by: BuildkiteCI Autosubmit: flokli Reviewed-by: Connor Brewster --- tvix/castore/src/fs/inodes.rs | 86 +++++++++++++++++++------------------- tvix/castore/src/fs/mod.rs | 97 +++++++++++++++---------------------------- 2 files changed, 76 insertions(+), 107 deletions(-) (limited to 'tvix/castore/src') diff --git a/tvix/castore/src/fs/inodes.rs b/tvix/castore/src/fs/inodes.rs index 69a75acb6b0d..c22bd4b2ebdc 100644 --- a/tvix/castore/src/fs/inodes.rs +++ b/tvix/castore/src/fs/inodes.rs @@ -2,6 +2,8 @@ //! about inodes, which present tvix-castore nodes in a filesystem. use std::time::Duration; +use bytes::Bytes; + use crate::proto as castorepb; use crate::B3Digest; @@ -12,7 +14,36 @@ pub enum InodeData { Directory(DirectoryInodeData), // either [DirectoryInodeData:Sparse] or [DirectoryInodeData:Populated] } +/// This encodes the two different states of [InodeData::Directory]. +/// Either the data still is sparse (we only saw a [castorepb::DirectoryNode], +/// but didn't fetch the [castorepb::Directory] struct yet, or we processed a +/// lookup and did fetch the data. +#[derive(Clone, Debug)] +pub enum DirectoryInodeData { + Sparse(B3Digest, u64), // digest, size + Populated(B3Digest, Vec<(u64, castorepb::node::Node)>), // [(child_inode, node)] +} + impl InodeData { + /// Constructs a new InodeData by consuming a [Node]. + /// It splits off the orginal name, so it can be used later. + pub fn from_node(node: castorepb::node::Node) -> (Self, Bytes) { + match node { + castorepb::node::Node::Directory(n) => ( + Self::Directory(DirectoryInodeData::Sparse( + n.digest.try_into().unwrap(), + n.size, + )), + n.name, + ), + castorepb::node::Node::File(n) => ( + Self::Regular(n.digest.try_into().unwrap(), n.size, n.executable), + n.name, + ), + castorepb::node::Node::Symlink(n) => (Self::Symlink(n.target), n.name), + } + } + pub fn as_fuse_file_attr(&self, inode: u64) -> fuse_backend_rs::abi::fuse_abi::Attr { fuse_backend_rs::abi::fuse_abi::Attr { ino: inode, @@ -45,50 +76,19 @@ impl InodeData { ..Default::default() } } -} - -/// This encodes the two different states of [InodeData::Directory]. -/// Either the data still is sparse (we only saw a [castorepb::DirectoryNode], -/// but didn't fetch the [castorepb::Directory] struct yet, or we processed a -/// lookup and did fetch the data. -#[derive(Clone, Debug)] -pub enum DirectoryInodeData { - Sparse(B3Digest, u64), // digest, size - Populated(B3Digest, Vec<(u64, castorepb::node::Node)>), // [(child_inode, node)] -} - -impl From<&castorepb::node::Node> for InodeData { - fn from(value: &castorepb::node::Node) -> Self { - match value { - castorepb::node::Node::Directory(directory_node) => directory_node.into(), - castorepb::node::Node::File(file_node) => file_node.into(), - castorepb::node::Node::Symlink(symlink_node) => symlink_node.into(), - } - } -} - -impl From<&castorepb::SymlinkNode> for InodeData { - fn from(value: &castorepb::SymlinkNode) -> Self { - InodeData::Symlink(value.target.clone()) - } -} -impl From<&castorepb::FileNode> for InodeData { - fn from(value: &castorepb::FileNode) -> Self { - InodeData::Regular( - value.digest.clone().try_into().unwrap(), - value.size, - value.executable, - ) - } -} + /// Returns the u32 fuse type + pub fn as_fuse_type(&self) -> u32 { + #[allow(clippy::let_and_return)] + let ty = match self { + InodeData::Regular(_, _, _) => libc::S_IFREG, + InodeData::Symlink(_) => libc::S_IFLNK, + InodeData::Directory(_) => libc::S_IFDIR, + }; + // libc::S_IFDIR is u32 on Linux and u16 on MacOS + #[cfg(target_os = "macos")] + let ty = ty as u32; -/// Converts a DirectoryNode to a sparsely populated InodeData::Directory. -impl From<&castorepb::DirectoryNode> for InodeData { - fn from(value: &castorepb::DirectoryNode) -> Self { - InodeData::Directory(DirectoryInodeData::Sparse( - value.digest.clone().try_into().unwrap(), - value.size, - )) + ty } } diff --git a/tvix/castore/src/fs/mod.rs b/tvix/castore/src/fs/mod.rs index 1988a49cfc15..b714bad1e868 100644 --- a/tvix/castore/src/fs/mod.rs +++ b/tvix/castore/src/fs/mod.rs @@ -26,6 +26,7 @@ use crate::{ B3Digest, }; use bstr::ByteVec; +use bytes::Bytes; use fuse_backend_rs::abi::fuse_abi::{stat64, OpenOptions}; use fuse_backend_rs::api::filesystem::{ Context, FileSystem, FsOptions, GetxattrReply, ListxattrReply, ROOT_ID, @@ -91,7 +92,7 @@ pub struct TvixStoreFs { show_xattr: bool, /// This maps a given basename in the root to the inode we allocated for the node. - root_nodes: RwLock, u64>>, + root_nodes: RwLock>, /// This keeps track of inodes and data alongside them. inode_tracker: RwLock, @@ -196,13 +197,16 @@ where // Turn the retrieved directory into a InodeData::Directory(DirectoryInodeData::Populated(..)), // allocating inodes for the children on the way. + // FUTUREWORK: there's a bunch of cloning going on here, which we can probably avoid. let children = { let mut inode_tracker = self.inode_tracker.write(); let children: Vec<(u64, castorepb::node::Node)> = directory .nodes() .map(|child_node| { - let child_ino = inode_tracker.put((&child_node).into()); + let (inode_data, _) = InodeData::from_node(child_node.clone()); + + let child_ino = inode_tracker.put(inode_data); (child_ino, child_node) }) .collect(); @@ -263,7 +267,7 @@ where // the root node doesn't exist, so the file doesn't exist. Ok(None) => Err(io::Error::from_raw_os_error(libc::ENOENT)), // The root node does exist - Ok(Some(ref root_node)) => { + Ok(Some(root_node)) => { // The name must match what's passed in the lookup, otherwise this is also a ENOENT. if root_node.get_name() != name.to_bytes() { debug!(root_node.name=?root_node.get_name(), found_node.name=%name.to_string_lossy(), "node name mismatch"); @@ -286,9 +290,9 @@ where // insert the (sparse) inode data and register in // self.root_nodes. - let inode_data: InodeData = root_node.into(); + let (inode_data, name) = InodeData::from_node(root_node); let ino = inode_tracker.put(inode_data.clone()); - root_nodes.insert(name.to_bytes().into(), ino); + root_nodes.insert(name, ino); Ok((ino, Arc::new(inode_data))) } @@ -463,30 +467,22 @@ where io::Error::from_raw_os_error(libc::EIO) })?; - let name = root_node.get_name(); - let ty = match root_node { - Node::Directory(_) => libc::S_IFDIR, - Node::File(_) => libc::S_IFREG, - Node::Symlink(_) => libc::S_IFLNK, - }; + let (inode_data, name) = InodeData::from_node(root_node); // obtain the inode, or allocate a new one. - let ino = self.get_inode_for_root_name(name).unwrap_or_else(|| { + let ino = self.get_inode_for_root_name(&name).unwrap_or_else(|| { // insert the (sparse) inode data and register in // self.root_nodes. - let ino = self.inode_tracker.write().put((&root_node).into()); - self.root_nodes.write().insert(name.into(), ino); + let ino = self.inode_tracker.write().put(inode_data.clone()); + self.root_nodes.write().insert(name.clone(), ino); ino }); - #[cfg(target_os = "macos")] - let ty = ty as u32; - let written = add_entry(fuse_backend_rs::api::filesystem::DirEntry { ino, offset: offset + i as u64 + 1, - type_: ty, - name, + type_: inode_data.as_fuse_type(), + name: &name, })?; // If the buffer is full, add_entry will return `Ok(0)`. if written == 0 { @@ -500,23 +496,15 @@ where let (parent_digest, children) = self.get_directory_children(inode)?; Span::current().record("directory.digest", parent_digest.to_string()); - for (i, (ino, child_node)) in children.iter().skip(offset as usize).enumerate() { + for (i, (ino, child_node)) in children.into_iter().skip(offset as usize).enumerate() { + let (inode_data, name) = InodeData::from_node(child_node); + // the second parameter will become the "offset" parameter on the next call. let written = add_entry(fuse_backend_rs::api::filesystem::DirEntry { - ino: *ino, + ino, offset: offset + i as u64 + 1, - type_: match child_node { - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::Directory(_) => libc::S_IFDIR as u32, - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::File(_) => libc::S_IFREG as u32, - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::Symlink(_) => libc::S_IFLNK as u32, - }, - name: child_node.get_name(), + type_: inode_data.as_fuse_type(), + name: &name, })?; // If the buffer is full, add_entry will return `Ok(0)`. if written == 0 { @@ -566,33 +554,23 @@ where io::Error::from_raw_os_error(libc::EPERM) })?; - let name = root_node.get_name(); - let ty = match root_node { - Node::Directory(_) => libc::S_IFDIR, - Node::File(_) => libc::S_IFREG, - Node::Symlink(_) => libc::S_IFLNK, - }; - - let inode_data: InodeData = (&root_node).into(); + let (inode_data, name) = InodeData::from_node(root_node); // obtain the inode, or allocate a new one. - let ino = self.get_inode_for_root_name(name).unwrap_or_else(|| { + let ino = self.get_inode_for_root_name(&name).unwrap_or_else(|| { // insert the (sparse) inode data and register in // self.root_nodes. let ino = self.inode_tracker.write().put(inode_data.clone()); - self.root_nodes.write().insert(name.into(), ino); + self.root_nodes.write().insert(name.clone(), ino); ino }); - #[cfg(target_os = "macos")] - let ty = ty as u32; - let written = add_entry( fuse_backend_rs::api::filesystem::DirEntry { ino, offset: offset + i as u64 + 1, - type_: ty, - name, + type_: inode_data.as_fuse_type(), + name: &name, }, inode_data.as_fuse_entry(ino), )?; @@ -608,27 +586,18 @@ where let (parent_digest, children) = self.get_directory_children(inode)?; Span::current().record("directory.digest", parent_digest.to_string()); - for (i, (ino, child_node)) in children.iter().skip(offset as usize).enumerate() { - let inode_data: InodeData = child_node.into(); + for (i, (ino, child_node)) in children.into_iter().skip(offset as usize).enumerate() { + let (inode_data, name) = InodeData::from_node(child_node); + // the second parameter will become the "offset" parameter on the next call. let written = add_entry( fuse_backend_rs::api::filesystem::DirEntry { - ino: *ino, + ino, offset: offset + i as u64 + 1, - type_: match child_node { - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::Directory(_) => libc::S_IFDIR as u32, - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::File(_) => libc::S_IFREG as u32, - #[allow(clippy::unnecessary_cast)] - // libc::S_IFDIR is u32 on Linux and u16 on MacOS - Node::Symlink(_) => libc::S_IFLNK as u32, - }, - name: child_node.get_name(), + type_: inode_data.as_fuse_type(), + name: &name, }, - inode_data.as_fuse_entry(*ino), + inode_data.as_fuse_entry(ino), )?; // If the buffer is full, add_entry will return `Ok(0)`. if written == 0 { -- cgit 1.4.1