Skip to content

Conversation

@Vitus213
Copy link
Contributor

实现 fallocate 系统调用 (syscall 285),用于为文件分配磁盘空间。

  • 新增 sys_fallocate.rs 系统调用处理
  • 支持 mode=0 的默认分配操作,通过 vfs_truncate 扩展文件
  • 暂不支持 FALLOC_FL_KEEP_SIZE、FALLOC_FL_PUNCH_HOLE 等标志位

  实现 fallocate 系统调用 (syscall 285),用于为文件分配磁盘空间。

  - 新增 sys_fallocate.rs 系统调用处理
  - 支持 mode=0 的默认分配操作,通过 vfs_truncate 扩展文件
  - 暂不支持 FALLOC_FL_KEEP_SIZE、FALLOC_FL_PUNCH_HOLE 等标志位
@github-actions github-actions bot added the enhancement New feature or request label Jan 14, 2026
@Vitus213
Copy link
Contributor Author

代码审查

发现 4 个问题:

  1. 缺少 O_PATH 文件描述符检查 (参考 file.rs:1270-1274 的 ftruncate 实现)
    O_PATH 文件描述符用于仅获取文件描述符而不允许 I/O 操作,fallocate 作为修改文件分配的 I/O 操作应该拒绝 O_PATH 描述符并返回 EBADF。

    https://github.com/Vitus213/DragonOS/blob/b35fda976ac5e2ea447f7571a7dc7880917dc966/kernel/src/filesystem/vfs/syscall/sys_fallocate.rs#L62-L65

  2. 不可写文件的错误码应该是 EINVAL 而非 EBADF (参考 file.rs:1276-1279)
    ftruncate 实现中,非可写打开的文件返回 EINVAL(注释说明"对齐 gVisor 预期"),fallocate 作为类似操作应保持一致。

    https://github.com/Vitus213/DragonOS/blob/b35fda976ac5e2ea447f7571a7dc7880917dc966/kernel/src/filesystem/vfs/syscall/sys_fallocate.rs#L62-L65

  3. 缺少 RLIMIT_FSIZE 资源限制检查 (参考 file.rs:1281-1287)
    扩展文件大小的操作应该检查 RLIMIT_FSIZE,超过限制时应发送 SIGXFSZ 信号并返回 EFBIG。

    https://github.com/Vitus213/DragonOS/blob/b35fda976ac5e2ea447f7571a7dc7880917dc966/kernel/src/filesystem/vfs/syscall/sys_fallocate.rs#L67-L69

  4. 文档与实现不一致:不支持的模式应返回 EOPNOTSUPP
    第 33-34 行文档说明暂不支持的模式会返回 EOPNOTSUPP,但第 50 行实际返回的是 ENOSYS。其他 syscalls 如 sys_preadv2.rs 和 sys_pwritev2.rs 使用 EOPNOTSUPP_OR_ENOTSUP。

    https://github.com/Vitus213/DragonOS/blob/b35fda976ac5e2ea447f7571a7dc7880917dc966/kernel/src/filesystem/vfs/syscall/sys_fallocate.rs#L33-L50

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Vitus213
Copy link
Contributor Author

代码审查 (更新)

发现 1 个问题:

  1. fallocate 不应截断文件,只能扩展 (Linux man page: "The file size will be changed if offset+len is greater than the file size")

当前实现无条件地将文件截断到 ,但 Linux fallocate(mode=0) 应该只在 大于当前文件大小时才扩展文件,不应该缩小文件。缩小文件应该通过 ftruncate() 完成。

例如:如果文件当前大小为 1000 字节,调用 会错误地将文件截断到 500 字节,导致数据丢失。

let new_size = offset.saturating_add(len);
let current_pcb = ProcessManager::current_pcb();
let fsize_limit = current_pcb.get_rlimit(RLimitID::Fsize);
if fsize_limit.rlim_cur != u64::MAX && new_size as u64 > fsize_limit.rlim_cur {
let _ = send_signal_to_pid(current_pcb.raw_pid(), Signal::SIGXFSZ);
return Err(SystemError::EFBIG);
}
let r = crate::filesystem::vfs::vcore::vfs_truncate(file.inode(), new_size).map(|_| 0);

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

@Vitus213
Copy link
Contributor Author

代码审查 (更新)

发现 1 个问题:

  1. fallocate 不应截断文件,只能扩展 (Linux man page: "The file size will be changed if offset+len is greater than the file size")

当前实现无条件地将文件截断到 offset + len,但 Linux fallocate(mode=0) 应该只在 offset+len 大于当前文件大小时才扩展文件,不应该缩小文件。缩小文件应该通过 ftruncate() 完成。

例如:如果文件当前大小为 1000 字节,调用 fallocate(fd, 0, 0, 500) 会错误地将文件截断到 500 字节,导致数据丢失。

let new_size = offset.saturating_add(len);
let current_pcb = ProcessManager::current_pcb();
let fsize_limit = current_pcb.get_rlimit(RLimitID::Fsize);
if fsize_limit.rlim_cur != u64::MAX && new_size as u64 > fsize_limit.rlim_cur {
let _ = send_signal_to_pid(current_pcb.raw_pid(), Signal::SIGXFSZ);
return Err(SystemError::EFBIG);
}
let r = crate::filesystem::vfs::vcore::vfs_truncate(file.inode(), new_size).map(|_| 0);

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

    According to Linux fallocate(2) semantics, mode=0 should only
    extend the file size when offset+len exceeds the current file
    size. It should never shrink the file.
@fslongjin
Copy link
Member

@claude review

@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Jan 15, 2026

Claude finished @fslongjin's task in 4m 40s —— View job


Comprehensive Code Review

I've reviewed the fallocate system call implementation in kernel/src/filesystem/vfs/syscall/sys_fallocate.rs. The implementation addresses most of the issues raised in previous reviews, but several important improvements are needed:

Issues Already Fixed

  1. O_PATH file descriptor check - Present (lines 68-70)
  2. Non-writable file error (EINVAL) - Correct (lines 72-74)
  3. RLIMIT_FSIZE resource limit check - Present (lines 80-85)
  4. Shrinking behavior - Correctly only extends when new_size > current_size (lines 78-90)
  5. Error code consistency - Uses EOPNOTSUPP_OR_ENOTSUP (line 54) - matches other syscalls

⚠️ Remaining Issues from Previous Reviews

  1. Documentation mismatch - Lines 33-34 say "会返回 EOPNOTSUPP" but code uses EOPNOTSUPP_OR_ENOTSUP. Update docs to match.

🔍 New Issues Found

  1. Overflow handling uses saturating_add - Line 76 uses offset.saturating_add(len) which silently saturates on overflow instead of returning EINVAL. Should use checked_add:

    let new_size = offset.checked_add(len).ok_or(SystemError::EINVAL)?;
  2. Missing regular file type check - ftruncate checks file_type != FileType::File returns EINVAL. fallocate should do the same check before attempting allocation:

    let md = file.inode().metadata()?;
    if md.file_type != FileType::File {
        return Err(SystemError::EINVAL);
    }
  3. Offset validation could be stricter - While offset > isize::MAX as usize catches negative offsets (via wrap-around), explicitly checking for negative values after casting to i64 would be clearer. Consider aligning with Linux's loff_t semantics.

📝 Additional Suggestions

  • Consider adding a test case for the overflow scenario
  • Ensure error propagation from vfs_truncate is handled (currently uses map(|_| 0) which discards any returned value)
  • Verify that len == 0 returns EINVAL (already done line 57-59) matches Linux behavior (Linux returns success for len == 0? Need to check)

Overall Assessment

The implementation is structurally sound and follows the existing patterns in the codebase. With the above fixes, it will be robust and conform to Linux semantics.


  - 修正文档中的错误码描述(EOPNOTSUPP → EOPNOTSUPP_OR_ENOTSUP)
  - 修复溢出处理:使用 checked_add 替代 saturating_add,溢出时正确返回 EINVAL
  - 添加文件类型检查,只支持普通文件,与 ftruncate 行为保持一致
@fslongjin
Copy link
Member

@claude review

@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Jan 15, 2026

Claude finished @fslongjin's task in 5m 53s —— View job


Comprehensive Code Review

I've reviewed the fallocate system call implementation in kernel/src/filesystem/vfs/syscall/sys_fallocate.rs. The implementation addresses most of the issues raised in previous reviews and follows established patterns in the codebase.

Issues Already Fixed (from previous reviews)

  1. O_PATH file descriptor check - Present (lines 68-70)
  2. Non-writable file error (EINVAL) - Correct (lines 72-74)
  3. RLIMIT_FSIZE resource limit check - Present (lines 80-85)
  4. Shrinking behavior - Correctly only extends when new_size > current_size (lines 78-90)
  5. Error code consistency - Uses EOPNOTSUPP_OR_ENOTSUP (line 54) - matches other syscalls
  6. Overflow handling - Uses checked_add with EINVAL on overflow (line 81) - fixed
  7. Regular file type check - Present (lines 76-79) - fixed
  8. Documentation mismatch - Already corrected (line 38 mentions EOPNOTSUPP_OR_ENOTSUP)

⚠️ Remaining Issues from Previous Reviews

None - All previously identified issues appear to have been addressed in the current implementation.

🔍 New Issues Found

  1. Double write permission check redundancy - Lines 72-74 check both FMODE_WRITE flag and can_write() method:

    if !mode_flags.contains(FileMode::FMODE_WRITE) || !mode_flags.can_write() {
        return Err(SystemError::EINVAL);
    }

    This seems redundant. According to ftruncate implementation in file.rs:1276-1279, only FMODE_WRITE is checked. Consider removing can_write() check unless there's a specific reason for both.

  2. Signal sending error ignored - Line 88 uses let _ = send_signal_to_pid(...) which discards any error from signal delivery. While signal delivery failures are typically non-fatal, consider logging or at least noting the intent.

  3. Offset validation could be more explicit - Lines 122-127 validate offset by checking offset > isize::MAX as usize. This catches negative offsets (via wrap-around) but could be more explicit. Consider aligning with Linux's loff_t semantics if needed, though current approach matches other syscalls like ftruncate.

📝 Additional Suggestions

  • Test coverage: Consider adding test cases for edge cases:
    • Overflow scenarios (offset + len > usize::MAX)
    • RLIMIT_FSIZE limit enforcement
    • O_PATH file descriptor rejection
    • Regular file type requirement
  • Error propagation: The vfs_truncate error propagation is correct (line 92-93 uses map(|_| 0) which preserves errors).
  • Zero-length handling: Current implementation returns EINVAL for len == 0 (line 57-59). Verify this matches Linux behavior (Linux may return success for len == 0).

Overall Assessment

The implementation is structurally sound and follows the existing patterns in the codebase. With the minor improvements suggested above, it will be robust and conform to Linux semantics. The code is clean, well-documented, and integrates properly with the existing syscall infrastructure.

Ready for merge after addressing the minor issues.


Branch: fix/deleted_test
Previous job run: View previous job run

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants