Fix some issues about naming, function parameters, and comments, and redefined the method for bind mount.

Signed-off-by: Zhenchen Wang <m202372036@hust.edu.cn>

kernel/aster-nix/src/fs/path/dentry.rs

@@ -104,11 +104,12 @@ impl Dentry_ {
         DentryFlags::from_bits(flags).unwrap()
     }
 
-    /// Check if the Dentry_ is a subdir of the given Dentry_.
+    /// Check if this dentry is a descendant (child, grandchild, or
-    pub fn is_subdir(&self, dentry: Arc<Self>) -> bool {
+    /// great-grandchild, etc.) of another dentry.
+    pub fn is_descendant_of(&self, ancestor: &Arc<Self>) -> bool {
         let mut parent = self.parent();
         while let Some(p) = parent {
-            if Arc::ptr_eq(&p, &dentry) {
+            if Arc::ptr_eq(&p, ancestor) {
                 return true;
             }
             parent = p.parent();
@@ -571,10 +572,10 @@ impl Dentry {
         }
     }
 
-    /// Make this Dentry' inner to be a mountpoint,
+    /// Make this Dentry's inner to be a mountpoint,
     /// and set the mountpoint of the child mount to this Dentry's inner.
-    pub fn set_mountpoint(&self, child_mount: Arc<MountNode>) {
+    pub(super) fn set_mountpoint(&self, child_mount: Arc<MountNode>) {
-        child_mount.set_mountpoint_dentry(self.inner.clone());
+        child_mount.set_mountpoint_dentry(&self.inner);
         self.inner.set_mountpoint_dentry();
     }
 
@@ -600,7 +601,7 @@ impl Dentry {
     /// Unmount and return the mounted child mount.
     ///
     /// Note that the root mount cannot be unmounted.
-    pub fn umount(&self) -> Result<Arc<MountNode>> {
+    pub fn unmount(&self) -> Result<Arc<MountNode>> {
         if !self.inner.is_root_of_mount() {
             return_errno_with_message!(Errno::EINVAL, "not mounted");
         }
@@ -613,7 +614,7 @@ impl Dentry {
         let mountpoint_mount_node = mount_node.parent().unwrap().upgrade().unwrap();
         let mountpoint = Self::new(mountpoint_mount_node.clone(), mountpoint_dentry.clone());
 
-        let child_mount = mountpoint_mount_node.umount(&mountpoint)?;
+        let child_mount = mountpoint_mount_node.unmount(&mountpoint)?;
         mountpoint_dentry.clear_mountpoint();
         Ok(child_mount)
     }
@@ -650,12 +651,17 @@ impl Dentry {
         self.inner.rename(old_name, &new_dir.inner, new_name)
     }
 
-    pub fn do_loopback(&self, recursive: bool) -> Arc<MountNode> {
+    /// Bind mount the Dentry to the destination Dentry.
-        if recursive {
+    ///
-            self.mount_node.copy_mount_node_tree(self.inner.clone())
+    /// If recursive is true, it will bind mount the whole mount tree
-        } else {
+    /// to the destination Dentry. Otherwise, it will only bind mount
-            self.mount_node.clone_mount_node(self.inner.clone())
+    /// the root mount node.
-        }
+    pub fn bind_mount_to(&self, dst_dentry: &Arc<Self>, recursive: bool) -> Result<()> {
+        let src_mount = self
+            .mount_node
+            .clone_mount_node_tree(&self.inner, recursive);
+        src_mount.graft_mount_node_tree(dst_dentry)?;
+        Ok(())
     }
 
     /// Get the arc reference to self.

kernel/aster-nix/src/fs/path/mount.rs

@@ -84,7 +84,7 @@ impl MountNode {
     /// Unmount a child mount node from the mountpoint and return it.
     ///
     /// The mountpoint should belong to this mount node, or an error is returned.
-    pub fn umount(&self, mountpoint: &Dentry) -> Result<Arc<Self>> {
+    pub fn unmount(&self, mountpoint: &Dentry) -> Result<Arc<Self>> {
         if !Arc::ptr_eq(mountpoint.mount_node(), &self.this()) {
             return_errno_with_message!(Errno::EINVAL, "mountpoint not belongs to this");
         }
@@ -97,12 +97,13 @@ impl MountNode {
         Ok(child_mount)
     }
 
-    /// Clone a mount node with the an root Dentry_.
+    /// Clone a mount node with the an root `Dentry_`.
+    ///
     /// The new mount node will have the same fs as the original one and
     /// have no parent and children. We should set the parent and children manually.
-    pub fn clone_mount_node(&self, root_dentry: Arc<Dentry_>) -> Arc<Self> {
+    fn clone_mount_node(&self, root_dentry: &Arc<Dentry_>) -> Arc<Self> {
         Arc::new_cyclic(|weak_self| Self {
-            root_dentry,
+            root_dentry: root_dentry.clone(),
             mountpoint_dentry: RwLock::new(None),
             parent: RwLock::new(None),
             children: Mutex::new(BTreeMap::new()),
@@ -111,37 +112,46 @@ impl MountNode {
         })
     }
 
-    /// Copies a mount tree starting from the specified root `Dentry_`.
+    /// Clone a mount tree starting from the specified root `Dentry_`.
+    ///
     /// The new mount tree will replicate the structure of the original tree.
     /// The new tree is a separate entity rooted at the given `Dentry_`,
     /// and the original tree remains unchanged.
-    pub fn copy_mount_node_tree(&self, root_dentry: Arc<Dentry_>) -> Arc<Self> {
+    ///
+    /// If `recursive` is set to `true`, the entire tree will be copied.
+    /// Otherwise, only the root mount node will be copied.
+    pub(super) fn clone_mount_node_tree(
+        &self,
+        root_dentry: &Arc<Dentry_>,
+        recursive: bool,
+    ) -> Arc<Self> {
         let new_root_mount = self.clone_mount_node(root_dentry);
+        if !recursive {
+            return new_root_mount.clone();
+        }
         let mut stack = vec![self.this()];
         let mut new_stack = vec![new_root_mount.clone()];
 
-        let mut dentry = new_root_mount.root_dentry.clone();
         while let Some(old_mount) = stack.pop() {
             let new_parent_mount = new_stack.pop().unwrap().clone();
             let old_children = old_mount.children.lock();
             for old_child_mount in old_children.values() {
                 let mountpoint_dentry = old_child_mount.mountpoint_dentry().unwrap();
-                if !dentry.is_subdir(mountpoint_dentry.clone()) {
+                if !mountpoint_dentry.is_descendant_of(old_mount.root_dentry()) {
                     continue;
                 }
-                stack.push(old_child_mount.clone());
                 let new_child_mount =
-                    old_child_mount.clone_mount_node(old_child_mount.root_dentry.clone());
+                    old_child_mount.clone_mount_node(old_child_mount.root_dentry());
                 let key = mountpoint_dentry.key();
                 new_parent_mount
                     .children
                     .lock()
                     .insert(key, new_child_mount.clone());
-                new_child_mount.set_parent(new_parent_mount.clone());
+                new_child_mount.set_parent(&new_parent_mount);
                 new_child_mount
-                    .set_mountpoint_dentry(old_child_mount.mountpoint_dentry().unwrap().clone());
+                    .set_mountpoint_dentry(&old_child_mount.mountpoint_dentry().unwrap());
+                stack.push(old_child_mount.clone());
                 new_stack.push(new_child_mount.clone());
-                dentry = new_child_mount.root_dentry.clone();
             }
         }
         new_root_mount.clone()
@@ -159,24 +169,24 @@ impl MountNode {
     }
 
     /// Attach the mount node to the mountpoint.
-    fn attach_mount_node(&self, mountpoint: Arc<Dentry>) {
+    fn attach_mount_node(&self, mountpoint: &Arc<Dentry>) {
         let key = mountpoint.key();
         mountpoint
             .mount_node()
             .children
             .lock()
             .insert(key, self.this());
-        self.set_parent(mountpoint.mount_node().clone());
+        self.set_parent(mountpoint.mount_node());
         mountpoint.set_mountpoint(self.this());
     }
 
     /// Graft the mount node tree to the mountpoint.
-    pub fn graft_mount_node_tree(&self, mountpoint: Arc<Dentry>) -> Result<()> {
+    pub fn graft_mount_node_tree(&self, mountpoint: &Arc<Dentry>) -> Result<()> {
         if mountpoint.type_() != InodeType::Dir {
             return_errno!(Errno::ENOTDIR);
         }
         self.detach_mount_node();
-        self.attach_mount_node(mountpoint.clone());
+        self.attach_mount_node(mountpoint);
         Ok(())
     }
 
@@ -188,12 +198,12 @@ impl MountNode {
         self.children.lock().get(&mountpoint.key()).cloned()
     }
 
-    /// Get the root Dentry_ of this mount node.
+    /// Get the root `Dentry_` of this mount node.
     pub fn root_dentry(&self) -> &Arc<Dentry_> {
         &self.root_dentry
     }
 
-    /// Try to get the mountpoint Dentry_ of this mount node.
+    /// Try to get the mountpoint `Dentry_` of this mount node.
     pub fn mountpoint_dentry(&self) -> Option<Arc<Dentry_>> {
         self.mountpoint_dentry.read().clone()
     }
@@ -202,9 +212,9 @@ impl MountNode {
     ///
     /// In some cases we may need to reset the mountpoint of
     /// the created MountNode, such as move mount.
-    pub fn set_mountpoint_dentry(&self, inner: Arc<Dentry_>) {
+    pub fn set_mountpoint_dentry(&self, inner: &Arc<Dentry_>) {
         let mut mountpoint_dentry = self.mountpoint_dentry.write();
-        *mountpoint_dentry = Some(inner);
+        *mountpoint_dentry = Some(inner.clone());
     }
 
     /// Flushes all pending filesystem metadata and cached file data to the device.
@@ -228,9 +238,9 @@ impl MountNode {
     ///
     /// In some cases we may need to reset the parent of
     /// the created MountNode, such as move mount.
-    pub fn set_parent(&self, mount_node: Arc<MountNode>) {
+    pub fn set_parent(&self, mount_node: &Arc<MountNode>) {
         let mut parent = self.parent.write();
-        *parent = Some(Arc::downgrade(&mount_node));
+        *parent = Some(Arc::downgrade(mount_node));
     }
 
     /// Get strong reference to self.

kernel/aster-nix/src/syscall/arch/x86.rs

@@ -228,7 +228,7 @@ impl_syscall_nums_and_dispatch_fn! {
     SYS_CHROOT = 161           => sys_chroot(args[..1]);
     SYS_SYNC = 162             => sys_sync(args[..0]);
     SYS_MOUNT = 165            => sys_mount(args[..5]);
-    SYS_UMOUNT = 166           => sys_umount(args[..2]);
+    SYS_UMOUNT2 = 166           => sys_umount(args[..2]);
     SYS_GETTID = 186           => sys_gettid(args[..0]);
     SYS_TIME = 201             => sys_time(args[..1]);
     SYS_FUTEX = 202            => sys_futex(args[..6]);

kernel/aster-nix/src/syscall/mount.rs

@@ -14,10 +14,10 @@ use crate::{
     util::read_cstring_from_user,
 };
 
-/// The data argument is interpreted by the different filesystems.
+/// The `data` argument is interpreted by the different filesystems.
 /// Typically it is a string of comma-separated options understood by
 /// this filesystem. The current implementation only considers the case
-/// where it is NULL. Because it should be interpreted by the specific filesystems.
+/// where it is `NULL`. Because it should be interpreted by the specific filesystems.
 pub fn sys_mount(
     devname_addr: Vaddr,
     dirname_addr: Vaddr,
@@ -34,7 +34,7 @@ pub fn sys_mount(
     );
 
     let current = current!();
-    let target_dentry = {
+    let dst_dentry = {
         let dirname = dirname.to_string_lossy();
         if dirname.is_empty() {
             return_errno_with_message!(Errno::ENOENT, "dirname is empty");
@@ -48,9 +48,9 @@ pub fn sys_mount(
     } else if mount_flags.contains(MountFlags::MS_REMOUNT) {
         do_remount()?;
     } else if mount_flags.contains(MountFlags::MS_BIND) {
-        do_loopback(
+        do_bind_mount(
-            devname.clone(),
+            devname,
-            target_dentry.clone(),
+            dst_dentry,
             mount_flags.contains(MountFlags::MS_REC),
         )?;
     } else if mount_flags.contains(MountFlags::MS_SHARED)
@@ -60,69 +60,69 @@ pub fn sys_mount(
     {
         do_change_type()?;
     } else if mount_flags.contains(MountFlags::MS_MOVE) {
-        do_move_mount_old(devname, target_dentry)?;
+        do_move_mount_old(devname, dst_dentry)?;
     } else {
-        do_new_mount(devname, fstype_addr, target_dentry)?;
+        do_new_mount(devname, fstype_addr, dst_dentry)?;
     }
 
     Ok(SyscallReturn::Return(0))
 }
 
 fn do_reconfigure_mnt() -> Result<()> {
-    todo!()
+    return_errno_with_message!(Errno::EINVAL, "do_reconfigure_mnt is not supported");
 }
 
 fn do_remount() -> Result<()> {
-    todo!()
+    return_errno_with_message!(Errno::EINVAL, "do_remount is not supported");
 }
 
-/// Bind a mount to a new location.
+/// Bind a mount to a dst location.
-fn do_loopback(old_name: CString, new_dentry: Arc<Dentry>, recursive: bool) -> Result<()> {
+///
+/// If recursive is true, then bind the mount recursively.
+/// Such as use user command `mount --rbind src dst`.
+fn do_bind_mount(src_name: CString, dst_dentry: Arc<Dentry>, recursive: bool) -> Result<()> {
     let current = current!();
-    let old_dentry = {
+    let src_dentry = {
-        let old_name = old_name.to_string_lossy();
+        let src_name = src_name.to_string_lossy();
-        if old_name.is_empty() {
+        if src_name.is_empty() {
-            return_errno_with_message!(Errno::ENOENT, "old_name is empty");
+            return_errno_with_message!(Errno::ENOENT, "src_name is empty");
         }
-        let fs_path = FsPath::new(AT_FDCWD, old_name.as_ref())?;
+        let fs_path = FsPath::new(AT_FDCWD, src_name.as_ref())?;
         current.fs().read().lookup(&fs_path)?
     };
 
-    if old_dentry.type_() != InodeType::Dir {
+    if src_dentry.type_() != InodeType::Dir {
-        return_errno_with_message!(Errno::ENOTDIR, "old_name must be directory");
+        return_errno_with_message!(Errno::ENOTDIR, "src_name must be directory");
     };
 
-    let new_mount = old_dentry.do_loopback(recursive);
+    src_dentry.bind_mount_to(&dst_dentry, recursive)?;
-    new_mount.graft_mount_node_tree(new_dentry.clone())?;
     Ok(())
 }
 
 fn do_change_type() -> Result<()> {
-    todo!()
+    return_errno_with_message!(Errno::EINVAL, "do_change_type is not supported");
 }
 
-/// Move a mount from old location to new location.
+/// Move a mount from src location to dst location.
-fn do_move_mount_old(old_name: CString, new_dentry: Arc<Dentry>) -> Result<()> {
+fn do_move_mount_old(src_name: CString, dst_dentry: Arc<Dentry>) -> Result<()> {
     let current = current!();
-    let old_dentry = {
+    let src_dentry = {
-        let old_name = old_name.to_string_lossy();
+        let src_name = src_name.to_string_lossy();
-        if old_name.is_empty() {
+        if src_name.is_empty() {
-            return_errno_with_message!(Errno::ENOENT, "old_name is empty");
+            return_errno_with_message!(Errno::ENOENT, "src_name is empty");
         }
-        let fs_path = FsPath::new(AT_FDCWD, old_name.as_ref())?;
+        let fs_path = FsPath::new(AT_FDCWD, src_name.as_ref())?;
         current.fs().read().lookup(&fs_path)?
     };
 
-    if !old_dentry.is_root_of_mount() {
+    if !src_dentry.is_root_of_mount() {
-        return_errno_with_message!(Errno::EINVAL, "old_name can not be moved");
+        return_errno_with_message!(Errno::EINVAL, "src_name can not be moved");
     };
-    if old_dentry.mount_node().parent().is_none() {
+    if src_dentry.mount_node().parent().is_none() {
-        return_errno_with_message!(Errno::EINVAL, "old_name can not be moved");
+        return_errno_with_message!(Errno::EINVAL, "src_name can not be moved");
     }
 
-    old_dentry
+    src_dentry.mount_node().graft_mount_node_tree(&dst_dentry)?;
-        .mount_node()
-        .graft_mount_node_tree(new_dentry.clone())?;
 
     Ok(())
 }
@@ -165,27 +165,27 @@ fn get_fs(fs_type: CString, devname: CString) -> Result<Arc<dyn FileSystem>> {
 
 bitflags! {
     struct MountFlags: u32 {
-        const MS_RDONLY = 1;              /* Mount read-only */
+        const MS_RDONLY        =   1 << 0;       // Mount read-only */
-        const MS_NOSUID = 1<<1;           /* Ignore suid and sgid bits */
+        const MS_NOSUID        =   1 << 1;       // Ignore suid and sgid bits */
-        const MS_NODEV = 1<<2;            /* Disallow access to device special files */
+        const MS_NODEV         =   1 << 2;       // Disallow access to device special files */
-        const MS_NOEXEC = 1<<3;           /* Disallow program execution */
+        const MS_NOEXEC        =   1 << 3;       // Disallow program execution */
-        const MS_SYNCHRONOUS = 1<<4;      /* Writes are synced at once */
+        const MS_SYNCHRONOUS   =   1 << 4;       // Writes are synced at once
-        const MS_REMOUNT = 1<<5;          /* Alter flags of a mounted FS */
+        const MS_REMOUNT       =   1 << 5;       // Alter flags of a mounted FS.
-        const MS_MANDLOCK = 1<<6;         /* Allow mandatory locks on an FS */
+        const MS_MANDLOCK      =   1 << 6;       // Allow mandatory locks on an FS.
-        const MS_DIRSYNC = 1<<7;          /* Directory modifications are synchronous */
+        const MS_DIRSYNC       =   1 << 7;       // Directory modifications are synchronous
-        const MS_NOSYMFOLLOW = 1<<8;      /* Do not follow symlinks */
+        const MS_NOSYMFOLLOW   =   1 << 8;       // Do not follow symlinks.
-        const MS_NOATIME = 1<<10;         /* Do not update access times. */
+        const MS_NOATIME       =   1 << 10;      // Do not update access times.
-        const MS_NODIRATIME = 1<<11;      /* Do not update directory access times. */
+        const MS_NODIRATIME    =   1 << 11;      // Do not update directory access times.
-        const MS_BIND = 1<<12;            /* Bind directory at different place. */
+        const MS_BIND          =   1 << 12;      // Bind directory at different place.
-        const MS_MOVE = 1<<13;            /* Move mount from old to new. */
+        const MS_MOVE          =   1 << 13;      // Move mount from old to new.
-        const MS_REC = 1<<14;             /* Create recursive mount. */
+        const MS_REC           =   1 << 14;      // Create recursive mount.
-        const MS_SILENT = 1<<15;          /* Suppress certain messages in kernel log. */
+        const MS_SILENT        =   1 << 15;      // Suppress certain messages in kernel log.
-        const MS_POSIXACL = 1<<16;        /* VFS does not apply the umask. */
+        const MS_POSIXACL      =   1 << 16;      // VFS does not apply the umask.
-        const MS_UNBINDABLE = 1<<17;      /* Change to unbindable. */
+        const MS_UNBINDABLE    =   1 << 17;      // Change to unbindable.
-        const MS_PRIVATE = 1<<18; 	      /* Change to private. */
+        const MS_PRIVATE       =   1 << 18; 	 // Change to private.
-        const MS_SLAVE = 1<<19;           /* Change to slave. */
+        const MS_SLAVE         =   1 << 19;      // Change to slave.
-        const MS_SHARED = 1<<20;          /* Change to shared. */
+        const MS_SHARED        =   1 << 20;      // Change to shared.
-        const MS_RELATIME = 1<<21; 	      /* Update atime relative to mtime/ctime. */
+        const MS_RELATIME      =   1 << 21; 	 // Update atime relative to mtime/ctime.
-        const MS_KERNMOUNT = 1<<22;       /* This is a kern_mount call. */
+        const MS_KERNMOUNT     =   1 << 22;      // This is a kern_mount call.
     }
 }

kernel/aster-nix/src/syscall/umount.rs

@@ -28,17 +28,17 @@ pub fn sys_umount(path_addr: Vaddr, flags: u64) -> Result<SyscallReturn> {
         current.fs().read().lookup(&fs_path)?
     };
 
-    target_dentry.umount()?;
+    target_dentry.unmount()?;
 
     Ok(SyscallReturn::Return(0))
 }
 
 bitflags! {
     struct UmountFlags: u32 {
-        const MNT_FORCE = 0x00000001;	/* Attempt to forcibily umount */
+        const MNT_FORCE       = 0x00000001;	// Attempt to forcibily umount.
-        const MNT_DETACH = 0x00000002;	/* Just detach from the tree */
+        const MNT_DETACH      = 0x00000002;	// Just detach from the tree.
-        const MNT_EXPIRE = 0x00000004;	/* Mark for expiry */
+        const MNT_EXPIRE      = 0x00000004;	// Mark for expiry.
-        const UMOUNT_NOFOLLOW = 0x00000008;	/* Don't follow symlink on umount */
+        const UMOUNT_NOFOLLOW = 0x00000008;	// Don't follow symlink on umount.
     }
 }