Skip to content

Conversation

xboHodx
Copy link
Contributor

@xboHodx xboHodx commented Aug 10, 2025

  • 在 rename 前检查目标路径是否存在文件并删除
  • 改进 truncate_complete_page 函数,从页面回收器中移除当前页面,避免后续访问已释放的inode
  • 在脏页回写函数中增加清理页面映射、清除脏位的逻辑
  • lru脏页刷新时收集孤儿页面并处理
  • rename_file_in_current_dir 和 move_to_another_dir函数中增加在缓存中更新 inode 的名称和父目录引用的逻辑

@github-actions github-actions bot added the Bug fix A bug is fixed in this pull request label Aug 10, 2025
@fslongjin fslongjin requested a review from Copilot August 10, 2025 09:24
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a critical issue with the mv command not being able to move files to overwrite existing files. The main changes focus on improving file system consistency and memory management when handling file operations.

  • Adds file overwrite functionality by checking and deleting target files before rename operations
  • Improves memory management by removing pages from the page reclaimer when truncating and handling orphaned pages
  • Updates inode metadata (name and parent directory references) in the cache during file moves

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File Description
kernel/src/mm/truncate.rs Removes pages from page reclaimer to prevent access to freed inodes
kernel/src/mm/page.rs Refactors page writeback with improved error handling and orphaned page cleanup
kernel/src/filesystem/vfs/syscall/rename_utils.rs Adds target file deletion before rename to enable overwrite functionality
kernel/src/filesystem/fat/fs.rs Updates cached inode metadata during rename operations to maintain consistency

.lookup_follow_symlink(new_parent_path.unwrap_or("/"), VFS_MAX_FOLLOW_SYMLINK_TIMES)?;

// 检查目标文件是否存在,如果存在则删除(实现覆盖功能)
if new_parent_inode.find(new_filename).is_ok() {
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

The code calls find() on the new_parent_inode and then immediately calls do_unlink_at() which likely performs another lookup. Consider storing the result of find() to avoid duplicate lookups.

Suggested change
if new_parent_inode.find(new_filename).is_ok() {
let target_exists = new_parent_inode.find(new_filename).is_ok();
if target_exists {

Copilot uses AI. Check for mistakes.

let mapper = &mut as_guard.user_mapper.utable;

// 安全地获取虚拟地址
if let Ok(virt) = vma.lock_irqsave().page_address(page_index) {
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

The vma.lock_irqsave() is called twice in this function for the same VMA - once here and once earlier at line 340. Consider storing the VMA guard to avoid redundant lock acquisitions.

Suggested change
if let Ok(virt) = vma.lock_irqsave().page_address(page_index) {
if let Ok(virt) = vma_guard.page_address(page_index) {

Copilot uses AI. Check for mistakes.

} else {
unsafe {
// 保护位设为只读,安全处理可能的错误
if let Some(entry) = mapper.get_entry(virt, 0) {
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

[nitpick] The magic number 0 is used as a parameter to get_entry(). Consider defining a named constant to clarify what this parameter represents.

Suggested change
if let Some(entry) = mapper.get_entry(virt, 0) {
if let Some(entry) = mapper.get_entry(virt, GET_ENTRY_DEFAULT_FLAGS) {

Copilot uses AI. Check for mistakes.

let self_ptr = self as *const LockedFATInode;
let other_ptr = other as *const LockedFATInode;
if self_ptr == other_ptr {
return self.rename_file_in_current_dir(old_name, new_name);
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

Using raw pointer comparison to check if two Arc references point to the same object is unsafe and unclear. Consider using Arc::ptr_eq() or comparing the underlying data addresses in a safer way.

Suggested change
return self.rename_file_in_current_dir(old_name, new_name);
let self_arc = self.0.lock().self_ref.upgrade();
let other_arc = other.0.lock().self_ref.upgrade();
if let (Some(self_arc), Some(other_arc)) = (&self_arc, &other_arc) {
if Arc::ptr_eq(self_arc, other_arc) {
return self.rename_file_in_current_dir(old_name, new_name);
}

Copilot uses AI. Check for mistakes.

let self_ptr = self as *const LockedFATInode;
let other_ptr = other as *const LockedFATInode;
if self_ptr == other_ptr {
return self.rename_file_in_current_dir(old_name, new_name);
Copy link

Copilot AI Aug 10, 2025

Choose a reason for hiding this comment

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

Using raw pointer comparison to check if two Arc references point to the same object is unsafe and unclear. Consider using Arc::ptr_eq() or comparing the underlying data addresses in a safer way.

Suggested change
return self.rename_file_in_current_dir(old_name, new_name);
// Use Arc::ptr_eq to safely check if both directories are the same
let self_arc = self.0.lock().self_ref.upgrade();
let other_arc = other.0.lock().self_ref.upgrade();
if let (Some(self_arc), Some(other_arc)) = (&self_arc, &other_arc) {
if Arc::ptr_eq(self_arc, other_arc) {
return self.rename_file_in_current_dir(old_name, new_name);
}

Copilot uses AI. Check for mistakes.

Comment on lines +34 to 37
// 从页面回收器中移除该页面,避免后续访问已释放的inode
let paddr = page.phys_address();
page_reclaimer_lock_irqsave().remove_page(&paddr);
}
Copy link
Member

Choose a reason for hiding this comment

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

这里这样删除可能会导致内存泄漏,因为:
页面还在page cache里面,然后这里就把他从page reclaimer里面删掉了。这样不合理。
要删除页面请参考shrink_list函数。

并且同步的删除page cache其实性能很差。我认为一种可行的解决方案就是,文件删除之后,标记其page cache为dead的了,然后页面回收器的线程异步回收。

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

Labels

Bug fix A bug is fixed in this pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants