Skip to content

fix lack of rename - #445

Open
jinwjinl wants to merge 4 commits into
vivoblueos:mainfrom
jinwjinl:fix_lack_of_rename
Open

fix lack of rename#445
jinwjinl wants to merge 4 commits into
vivoblueos:mainfrom
jinwjinl:fix_lack_of_rename

Conversation

@jinwjinl

Copy link
Copy Markdown
Contributor

Description
This commit completes rename support in the BlueOS kernel VFS. It adds the syscall entry, resolves source and destination paths through VFS, updates dcache state, and implements rename for both FATFS and tmpfs.

The external rust-fatfs library already provides Dir::rename, so no external-library modification is required.

Call path
std::fs::rename reaches the filesystem through the normal BlueOS syscall and VFS path:

Syscall number (header/src/lib.rs): Rename is appended before LastNR, preserving all existing syscall numbers.

Syscall handler (kernel/src/syscall_handlers/mod.rs): registers the rename(old_path, new_path) handler and dispatch-table entry. Builds without VFS return ENOTSUP.

Path resolution (kernel/src/vfs/syscalls.rs): validates both pointers and UTF-8 paths, resolves both parent directories with find_parent_and_name, then calls old_dir.rename(old_name, &new_dir, new_name). Errors are returned as normal errno values.

Dcache update (kernel/src/vfs/dcache.rs): validates the source entry and mount-point state, rejects an existing destination, calls the filesystem inode implementation, moves the cached child, and updates its stored name and parent. Renaming a path to itself succeeds only after confirming that the source exists.

FATFS implementation (kernel/src/vfs/fatfs.rs): FatInode::rename validates both directories and filesystem ownership, calls rust-fatfs::Dir::rename, and synchronizes the internal FatDir.children maps. Cross-directory directory moves also update the moved directory’s parent reference.

tmpfs implementation (kernel/src/vfs/tmpfs.rs): supports same-directory rename and cross-directory moves. Cross-directory moves update child maps, directory size counters, parent link counts, and the moved directory’s .. parent reference.

FAT file-handle safety
rust-fatfs requires that no live fatfs::File reference exists while its directory entry is renamed.

FatFile::internal_file is therefore stored as Option<File>. During rename:

  1. The current file handle is removed and dropped.
  2. Dir::rename updates the FAT directory entry.
  3. The file is reopened using its destination name.
  4. The reopened handle is stored back in the existing inode.

If rename fails, the original path is reopened. If reopening the destination fails, the code attempts to roll the directory entry back to its original name before returning the error.

Normal read, write, resize, and fsync paths return EIO if the internal handle is unexpectedly absent.

Error handling
The implementation returns errors instead of panicking:

  • EINVAL: "." or ".." names, invalid pointers or paths
  • ENOENT: missing source or parent directory
  • EEXIST: destination already exists
  • ENOTDIR: source or destination parent is not a directory
  • EXDEV: source and destination belong to different filesystems
  • EBUSY: source is a mount point
  • EIO: inconsistent internal FAT inode state

The newly added FATFS and tmpfs rename paths contain no unwrap() calls.

Tests
kernel/src/vfs/syscalls.rs adds coverage for:

  • Null source pointer
  • Null destination pointer
  • Missing source path

QEMU functional test:

Hello, shell!
> ls
dev/
proc/
> touch old.txt
> ls
dev/
old.txt
proc/
> rename old.txt new.txt
> ls
dev/
new.txt
proc/
>

Verification

  • qemu_riscv64.release kernel and shell rename path compiled successfully.
  • QEMU interactive rename test passed.
  • git diff --check passed.
  • Full check_all was not run.
  • A later rebuild is currently blocked by an unrelated framebuffer/libc mismatch where the current libc branch lacks the FBIOGET_FSCREENINFO, FBIOGET_VSCREENINFO, and FBIOPUT_VSCREENINFO constants.

@jinwjinl

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/31395200189.

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/31396448273.

Comment thread kernel/src/vfs/fatfs.rs
struct FatFile {
_parent: Weak<FatInode>,
internal_file: InternalFsLock<File>,
internal_file: InternalFsLock<Option<File>>,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is Option there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rust-fatfs::Dir::rename requires that no live File instance references the entry being renamed; otherwise the old File may later update the original directory entry and corrupt the filesystem.

BlueOS keeps a fatfs::File persistently inside FatInode, so Option<File> allows the rename path to take() and drop that handle before calling Dir::rename, then reopen the file under its new name and store it back afterward. If rename fails, the original file is reopened.

The None state exists only during the rename critical section while the inode write lock is held. Other file operations return EIO if this invariant is unexpectedly violated.

Using mem::replace would still require a dummy live File, so it would not satisfy the library’s rename requirement.

You can see it in https://github.com/rafalh/rust-fatfs/blob/c4b88477b22ca7e5131fbd8891f62a5deaa88e6e/src/dir.rs#L388

let (new_dir, new_name) = match path::find_parent_and_name(new_path) {
Some(result) => result,
None => return -libc::ENOENT,
};

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a file with new_path, how to handle this case

Comment thread kernel/src/vfs/tmpfs.rs Outdated
}

let mut source_inner = self.inner.write();
let mut target_inner = target.inner.write();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A deadlock may occur here (AB-BA).

Comment thread kernel/src/vfs/tmpfs.rs
child_inner.as_dir_mut().ok_or(code::EIO)?.parent = target.this.clone();
source_inner.dec_nlinks();
target_inner.inc_nlinks();
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check whether to move the dir to its subdir.

@jinwjinl

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/31561003633.

@jinwjinl

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/31561498227.

The per-call RENAME_LOCK Mutex guard duplicated the directory locking
already provided by the inode RwLock. Remove it and the now-unused
Mutex import.
@jinwjinl

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown

❌ Job failed. Failed jobs: build_and_check_boards (failure), see https://github.com/vivoblueos/kernel/actions/runs/31583311382.

@jinwjinl

Copy link
Copy Markdown
Contributor Author

build_prs

@github-actions

Copy link
Copy Markdown

@jinwjinl

Copy link
Copy Markdown
Contributor Author

Description

This change completes the VFS rename implementation for tmpfs and FATFS and addresses the reviewer feedback around existing targets, lock ordering, directory cycles, and dcache consistency.

Existing Target

When new_path already exists, rename now handles it explicitly instead of silently overwriting it:

  • Renaming to the same inode is treated as a no-op.
  • Renaming over a mount point returns EBUSY.
  • Renaming over a different existing inode returns EEXIST.
  • Tests cover the existing-target case.

This implementation intentionally uses EEXIST; replacement semantics are not introduced in this PR.

Tmpfs Locking

Cross-directory tmpfs rename previously acquired the source lock followed by the target lock, allowing concurrent inverse operations to form an AB-BA deadlock.

The new implementation:

  • Handles same-directory rename with one lock.
  • Orders two directory locks by stable inode address.
  • Uses the same ordering regardless of rename direction.
  • Moves the shared operation into rename_tmpfs_locked.
  • Updates directory size, link count, child name, and parent metadata while both directory locks are held.

Directory Cycles

Before moving a directory, dcache walks upward from the destination directory through its parents.

If the source directory is encountered, the operation returns EINVAL. This prevents operations such as:

rename("/a", "/a/b/moved")

A syscall test verifies that moving a directory into its own descendant is rejected.

Dcache Consistency

Dcache rename now updates the underlying filesystem and cache as one coordinated operation:

  • Looks up and retains the source child before locking both directory maps.
  • Verifies that the cached source entry still refers to the same child.
  • Uses stable-address ordering for source and destination children locks.
  • Validates an existing destination before invoking the inode rename.
  • Removes the old source entry and any validated target entry.
  • Updates the child’s cached name and parent.
  • Inserts the child into the destination cache when it is cacheable.
  • Handles same-directory rename separately to avoid locking the same RwLock twice.

The temporary global RENAME_LOCK was removed. Directory-level locks already provide the required synchronization, while a global lock unnecessarily serialized mount, unmount, link, and rename operations.

FATFS Open Files

FatFile::internal_file uses Option<File> because rust-fatfs cannot rename a directory entry while its file handle remains active.

During rename:

  1. The active File is taken from the Option and dropped.
  2. The directory entry is renamed.
  3. The file is reopened using its new path.
  4. If reopening fails, the rename is rolled back and the original handle is restored.

Fallible conversions now return EIO or ENOTDIR instead of panicking through unwrap.

Tests

Added coverage for:

  • Null rename paths
  • Missing source
  • Existing destination
  • Moving a directory into its descendant

RISC-V release CI completed successfully for both direct-syscall configurations, including kernel unit tests and test_procfs_posix.

@github-actions

Copy link
Copy Markdown

✅ All jobs completed successfully, see https://github.com/vivoblueos/kernel/actions/runs/31583974505.

@jinwjinl

Copy link
Copy Markdown
Contributor Author

@yyang0814 Problem found is fixed by checking and removing the existing destination before rename, enforcing a deterministic inode lock order to avoid AB-BA deadlocks, rejecting moves into the source directory’s subtree, and updating the dcache after the rename. Please review again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants