Skip to content

Conversation

@kaleidoscope416
Copy link
Contributor

@kaleidoscope416 kaleidoscope416 commented Dec 7, 2025

PR Summary

  • Align truncate/ftruncate semantics: reject负长度,要求常规文件,非可写/非可写能力返回EINVAL,O_PATH返回EBADF,超出RLIMIT_FSIZE发送SIGXFSZ并报EFBIG。

  • 移除在 FAT resize 中意外的文件大小限制,恢复正常扩展能力。

Testing

  • gVisor truncate_test(剩余 shm 未挂载导致的打开失败需环境侧解决)。

Note

Aligns truncate/ftruncate with proper checks/signals, truncates page cache on resize, chunks zero-fills to cap memory, enforces FAT limits, and enables gVisor truncate tests.

  • VFS/syscalls:
    • ftruncate/truncate semantics aligned:
      • Validate length (len <= isize::MAX).
      • Require regular files; O_PATH -> EBADF; non-writable -> EINVAL.
      • Enforce RLIMIT_FSIZE: send SIGXFSZ, return EFBIG.
    • Centralize via vfs_truncate and add DAC write permission check in truncate.
  • FAT FS:
    • FATInode::resize: enforce MAX_FILE_SIZE with EFBIG; truncate page cache via truncate_inode_pages; expand using fixed zero buffer chunks; sync metadata.
    • FATFile::zero_range: switch to 512KB chunked zero-writes to avoid large allocations.
  • Tests:
    • Enable truncate_test in user/apps/tests/syscall/gvisor/whitelist.txt and blocklist TruncateTest.FtruncateVirtualTmp.

Written by Cursor Bugbot for commit 4f76c1e. This will update automatically on new commits. Configure here.

@github-actions github-actions bot added the Bug fix A bug is fixed in this pull request label Dec 7, 2025
@fslongjin
Copy link
Member

https://github.com/DragonOS-Community/DragonOS/actions/runs/20005297190

这里拉到底有个CR报告。
(至于为啥没comment上来,我得再看看

@kaleidoscope416
Copy link
Contributor Author

https://github.com/DragonOS-Community/DragonOS/actions/runs/20005297190

这里拉到底有个CR报告。 (至于为啥没comment上来,我得再看看

可以

@fslongjin
Copy link
Member

改进空间:

  1. 512 * 1024的缓冲区大小建议使用带文档说明的命名常量,在全局定义
    🐛 潜在缺陷与问题
    关键问题:
  2. 零字节写入静默失败(kernel/src/filesystem/fat/entry.rs:205-209)
    ◦ 当磁盘写入返回0字节时,循环中断但返回包含部分数据的Ok(write_ok)
    ◦ 建议:返回Err(SystemError::EIO)而非静默接受部分写入

次要问题:

  1. 缓冲区大小常量不一致
    ◦ ZERO_BUF_SIZE在多个位置定义(entry.rs:332, fs.rs:1763)
    ◦ 建议:在共享位置统一定义
  2. 整数溢出风险——长度验证检查len > isize::MAX as usize但未检查文件系统特定限制(MAX_FILE_SIZE)

@kaleidoscope416
Copy link
Contributor Author

主线中OpenTest.AppendConcurrentWrite 测试不稳定,有时会失败

[ RUN      ] OpenTest.AppendConcurrentWrite
test/syscalls/linux/open.cc:349: Failure
Expected equality of these values:
  st.st_size
    Which is: 49999
  kThreadCount * kBytesPerThread
    Which is: 50000

https://github.com/DragonOS-Community/DragonOS/blob/master/kernel/src/filesystem/fat/entry.rs#L203
write_at_bytes()不会返回0,不会在loop中死循环,故回退

关键问题:
2. 零字节写入静默失败(kernel/src/filesystem/fat/entry.rs:205-209)
◦ 当磁盘写入返回0字节时,循环中断但返回包含部分数据的Ok(write_ok)
◦ 建议:返回Err(SystemError::EIO)而非静默接受部分写入

@github-actions github-actions bot added the test Unitest/User space test label Dec 12, 2025
@github-actions github-actions bot removed the test Unitest/User space test label Dec 14, 2025
@fslongjin
Copy link
Member

@claude review this PR in Chinese

@github-actions github-actions bot added the test Unitest/User space test label Dec 14, 2025
@dragonos-community-ai-pr-reviewer
Copy link

dragonos-community-ai-pr-reviewer bot commented Dec 14, 2025

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


PR #1444 代码审查

我正在审查PR #1444 "fix(vfs): 修复truncate系统调用,添加截断时的页缓存截断,修复边界条件"。让我分析变更并提供反馈。

<分析>
<总结>

  • 事件类型:PR审查请求(中文)
  • 触发评论:@claude review this PR in Chinese
  • PR状态:OPEN
  • 变更文件:7个文件
  • PR摘要:修复truncate/ftruncate系统调用语义,对齐检查逻辑,添加页缓存截断,修复边界条件,启用gVisor测试
    </总结>

<关键发现>

  1. PR实现了truncate/ftruncate系统调用的语义对齐
  2. 添加了页缓存截断功能
  3. 修复了边界条件和错误处理
  4. 启用了gVisor测试
  5. 已有评论指出了一些改进空间和潜在缺陷
    </关键发现>

<审查计划>

  1. 检查每个变更文件的具体实现
  2. 验证代码逻辑的正确性
  3. 审查边界条件处理
  4. 检查常量定义和代码一致性
  5. 分析已提到的潜在问题
    </分析>

待办事项列表

  • 阅读所有变更文件,理解代码逻辑
  • 检查truncate/ftruncate系统调用实现
  • 审查FAT文件系统变更
  • 验证测试配置变更
  • 分析评论中提到的潜在问题
  • 提供完整的代码审查反馈

代码审查反馈

总体评价

这个PR很好地修复了truncate/ftruncate系统调用的语义对齐问题,增加了必要的边界检查和错误处理,实现了页缓存截断功能,并启用了gVisor测试。代码质量整体较好,但存在一些可改进的地方。

主要变更分析

1. 系统调用语义对齐 ✓

  • sys_truncate.rs: 增加了DAC写权限检查、RLIMIT_FSIZE检查、长度验证(len > isize::MAX as usize),符合Linux语义
  • sys_ftruncate.rs: 增加了长度验证,与truncate保持一致
  • file.rs: ftruncate方法中增加了O_PATH检查、非可写文件检查、RLIMIT_FSIZE检查,通过vfs_truncate统一处理

2. 页缓存截断 ✓

  • fs.rs: 在resize方法中添加了truncate_inode_pages调用,正确处理页缓存截断
  • 先清除被截断区间的缓存页,再缩容缓存大小,逻辑正确

3. FAT文件系统改进 ✓

  • fs.rs: 在resize中增加了MAX_FILE_SIZE检查,防止文件超过FAT32最大限制(4GB)
  • entry.rs: zero_range方法使用512KB缓冲区,避免大文件扩展时分配过大内存
  • 统一了缓冲区大小策略,使用固定大小的缓冲区进行零填充

4. 测试配置 ✓

  • 启用了truncate_test,将TruncateTest.FtruncateVirtualTmp加入blocklist
  • 合理的测试策略,排除已知不支持的测试用例

代码质量问题

1. 常量定义不一致 ⚠️

问题: ZERO_BUF_SIZE在多个位置重复定义:

  • kernel/src/filesystem/fat/entry.rs:326
  • kernel/src/filesystem/fat/fs.rs:1779

建议: 在entry.rsfs.rs的模块级别统一定义,并在需要的地方引用。例如:

// 在entry.rs或fs.rs的模块顶部
pub const ZERO_BUF_SIZE: usize = 512 * 1024; // 512KB

2. 整数溢出风险检查 ✓

现状:

  • sys_truncate.rssys_ftruncate.rs检查len > isize::MAX as usize
  • fs.rsresize检查len as u64 > MAX_FILE_SIZE

评估: 现有检查已覆盖主要溢出风险,但可以更明确地处理类型转换。建议在vfs_truncate中增加统一的最大长度检查。

3. 零字节写入静默失败 ⚠️

问题: 评论中提到entry.rs:205-209存在零字节写入静默失败风险
作者回复: write_at_bytes()不会返回0,不会在loop中死循环,故回退

建议: 虽然作者认为不会发生,但为健壮性考虑,可以添加防御性检查:

let w = fs.gendisk.write_at_bytes(...)?;
if w == 0 {
    // 磁盘写入返回0字节,可能是设备错误或空间不足
    return Err(SystemError::EIO);
}

4. 缓冲区大小常量缺乏文档 ⚠️

问题: 512 * 1024这样的魔术数字缺乏明确文档说明

建议: 添加详细的注释说明选择512KB的原因(内存使用与IO效率的平衡):

/// 零填充缓冲区大小(512KB),平衡内存使用和IO效率
/// - 足够大以减少系统调用次数
/// - 足够小以避免过度内存分配
pub const ZERO_BUF_SIZE: usize = 512 * 1024;

潜在改进

1. 错误处理一致性

  • 所有系统调用错误返回应保持一致的错误码映射
  • 检查所有Err(SystemError::EINVAL)的使用场景是否都符合Linux语义

2. 测试覆盖

  • 考虑添加更多边界条件测试(如RLIMIT_FSIZE、O_PATH、只读文件等场景)
  • 验证页缓存截断的正确性

3. 性能考虑

  • 大文件截断时,零填充操作可能较慢,考虑是否需要进行优化
  • 页缓存截断的锁竞争情况需要评估

安全审查

  1. 权限检查: ✓ DAC写权限检查已实现
  2. 资源限制: ✓ RLIMIT_FSIZE检查已实现,超限发送SIGXFSZ
  3. 边界检查: ✓ 长度验证防止整数溢出
  4. 文件类型检查: ✓ 只允许普通文件,目录返回EINVAL
  5. O_PATH处理: ✓ 正确返回EBADF

总结建议

批准前需要修复的问题:

  1. 统一ZERO_BUF_SIZE常量定义
  2. 为缓冲区大小常量添加详细文档

建议但不是必需的改进:

  1. 在磁盘写入返回0时添加防御性错误处理
  2. 考虑在vfs_truncate中添加统一的最大长度检查
  3. 确保所有错误码映射符合Linux语义

总体评价: 这个PR实现了重要的功能修复和安全增强,代码质量良好,建议在解决上述两个主要问题后合并。


审查者: Claude
状态: 有条件批准(需修复常量定义和文档问题)

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 test Unitest/User space test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants