Conversation
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThis PR introduces process-level umask support for POSIX file creation, generalizes block device handling via generic BlockDriverOps traits and SharedBlockDevice wrapper, adds block device support to devfs, implements mounted mountpoint tracking, and refactors mount/umount logic. Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant VFS as VFS/Mountpoint
participant Registry as MOUNTED_MOUNTPOINTS
participant Filesystem as Target Filesystem
Client->>VFS: sys_mount(source, target, type)
VFS->>Registry: lookup_mounted_mountpoint(target)
alt already mounted
Registry-->>VFS: Some(mountpoint)
VFS-->>Client: EBUSY
else not mounted
Registry-->>VFS: None
VFS->>Filesystem: lookup_mountable_filesystem(source_candidates)
Filesystem-->>VFS: filesystem handle
VFS->>VFS: mount operation
VFS->>Registry: register_mounted_mountpoint(target, mp)
Registry-->>VFS: ok
VFS-->>Client: success
end
Client->>VFS: sys_umount2(target)
VFS->>Registry: lookup_mounted_mountpoint(target)
alt mounted
Registry-->>VFS: Some(mountpoint)
VFS->>VFS: perform unmount
VFS->>Registry: unregister_mounted_mountpoint(target)
Registry-->>VFS: true
VFS-->>Client: success
else not mounted
Registry-->>VFS: None
VFS-->>Client: EINVAL
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
Hi—could you clarify what you’d like me to adjust here or any specific concerns with the devfs changes? I’ll address them right away once I have details. |
|
To use Codex here, create a Codex account and connect to github. |
Summary
Testing
|
There was a problem hiding this comment.
Pull request overview
This PR improves PulseOS’s devfs/mount plumbing by exposing block devices under /dev/vd*, tracking mounted mountpoints more explicitly in axfs, and wiring process umask into mkdir behavior.
Changes:
- Add
/dev/vd*block device nodes indevfsand allow direct non-cached block I/O to them. - Rework mount bookkeeping by storing mounted mountpoints in
axfsand enhancing mount source matching (e.g.,/dev/vda1→ candidate lookup). - Add
umask(2)syscall support and apply umask when creating directories.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/testcode_cmd.sh | Ensure /fs exists before mounting test filesystem. |
| records/logs/2026.4.22.md | Add development log entry for /dev/vdx + mount logic changes. |
| records/logs/2026.4.21.md | Add development log entry (formatting + task stack size note). |
| records/ai-logs/Reviewer/2026-04-22_axfs_devfs_procfs_tmpfs_review.md | Add AI review log document for fs work. |
| pulse_syscalls/src/impls/misc.rs | Add sys_umask syscall implementation. |
| pulse_syscalls/src/impls/fs/path.rs | Apply umask to mkdirat, revise mount/umount logic and source resolution. |
| pulse_syscalls/src/handler.rs | Dispatch Sysno::umask to implementation. |
| pulse_core/src/task/process.rs | Add per-process umask storage + accessors and inheritance on fork. |
| pulse_core/src/fd_table.rs | Remove verbose read_dirents64 logging. |
| arceos/modules/axfs/src/lib.rs | Register mounted mountpoints; register mountable devices under both deviceN and /dev/vdX; pass block devices into devfs. |
| arceos/modules/axfs/src/highlevel/file.rs | Treat block devices as non-cacheable for open options. |
| arceos/modules/axfs/src/fs/mod.rs | Generalize block-device filesystem creation over BlockDriverOps; pass block device specs to devfs. |
| arceos/modules/axfs/src/fs/fat/fs.rs | Update FAT fs constructor to accept generic BlockDriverOps. |
| arceos/modules/axfs/src/fs/ext4/mod.rs | Generic-ize ext4 disk wrapper over BlockDriverOps. |
| arceos/modules/axfs/src/fs/ext4/inode.rs | Formatting/cleanup; no functional change intended. |
| arceos/modules/axfs/src/fs/ext4/fs.rs | Update ext4 fs constructor to accept generic BlockDriverOps. |
| arceos/modules/axfs/src/fs/devfs.rs | Add block-device nodes + I/O handling for them; refactor device content types. |
| arceos/modules/axfs/src/disk.rs | Introduce SharedBlockDevice wrapper and generic SeekableDisk. |
| arceos/api/arceos_posix_api/src/imp/fs.rs | Apply umask and EEXIST handling to sys_mkdir. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let len = min(buf.len() as u64, size - offset) as usize; | ||
| let mut disk = block.disk.lock(); | ||
| disk.set_position(offset).map_err(map_dev_err)?; | ||
| disk.write(&buf[..len]).map_err(map_dev_err) |
There was a problem hiding this comment.
Writes to block-device nodes can remain buffered in SeekableDisk (write_buffer_dirty stays true when the write ends mid-block). Since this write_at path does not flush after the write, and DevNode::sync() is currently a no-op, data written to /dev/vd* may not reach the underlying device until a later reposition/read/write happens. Consider flushing the SeekableDisk after completing a block-device write_at (or implementing sync() to flush for block devices and ensuring it is invoked on close/fsync).
| disk.write(&buf[..len]).map_err(map_dev_err) | |
| let written = disk.write(&buf[..len]).map_err(map_dev_err)?; | |
| disk.flush().map_err(map_dev_err)?; | |
| Ok(written) |
| Ok(mountpoint) => { | ||
| MOUNTED_TARGETS.lock().insert(target_path.clone()); | ||
| axfs::register_mounted_mountpoint(&target_path, mountpoint); | ||
| axfs::register_mount(&source, &target_path, &fstype, "rw,relatime"); | ||
| let _ = pulse_core::task::current_process().map(|process| process.save_fs_context()); |
There was a problem hiding this comment.
sys_mount/sys_umount2 now use axfs::register_mounted_mountpoint()/lookup_mounted_mountpoint() as the source of truth for mounted targets, but this code still updates the separate MOUNTED_TARGETS set. Keeping two independent registries risks divergence over time and makes mount state harder to reason about. Consider removing MOUNTED_TARGETS here (or clearly defining which registry is authoritative and deriving the other from it).
| NodeContent::BlockDevice(block) => { | ||
| let size = inode.metadata.lock().size; | ||
| if offset >= size { | ||
| return Ok(0); | ||
| } | ||
| let len = min(buf.len() as u64, size - offset) as usize; | ||
| let mut disk = block.disk.lock(); | ||
| disk.set_position(offset).map_err(map_dev_err)?; | ||
| disk.read(&mut buf[..len]).map_err(map_dev_err) | ||
| } |
There was a problem hiding this comment.
This change adds new externally visible behavior (auto-populating /dev with block device nodes and supporting reads/writes via devfs). There are existing axfs tests under arceos/modules/axfs/tests, but nothing here exercises the new devfs block-device semantics (e.g., read/write correctness and flush/sync behavior). Adding a focused regression test would help prevent silent data-loss bugs and ensure /dev/vd* nodes behave consistently.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 94b29dbb0a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let len = min(buf.len() as u64, size - offset) as usize; | ||
| let mut disk = block.disk.lock(); | ||
| disk.set_position(offset).map_err(map_dev_err)?; | ||
| disk.write(&buf[..len]).map_err(map_dev_err) |
There was a problem hiding this comment.
In DevNode::write_at's NodeContent::BlockDevice arm, the code returns after disk.write(...) without a flush. SeekableDisk keeps partial-block writes buffered (write_buffer_dirty) until a later set_position/flush, and DevNode::sync is currently a no-op, so a single unaligned write to /dev/vd* can be lost or remain invisible to other consumers until unrelated later I/O touches that node. This is a real data-integrity issue for short writes (e.g., metadata/superblock updates).
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| if MOUNTED_TARGETS.lock().contains(&target_path) { | ||
| if axfs::lookup_mounted_mountpoint(&target_path).is_some() { |
There was a problem hiding this comment.
sys_mount now rejects busy targets only via axfs::lookup_mounted_mountpoint, but root (/) is never inserted into that map during filesystem init (only register_mount is called for /). This allows mount(..., "/", ...) to over-mount the root unexpectedly; combined with sys_umount2 hard-rejecting "/", the original root becomes inaccessible for the remainder of the session.
Useful? React with 👍 / 👎.
Summary by CodeRabbit
Release Notes
New Features
/devfor direct disk I/O access.umasksyscall to control default file permission masks per-process.Refactor