From b7ae7383102800529a079f00f92178511d35b8f3 Mon Sep 17 00:00:00 2001 From: Wang Siyuan Date: Thu, 8 Jan 2026 09:48:25 +0000 Subject: [PATCH] Cleanup: Only maintain children for directory dentries --- kernel/src/fs/path/dentry.rs | 176 ++++++++++++++++++++------------- kernel/src/fs/path/mod.rs | 28 ++++-- kernel/src/fs/path/resolver.rs | 9 +- 3 files changed, 130 insertions(+), 83 deletions(-) diff --git a/kernel/src/fs/path/dentry.rs b/kernel/src/fs/path/dentry.rs index 43d1277e8..6e039903e 100644 --- a/kernel/src/fs/path/dentry.rs +++ b/kernel/src/fs/path/dentry.rs @@ -1,6 +1,9 @@ // SPDX-License-Identifier: MPL-2.0 -use core::sync::atomic::{AtomicU32, Ordering}; +use core::{ + ops::Deref, + sync::atomic::{AtomicU32, Ordering}, +}; use hashbrown::HashMap; use ostd::sync::RwMutexWriteGuard; @@ -19,8 +22,7 @@ pub(super) struct Dentry { inode: Arc, type_: InodeType, name_and_parent: NameAndParent, - // FIXME: Only maintain children for directory dentries. - children: RwMutex, + children: Option>, flags: AtomicU32, mount_count: AtomicU32, this: Weak, @@ -101,11 +103,14 @@ impl Dentry { DentryOptions::Pseudo(name_fn) => NameAndParent::Pseudo(name_fn), }; + let children = + matches!(inode.type_(), InodeType::Dir).then(|| RwMutex::new(DentryChildren::new())); + Arc::new_cyclic(|weak_self| Self { type_: inode.type_(), inode, name_and_parent, - children: RwMutex::new(DentryChildren::new()), + children, flags: AtomicU32::new(DentryFlags::empty().bits()), mount_count: AtomicU32::new(0), this: weak_self.clone(), @@ -197,17 +202,66 @@ impl Dentry { } } - /// Creates a `Dentry_` by creating a new inode of the `type_` with the `mode`. + /// Gets the absolute path name of this `Dentry` within the filesystem. + pub(super) fn path_name(&self) -> String { + let mut path_name = self.name().to_string(); + let mut current_dir = self.this(); + + while let Some(parent_dir) = current_dir.parent() { + path_name = { + let parent_name = parent_dir.name(); + if parent_name != "/" { + parent_name + "/" + &path_name + } else { + parent_name + &path_name + } + }; + current_dir = parent_dir; + } + + debug_assert!(path_name.starts_with('/') || self.is_pseudo()); + path_name + } + + pub(super) fn as_dir_dentry_or_err(&self) -> Result> { + debug_assert_eq!(self.children.is_some(), self.type_ == InodeType::Dir); + + let Some(children) = &self.children else { + return_errno_with_message!( + Errno::ENOTDIR, + "the dentry is not related to a directory inode" + ); + }; + + Ok(DirDentry { + inner: self, + children, + }) + } +} + +/// A `Dentry` wrapper that has been validated to represent a directory. +pub(super) struct DirDentry<'a> { + inner: &'a Dentry, + children: &'a RwMutex, +} + +impl Deref for DirDentry<'_> { + type Target = Dentry; + + fn deref(&self) -> &Self::Target { + self.inner + } +} + +impl DirDentry<'_> { + /// Creates a `Dentry` by creating a new inode of the `type_` with the `mode`. pub(super) fn create( &self, name: &str, type_: InodeType, mode: InodeMode, - ) -> Result> { - if self.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } - + ) -> Result> { let children = self.children.upread(); if children.contains_valid(name) { return_errno!(Errno::EEXIST); @@ -237,7 +291,7 @@ impl Dentry { // TODO: Add a right implementation to cache negative dentry. let inode = self.inode.lookup(name)?; let name = String::from(name); - let target = Self::new(inode, DentryOptions::Leaf((name.clone(), self.this()))); + let target = Dentry::new(inode, DentryOptions::Leaf((name.clone(), self.this()))); if target.is_dentry_cacheable() { children.upgrade().insert(name, target.clone()); @@ -247,11 +301,12 @@ impl Dentry { } /// Creates a `Dentry` by making an inode of the `type_` with the `mode`. - pub(super) fn mknod(&self, name: &str, mode: InodeMode, type_: MknodType) -> Result> { - if self.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } - + pub(super) fn mknod( + &self, + name: &str, + mode: InodeMode, + type_: MknodType, + ) -> Result> { let children = self.children.upread(); if children.contains_valid(name) { return_errno!(Errno::EEXIST); @@ -268,18 +323,13 @@ impl Dentry { Ok(new_child) } - /// Links a new name for the `Dentry` by `link()` the inner inode. - pub(super) fn link(&self, old: &Arc, name: &str) -> Result<()> { - if self.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } - + /// Links a new `Dentry` by `link()` the old inode. + pub(super) fn link(&self, old_inode: &Arc, name: &str) -> Result<()> { let children = self.children.upread(); if children.contains_valid(name) { return_errno!(Errno::EEXIST); } - let old_inode = old.inode(); self.inode.link(old_inode, name)?; let name = String::from(name); let dentry = Dentry::new( @@ -296,10 +346,6 @@ impl Dentry { /// Deletes a `Dentry` by `unlink()` the inner inode. pub(super) fn unlink(&self, name: &str) -> Result<()> { - if self.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } - if is_dot_or_dotdot(name) { return_errno_with_message!(Errno::EINVAL, "unlink on . or .."); } @@ -310,6 +356,7 @@ impl Dentry { let mut children = children.upgrade(); let cached_child = children.delete(name); + let dir_inode = &self.inode; let child_inode = match cached_child { Some(child) => { // Cache hit: use the cached dentry @@ -318,11 +365,11 @@ impl Dentry { None => { // Cache miss: need to lookup from the underlying filesystem drop(children); - self.inode.lookup(name)? + dir_inode.lookup(name)? } }; - self.inode.unlink(name)?; + dir_inode.unlink(name)?; let nlinks = child_inode.metadata().nlinks; fs::notify::on_link_count(&child_inode); @@ -330,7 +377,7 @@ impl Dentry { // FIXME: `DELETE_SELF` should be generated after closing the last FD. fs::notify::on_inode_removed(&child_inode); } - fs::notify::on_delete(self.inode(), &child_inode, || name.to_string()); + fs::notify::on_delete(dir_inode, &child_inode, || name.to_string()); if nlinks == 0 { // Ideally, we would use `fs_event_publisher()` here to avoid creating a // `FsEventPublisher` instance on a dying inode. However, it isn't possible because we @@ -347,10 +394,6 @@ impl Dentry { /// Deletes a directory `Dentry` by `rmdir()` the inner inode. pub(super) fn rmdir(&self, name: &str) -> Result<()> { - if self.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } - if is_dot(name) { return_errno_with_message!(Errno::EINVAL, "rmdir on ."); } @@ -364,6 +407,7 @@ impl Dentry { let mut children = children.upgrade(); let cached_child = children.delete(name); + let dir_inode = &self.inode; let child_inode = match cached_child { Some(child) => { // Cache hit: use the cached dentry @@ -372,18 +416,18 @@ impl Dentry { None => { // Cache miss: need to lookup from the underlying filesystem drop(children); - self.inode.lookup(name)? + dir_inode.lookup(name)? } }; - self.inode.rmdir(name)?; + dir_inode.rmdir(name)?; let nlinks = child_inode.metadata().nlinks; if nlinks == 0 { // FIXME: `DELETE_SELF` should be generated after closing the last FD. fs::notify::on_inode_removed(&child_inode); } - fs::notify::on_delete(self.inode(), &child_inode, || name.to_string()); + fs::notify::on_delete(dir_inode, &child_inode, || name.to_string()); if nlinks == 0 { // Ideally, we would use `fs_event_publisher()` here to avoid creating a // `FsEventPublisher` instance on a dying inode. However, it isn't possible because we @@ -399,31 +443,42 @@ impl Dentry { } /// Renames a `Dentry` to the new `Dentry` by `rename()` the inner inode. - pub(super) fn rename(&self, old_name: &str, new_dir: &Arc, new_name: &str) -> Result<()> { + pub(super) fn rename( + old_dir_arc: &Arc, + old_name: &str, + new_dir_arc: &Arc, + new_name: &str, + ) -> Result<()> { + let old_dir = old_dir_arc.as_dir_dentry_or_err()?; + let new_dir = new_dir_arc.as_dir_dentry_or_err()?; + if is_dot_or_dotdot(old_name) || is_dot_or_dotdot(new_name) { return_errno_with_message!(Errno::EISDIR, "old_name or new_name is a directory"); } - if self.type_() != InodeType::Dir || new_dir.type_() != InodeType::Dir { - return_errno!(Errno::ENOTDIR); - } + + let old_dir_inode = old_dir.inode(); + let new_dir_inode = new_dir.inode(); // The two are the same dentry, we just modify the name - if Arc::ptr_eq(&self.this(), new_dir) { + if Arc::ptr_eq(old_dir_arc, new_dir_arc) { if old_name == new_name { return Ok(()); } - let children = self.children.upread(); + let children = old_dir.children.upread(); let old_dentry = children.check_mountpoint_then_find(old_name)?; children.check_mountpoint(new_name)?; - self.inode.rename(old_name, &self.inode, new_name)?; + old_dir_inode.rename(old_name, old_dir_inode, new_name)?; let mut children = children.upgrade(); match old_dentry.as_ref() { Some(dentry) => { children.delete(old_name); - dentry.name_and_parent.set(new_name, self.this()).unwrap(); + dentry + .name_and_parent + .set(new_name, old_dir_arc.clone()) + .unwrap(); if dentry.is_dentry_cacheable() { children.insert(String::from(new_name), dentry.clone()); } @@ -435,17 +490,17 @@ impl Dentry { } else { // The two are different dentries let (mut self_children, mut new_dir_children) = - write_lock_children_on_two_dentries(self, new_dir); + write_lock_children_on_two_dentries(&old_dir, &new_dir); let old_dentry = self_children.check_mountpoint_then_find(old_name)?; new_dir_children.check_mountpoint(new_name)?; - self.inode.rename(old_name, &new_dir.inode, new_name)?; + old_dir_inode.rename(old_name, new_dir_inode, new_name)?; match old_dentry.as_ref() { Some(dentry) => { self_children.delete(old_name); dentry .name_and_parent - .set(new_name, new_dir.this()) + .set(new_name, new_dir_arc.clone()) .unwrap(); if dentry.is_dentry_cacheable() { new_dir_children.insert(String::from(new_name), dentry.clone()); @@ -458,27 +513,6 @@ impl Dentry { } Ok(()) } - - /// Gets the absolute path name of this `Dentry` within the filesystem. - pub(super) fn path_name(&self) -> String { - let mut path_name = self.name().to_string(); - let mut current_dir = self.this(); - - while let Some(parent_dir) = current_dir.parent() { - path_name = { - let parent_name = parent_dir.name(); - if parent_name != "/" { - parent_name + "/" + &path_name - } else { - parent_name + &path_name - } - }; - current_dir = parent_dir; - } - - debug_assert!(path_name.starts_with('/') || self.is_pseudo()); - path_name - } } impl Debug for Dentry { @@ -612,8 +646,8 @@ impl DentryChildren { } fn write_lock_children_on_two_dentries<'a>( - this: &'a Dentry, - other: &'a Dentry, + this: &'a DirDentry, + other: &'a DirDentry, ) -> ( RwMutexWriteGuard<'a, DentryChildren>, RwMutexWriteGuard<'a, DentryChildren>, diff --git a/kernel/src/fs/path/mod.rs b/kernel/src/fs/path/mod.rs index 18b295d77..abfe97d31 100644 --- a/kernel/src/fs/path/mod.rs +++ b/kernel/src/fs/path/mod.rs @@ -12,7 +12,7 @@ pub use resolver::{AT_FDCWD, AbsPathResult, FsPath, LookupResult, PathResolver, use crate::{ fs::{ inode_handle::InodeHandle, - path::dentry::Dentry, + path::dentry::{Dentry, DirDentry}, utils::{ CreationFlags, FileSystem, FsFlags, Inode, InodeMode, InodeType, Metadata, MknodType, OpenArgs, Permission, StatusFlags, XattrName, XattrNamespace, XattrSetFlags, @@ -61,7 +61,10 @@ impl Path { { return_errno!(Errno::EACCES); } - let new_child_dentry = self.dentry.create(name, type_, mode)?; + let new_child_dentry = self + .dentry + .as_dir_dentry_or_err()? + .create(name, type_, mode)?; Ok(Self::new(self.mount.clone(), new_child_dentry)) } @@ -391,12 +394,13 @@ impl Path { impl Path { pub fn inode(&self) -> &Arc; pub fn type_(&self) -> InodeType; - pub fn unlink(&self, name: &str) -> Result<()>; - pub fn rmdir(&self, name: &str) -> Result<()>; /// Creates a `Path` by making an inode of the `type_` with the `mode`. pub fn mknod(&self, name: &str, mode: InodeMode, type_: MknodType) -> Result { - let inner = self.dentry.mknod(name, mode, type_)?; + let inner = self + .dentry + .as_dir_dentry_or_err()? + .mknod(name, mode, type_)?; Ok(Self::new(self.mount.clone(), inner)) } @@ -406,7 +410,17 @@ impl Path { return_errno_with_message!(Errno::EXDEV, "the operation cannot cross mounts"); } - self.dentry.link(&old.dentry, name) + self.dentry.as_dir_dentry_or_err()?.link(old.inode(), name) + } + + /// Unlinks a name from the `Path`. + pub fn unlink(&self, name: &str) -> Result<()> { + self.dentry.as_dir_dentry_or_err()?.unlink(name) + } + + /// Removes a directory by `rmdir()` the inner inode. + pub fn rmdir(&self, name: &str) -> Result<()> { + self.dentry.as_dir_dentry_or_err()?.rmdir(name) } /// Renames a `Path` to the new `Path` by `rename()` the inner inode. @@ -415,7 +429,7 @@ impl Path { return_errno_with_message!(Errno::EXDEV, "the operation cannot cross mounts"); } - self.dentry.rename(old_name, &new_dir.dentry, new_name) + DirDentry::rename(&self.dentry, old_name, &new_dir.dentry, new_name) } } diff --git a/kernel/src/fs/path/resolver.rs b/kernel/src/fs/path/resolver.rs index 470d4010d..2e4afb189 100644 --- a/kernel/src/fs/path/resolver.rs +++ b/kernel/src/fs/path/resolver.rs @@ -376,9 +376,8 @@ impl core::fmt::Display for MountInfoEntry<'_> { impl PathResolver { /// Looks up a child entry with `name` within a directory `path`. pub fn lookup_at_path(&self, path: &Path, name: &str) -> Result { - if path.type_() != InodeType::Dir { - return_errno_with_message!(Errno::ENOTDIR, "the path is not a directory"); - } + let dir_dentry = path.dentry.as_dir_dentry_or_err()?; + if path.inode().check_permission(Permission::MAY_EXEC).is_err() { return_errno_with_message!(Errno::EACCES, "the path cannot be looked up"); } @@ -391,11 +390,11 @@ impl PathResolver { } else if super::is_dotdot(name) { self.resolve_parent(path).unwrap_or_else(|| path.this()) } else { - let target_inner_opt = path.dentry.lookup_via_cache(name)?; + let target_inner_opt = dir_dentry.lookup_via_cache(name)?; match target_inner_opt { Some(target_inner) => Path::new(path.mount.clone(), target_inner), None => { - let target_inner = path.dentry.lookup_via_fs(name)?; + let target_inner = dir_dentry.lookup_via_fs(name)?; Path::new(path.mount.clone(), target_inner) } }