fix(slayerfs): rename & examples #302
Conversation
Signed-off-by: Luxian <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the rename operation to support POSIX-compliant replace semantics and updates an example to use explicit type parameters for MetaStoreFactory.
Key Changes:
- Replaces simple "destination must not exist" check with full POSIX replace semantics (files/symlinks can be replaced; directories require empty destination)
- Adds explicit
DatabaseMetaStoretype parameter toMetaStoreFactoryin the S3 persistence example for API clarity
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
| project/slayerfs/src/vfs/fs.rs | Implements POSIX rename replace semantics with destination validation, empty directory checks, and automatic unlinking/rmdir before rename |
| project/slayerfs/examples/persistence_s3_demo.rs | Adds explicit type parameter to MetaStoreFactory for clarity and consistency with the factory's generic API design |
| if let Ok(Some((dest_ino, dest_kind))) = self.core.meta_layer.lookup_path(&new).await { | ||
| // resolve parent directory ino for destination | ||
| let new_dir_ino = if &new_dir == "/" { | ||
| self.core.root | ||
| } else { | ||
| // parent must exist when destination exists | ||
| self.core | ||
| .meta_layer | ||
| .lookup_path(&new_dir) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| .ok_or_else(|| "parent not found".to_string())? | ||
| .0 | ||
| }; | ||
|
|
||
| if dest_kind == FileType::Dir { | ||
| // source must be directory | ||
| if src_attr.kind != FileType::Dir { | ||
| return Err("not a directory".into()); | ||
| } | ||
|
|
||
| // ensure destination dir is empty | ||
| let children = self | ||
| .core | ||
| .meta_layer | ||
| .readdir(dest_ino) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| if !children.is_empty() { | ||
| return Err("directory not empty".into()); | ||
| } | ||
|
|
||
| // remove the empty destination directory | ||
| self.core | ||
| .meta_layer | ||
| .rmdir(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } else { | ||
| // dest is a file or symlink: unlink it to allow replace | ||
| self.core | ||
| .meta_layer | ||
| .unlink(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } | ||
| } | ||
|
|
||
| // Ensure destination parent exists (create as needed) | ||
| let new_dir_ino = self.mkdir_p(&new_dir).await?; |
There was a problem hiding this comment.
Potential race condition: There's a TOCTOU (Time-of-Check-Time-of-Use) issue between checking if the destination exists (line 827) and then calling unlink or rmdir (lines 862, 869). Another process could modify the destination between these operations. Additionally, the subsequent mkdir_p call (line 876) could also race with the destination checks. This violates the "atomic rename/replace" design principle from the slayerfs guidelines. Consider implementing this at a lower layer where atomicity can be guaranteed, or add explicit locking/transaction semantics.
| if dest_kind == FileType::Dir { | ||
| // source must be directory | ||
| if src_attr.kind != FileType::Dir { | ||
| return Err("not a directory".into()); |
There was a problem hiding this comment.
The error message "not a directory" (line 845) is ambiguous in this context. It's used when trying to rename a non-directory over a directory, but the message doesn't clarify whether it's the source or destination that's problematic. Consider a more descriptive message like "cannot replace directory with non-directory" to make debugging easier.
| return Err("not a directory".into()); | |
| return Err("cannot replace directory with non-directory".into()); |
| self.core | ||
| .meta_layer | ||
| .rmdir(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } else { | ||
| // dest is a file or symlink: unlink it to allow replace | ||
| self.core | ||
| .meta_layer | ||
| .unlink(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } |
There was a problem hiding this comment.
Missing state tracking after destination removal. When removing the destination file/directory (lines 860-871), the modified.touch() calls are not made for affected inodes. Compare with the rmdir method (lines 782-783) and the end of this rename method (lines 885-886) which properly track modifications. This could lead to cache coherence issues. Add self.state.modified.touch(dest_ino) and self.state.modified.touch(new_dir_ino) after the unlink/rmdir operations.
| let new_dir_ino = if &new_dir == "/" { | ||
| self.core.root | ||
| } else { | ||
| // parent must exist when destination exists | ||
| self.core | ||
| .meta_layer | ||
| .lookup_path(&new_dir) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| .ok_or_else(|| "parent not found".to_string())? | ||
| .0 | ||
| }; | ||
|
|
||
| if dest_kind == FileType::Dir { | ||
| // source must be directory | ||
| if src_attr.kind != FileType::Dir { | ||
| return Err("not a directory".into()); | ||
| } | ||
|
|
||
| // ensure destination dir is empty | ||
| let children = self | ||
| .core | ||
| .meta_layer | ||
| .readdir(dest_ino) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| if !children.is_empty() { | ||
| return Err("directory not empty".into()); | ||
| } | ||
|
|
||
| // remove the empty destination directory | ||
| self.core | ||
| .meta_layer | ||
| .rmdir(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } else { | ||
| // dest is a file or symlink: unlink it to allow replace | ||
| self.core | ||
| .meta_layer | ||
| .unlink(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } | ||
| } | ||
|
|
||
| // Ensure destination parent exists (create as needed) | ||
| let new_dir_ino = self.mkdir_p(&new_dir).await?; |
There was a problem hiding this comment.
Redundant lookup of destination parent. The new_dir_ino is resolved twice: once in the destination-exists block (lines 829-840) and again via mkdir_p (line 876). This is wasteful when the destination exists. Consider refactoring to look up new_dir_ino once before the destination check, or reuse the value from the first lookup if available. This would reduce unnecessary kernel round-trips, which is a key performance concern per the slayerfs guidelines.
| let src_ino = self | ||
| .core | ||
| .meta_layer | ||
| .lookup(old_parent_ino, &old_name) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| .ok_or_else(|| "not found".to_string())?; | ||
|
|
||
| let src_attr = self | ||
| .core | ||
| .meta_layer | ||
| .stat(src_ino) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| .ok_or_else(|| "not found".to_string())?; | ||
|
|
||
| // If destination exists, apply replace semantics: | ||
| // - If dest is file/symlink: unlink it | ||
| // - If dest is dir: source must be dir and dest must be empty; rmdir it | ||
| if let Ok(Some((dest_ino, dest_kind))) = self.core.meta_layer.lookup_path(&new).await { | ||
| // resolve parent directory ino for destination | ||
| let new_dir_ino = if &new_dir == "/" { | ||
| self.core.root | ||
| } else { | ||
| // parent must exist when destination exists | ||
| self.core | ||
| .meta_layer | ||
| .lookup_path(&new_dir) | ||
| .await | ||
| .map_err(|e| e.to_string())? | ||
| .ok_or_else(|| "parent not found".to_string())? | ||
| .0 | ||
| }; | ||
|
|
||
| if dest_kind == FileType::Dir { | ||
| // source must be directory | ||
| if src_attr.kind != FileType::Dir { | ||
| return Err("not a directory".into()); | ||
| } | ||
|
|
||
| // ensure destination dir is empty | ||
| let children = self | ||
| .core | ||
| .meta_layer | ||
| .readdir(dest_ino) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| if !children.is_empty() { | ||
| return Err("directory not empty".into()); | ||
| } | ||
|
|
||
| // remove the empty destination directory | ||
| self.core | ||
| .meta_layer | ||
| .rmdir(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } else { | ||
| // dest is a file or symlink: unlink it to allow replace | ||
| self.core | ||
| .meta_layer | ||
| .unlink(new_dir_ino, &new_name) | ||
| .await | ||
| .map_err(|e| e.to_string())?; | ||
| } | ||
| } | ||
|
|
||
| // Ensure destination parent exists (create as needed) | ||
| let new_dir_ino = self.mkdir_p(&new_dir).await?; |
There was a problem hiding this comment.
Missing validation: renaming a directory to be its own subdirectory. The code doesn't check if new is a subdirectory of old when both are directories. For example, renaming /a to /a/b should be rejected to prevent filesystem corruption. This is a standard POSIX rename requirement (EINVAL). Add a check to ensure the new path is not a descendant of the old path when renaming directories.
| // Resolve old parent and source inode/attributes first | ||
| let old_parent_ino = if &old_dir == "/" { |
There was a problem hiding this comment.
Missing optimization: renaming a path to itself (when old == new after normalization) should be a no-op and return success immediately. According to POSIX semantics, rename(path, path) is valid and does nothing. Add an early check like if old == new { return Ok(()); } after line 793 to avoid unnecessary operations and improve performance for this edge case.
| return Err("target exists".into()); | ||
| } | ||
|
|
||
| // Resolve old parent and source inode/attributes first |
There was a problem hiding this comment.
Missing validation: renaming anything to the root directory "/" should be rejected. The root directory is special and cannot be replaced. Add a check like if new == "/" { return Err("cannot replace root".into()); } after path normalization (after line 791) to prevent this invalid operation.
| return Err("target exists".into()); | ||
| } | ||
|
|
||
| // Resolve old parent and source inode/attributes first |
There was a problem hiding this comment.
Missing validation: renaming the root directory "/" should be rejected. Add a check like if old == "/" { return Err("cannot rename root".into()); } after path normalization (after line 791) to prevent this invalid operation, similar to the check in the rmdir method.
1 participant
No description provided.