Feat: improve async filesystem - #59
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
ⓘ Qodo reviews are paused because your trial has ended. Ask your workspace admin to add credits to resume reviews. Manage billing |
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough本次改动将块驱动、VirtIO、共享磁盘、Ext4、文件缓存和目录缓存改为异步并发流程。系统调用和用户态 trap 慢路径新增中断状态守卫。文件系统刷新统一使用异步总刷新入口。 Changes异步存储栈
中断与系统调用
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
pulse_core/src/trap.rs (1)
14-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win禁止将
TrapIrqEnableGuard跨 CPU 移动。
Drop::drop会修改当前 CPU 的中断状态，但当前只保存bool，会在没有 CPU 绑定约束时自动实现Send/Sync。加入PhantomData<*mut ()>，阻止守护值在其他 CPU 析构导致中断状态与创建 CPU 不一致。建议修改
pub struct TrapIrqEnableGuard { restore_disabled: bool, + _not_send_or_sync: core::marker::PhantomData<*mut ()>, } - Self { restore_disabled } + Self { + restore_disabled, + _not_send_or_sync: core::marker::PhantomData, + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pulse_core/src/trap.rs` around lines 14 - 25, Update TrapIrqEnableGuard to include PhantomData<*mut ()> state, preventing automatic Send/Sync implementations and ensuring the guard cannot move across CPUs before Drop::drop restores interrupt state. Initialize the marker in TrapIrqEnableGuard::new while preserving the existing restore_disabled behavior.crates/axdriver_block/src/lib.rs (1)
142-148: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win注册表在读路径上做全局锁 + 线性扫描，请改用有序索引。
claim_owned_read_buffer在每次块读取时获取全局OWNED_READ_BUFFERS自旋锁，并线性遍历Vec查找包含目标范围的条目。register_owned_read_buffer的重叠检查同样是 O(n)。页缓存可能注册大量页帧，此时每次直接读都变成 O(n)，并且所有 CPU 在同一把自旋锁上串行化，这与本 PR 的并发目标冲突。建议用按起始地址排序的结构（例如
BTreeMap<usize, Arc<OwnedReadBufferRange>>）替换Vec。查找时用range(..=start).next_back()定位候选条目，重叠检查同样只需比较前后相邻条目。♻️ 建议的索引结构调整方向
-static OWNED_READ_BUFFERS: Mutex<Vec<OwnedReadBufferEntry>> = Mutex::new(Vec::new()); +// key = range.start +static OWNED_READ_BUFFERS: Mutex<BTreeMap<usize, OwnedReadBufferEntry>> = + Mutex::new(BTreeMap::new());
claim_owned_read_buffer中的查找改为：let (_, entry) = buffers.range(..=start).next_back()?; if entry.range.start > start || end > entry.range.end { return None; }Also applies to: 172-199
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/axdriver_block/src/lib.rs` around lines 142 - 148, 将 OWNED_READ_BUFFERS 从 Vec 改为按起始地址索引的有序结构（如 BTreeMap<usize, Arc<OwnedReadBufferRange>>），并更新 register_owned_read_buffer 与 claim_owned_read_buffer 的访问逻辑。使用 range(..=start).next_back() 定位候选范围，注册时仅检查相邻前后条目的重叠，避免全量线性扫描，同时保持现有资源冲突与范围匹配行为。arceos/modules/axdriver/src/virtio.rs (1)
812-824: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win未知或越界 token 会让已用环永久停滞。
complete_one在peek_used返回的 token 没有对应挂起请求时只记录日志并返回None。该已用项没有被弹出，因此它永远留在环头。之后每次drain_completions（IRQ、定时器、poll）都会重新看到同一个 token，重复打印错误日志，并且所有真正的完成项都排在它后面，无法被认领。结果是全部块 I/O 静默挂死，同时日志被刷满。这两个分支表示驱动状态已损坏。请把它们变成显式的致命处理：记录一次日志后进入永久错误状态，让后续请求快速失败，或者直接
panic!，与本文件其他不变量（例如第 1234 行的virtio-blk reused an in-flight token）保持一致。🔒️ 建议的处理方向
let Some(slot) = self.pending.get_mut(token as usize) else { - axlog::error!("virtio-blk completed out-of-range token {}", token); - return None; + panic!("virtio-blk completed out-of-range token {}", token); }; let Some((request_id, mut request)) = slot.take() else { - axlog::error!("virtio-blk completed unknown token {}", token); - return None; + panic!("virtio-blk completed unknown token {}", token); };如果不能接受 panic，请改为置位一个
fatal: AtomicBool，并让poll在该位被置位时立即返回Err(DevError::BadState)，避免无限重扫环头。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axdriver/src/virtio.rs` around lines 812 - 824, Update complete_one so the out-of-range and unknown-token branches do not return normally while leaving the used-ring entry at its head. After logging once, transition the device into an unrecoverable error state that makes subsequent polling/request handling fail fast, or use panic! consistently with the existing “virtio-blk reused an in-flight token” invariant handling; ensure later drain_completions calls cannot repeatedly rescan the corrupted token.arceos/modules/axfs/src/highlevel/file.rs (1)
2192-2203: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win大范围写入会按页数构建条带索引，开销与写入长度成正比。
page_fill_lock_indices对每个页号都执行一次哈希并推入Vec::with_capacity(page_count)，随后排序去重。条带总数固定为PAGE_ACCESS_LOCK_STRIPES（256）。一次数百 MB 的写入会产生数十万次迭代和一次大数组分配，而结果最多只有 256 个索引。请在page_count >= PAGE_ACCESS_LOCK_STRIPES时直接使用全部条带索引。⚡ 建议的优化
// CachedFileShared::page_fill_lock_indices fn page_fill_lock_indices(&self, pn: u32, page_count: usize) -> VfsResult<Vec<usize>> { if page_count >= PAGE_ACCESS_LOCK_STRIPES { return Ok((0..PAGE_ACCESS_LOCK_STRIPES).collect()); } // ...现有逐页路径 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/highlevel/file.rs` around lines 2192 - 2203, 在 CachedFileShared::page_fill_lock_indices 中增加 page_count >= PAGE_ACCESS_LOCK_STRIPES 的快速路径，直接返回 0 到 PAGE_ACCESS_LOCK_STRIPES 的全部条带索引，避免逐页哈希、排序去重和大 Vec 分配；保留较小 page_count 的现有逐页处理逻辑不变。arceos/modules/axmm/src/backend/file.rs (1)
846-852: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win当前测试无法区分冷启动窗口与连续窗口。
readahead_resets_after_nonsequential_fault全部使用max_pages = 4。此时max_pages.min(COLD_FILE_FAULT_AROUND_PAGES)与max_pages结果相同，所以第 107 行的冷启动上限不会被验证。请增加一个max_pages > COLD_FILE_FAULT_AROUND_PAGES的用例。💚 建议的测试补充
#[test] fn readahead_resets_after_nonsequential_fault() { let mut state = FileReadAheadState::default(); assert_eq!(state.plan(3, 4), 4); assert_eq!(state.plan(20, 4), 4); assert_eq!(state.plan(24, 4), 4); } + + #[test] + fn cold_fault_is_capped_below_the_sequential_window() { + let mut state = FileReadAheadState::default(); + assert_eq!(state.plan(3, 8), COLD_FILE_FAULT_AROUND_PAGES); + assert_eq!(state.plan(3 + COLD_FILE_FAULT_AROUND_PAGES as u32, 8), 8); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axmm/src/backend/file.rs` around lines 846 - 852, Update the test readahead_resets_after_nonsequential_fault to include a case where max_pages exceeds COLD_FILE_FAULT_AROUND_PAGES, and assert the initial cold-start plan is capped at the cold-start limit while subsequent sequential plans can use the larger max_pages window.arceos/modules/axfs/src/disk.rs (1)
180-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
DiskWriteScope::run对同一 scope 的重入没有防护。
run接收&self，因此同一个DiskWriteScope可以被并发或嵌套地run。enter_write_scope在 ext4 实现中把scope_id压入按 task 的栈，leave_write_scope的debug_assert_eq!(position, stack.len().checked_sub(1))假设严格的后进先出顺序（见arceos/modules/axfs/src/fs/ext4/mod.rs第 267-282 行）。若两个 future 交错轮询，该断言会在 debug 构建中触发。建议把
run改为&mut self，或在文档注释中写明“同一 scope 一次只能运行一个 future”。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/disk.rs` around lines 180 - 192, Update DiskWriteScope::run to take &mut self, preventing concurrent or nested execution of the same scope and preserving the required LIFO ordering of enter_write_scope and leave_write_scope. Propagate any necessary mutability changes to callers while leaving the existing polling and guard behavior unchanged.arceos/modules/axfs/src/fs/ext4/mod.rs (2)
126-136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
ReadRangeGuards::len中的let _ = &**guard;无作用，请删除。该语句只做一次解引用，不影响返回值
1。它增加读者的理解负担。如果目的是抑制未使用字段的警告，请改用Self::One(_) => 1。♻️ 建议的简化
fn len(&self) -> usize { match self { - Self::One(guard) => { - let _ = &**guard; - 1 - } + Self::One(_) => 1, Self::Many(guards) => guards.len(), } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/mod.rs` around lines 126 - 136, 简化 ReadRangeGuards::len 中的 Self::One 分支，删除无作用的 let _ = &**guard;，并将未使用的 guard 绑定改为通配符模式，同时保持返回值为 1 不变。
1102-1107: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
set_block_size直接清空缓存，会丢弃脏块。
self.block_cache.lock().clear()丢弃全部缓存项，包含dirty == true的块。它也不清理flushing_evicted与dirty_owners，这两个结构随后会保留指向旧块大小的偏移。当前唯一调用点是arceos/modules/axfs/src/fs/ext4/fs.rs第 63 行的挂载流程，那时缓存内只有干净的超级块数据，因此不会立刻丢数据。请增加防护，避免后续调用引入静默数据丢失。
🛡️ 建议增加断言与状态清理
pub async fn set_block_size(&self, size: usize) { let _io_guards = self.lock_all_write_stripes().await; self.block_size .store(size, core::sync::atomic::Ordering::Relaxed); - self.block_cache.lock().clear(); + let mut cache = self.block_cache.lock(); + debug_assert!( + cache.iter().all(|(_, block)| !block.dirty), + "set_block_size must not discard dirty blocks" + ); + cache.clear(); + drop(cache); + debug_assert!(self.flushing_evicted.lock().is_empty()); + self.dirty_owners.lock().clear(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/mod.rs` around lines 1102 - 1107, 更新 Ext4 缓存的 set_block_size：在修改 block_size 并清空 block_cache 前，断言不存在脏块，防止静默丢失未写回数据；同时清理 flushing_evicted 和 dirty_owners 中与旧块大小相关的状态，确保缓存及其辅助索引不会残留旧偏移信息。arceos/modules/axfs/src/fs/ext4/inode.rs (1)
427-447: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
lock_directory_pair用ino判等，其他路径用指针判等，标准不一致。第 434 行用
self.ino == other.ino判定同一目录。link（第 1054 行）与unlink（第 1135 行）用core::ptr::eq。Inode::new通过active_inodes复用实例，但在 weak 引用刚失效的竞态窗口内，同一ino可能对应两个不同的Inode对象。此时lock_directory_pair只取一把锁，另一个对象的mutation_lock未被持有。请统一判等标准，或在注释中写明
ino到实例的唯一性保证。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/inode.rs` around lines 427 - 447, 统一 lock_directory_pair 与 link、unlink 的实例判等标准，使用 core::ptr::eq 判断 self 和 other 是否为同一 Inode；仅在指针相同的情况下只获取一把锁，否则按稳定顺序获取两个 mutation_lock，避免不同对象但相同 ino 时漏锁。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@arceos/modules/axfs/src/disk.rs`:
- Around line 345-350: 更新 read_partial 及同类的读写/刷新路径（包括 read_block、write_block
相关代码），避免在持有 inner 与 dev 自旋锁时执行可能阻塞的块 I/O：先克隆设备句柄并提取所需 block_id
与缓冲区到局部变量，释放两把锁后调用 read_block/write_block，再按现有逻辑写回必要状态。确保第
376-379、391-395、426-429 行对应路径同样遵循先释放锁、后执行 I/O 的顺序。
In `@arceos/modules/axfs/src/fs/ext4/fs.rs`:
- Around line 120-125: 更新 `flush_inode`，不要仅依赖按 owner 选择脏块的
`disk_flusher.flush_owner`；应改用或补充按 inode 完整匹配并写回所有脏块的刷新路径，覆盖无 owner 或在
`write_scope` 外通过 `write_offset` 产生的脏块。保持异步刷新流程及现有 `VfsError::Io` 错误映射不变。
In `@arceos/modules/axfs/src/fs/ext4/inode.rs`:
- Around line 1208-1209: 统一 Inode::mutation_lock 的加锁顺序以消除 rename、link、unlink
间的死锁：在 arceos/modules/axfs/src/fs/ext4/inode.rs:1208-1209，将目录锁与子节点锁合并、去重后按 ino
升序通过统一辅助函数一次性获取，替换 lock_directory_pair；在
arceos/modules/axfs/src/fs/ext4/inode.rs:1046-1058 和 :1134-1140，让对应操作同样使用该辅助函数按
ino 升序获取 self 与 child，移除“目录先、子节点后”的固定顺序。
- Around line 1134-1140: Update the unlink flow around Inode::new and its
active-reference check so the temporary child instance is not counted as an
external active inode, allowing immediate deletion when no other references
exist. Apply the same correction to the dst_node instance created by rename,
either by avoiding active_inodes registration for these temporary instances or
excluding the exact newly created instance during still_active evaluation.
- Around line 884-902: 在包含 snapshot 的两条 lookup 缓存发布路径中，先于
self.dir_snapshot(fs).await? 或使用已有 snapshot 的处理记录 lookup_generation，并将该预先记录的代际传给
publish_lookup；不要在读取 snapshot 后再调用 self.dir_cache.lookup_generation()。更新涉及
dir_cache 分支及 note_uncached_lookup 分支，保留现有 entry_from_cached_lookup 行为。
In `@arceos/modules/axfs/src/highlevel/file.rs`:
- Around line 674-692: 更新 PageCacheFrame::drop
的引用计数处理：不要将用户映射持有的引用计入缓存页释放逻辑，也不要对仅由映射持有的 ref_count == 1 调用 dec_ref。仅在
PageCacheFrame 明确持有独立引用时递减并在计数归零后释放；只要仍有用户映射或其他引用，就保留页面，避免触发 FrameTable::dec_ref
的非法减计数。
- Around line 1080-1105: Update the batch assembly loop around
submit_writeback_batch so page-cache lookup or data extraction errors are stored
in first_error instead of returned immediately. Stop submitting additional
batches after the first error, then continue polling and draining all existing
pending futures so in-flight writebacks complete and their page state is handled
normally.
---
Nitpick comments:
In `@arceos/modules/axdriver/src/virtio.rs`:
- Around line 812-824: Update complete_one so the out-of-range and unknown-token
branches do not return normally while leaving the used-ring entry at its head.
After logging once, transition the device into an unrecoverable error state that
makes subsequent polling/request handling fail fast, or use panic! consistently
with the existing “virtio-blk reused an in-flight token” invariant handling;
ensure later drain_completions calls cannot repeatedly rescan the corrupted
token.
In `@arceos/modules/axfs/src/disk.rs`:
- Around line 180-192: Update DiskWriteScope::run to take &mut self, preventing
concurrent or nested execution of the same scope and preserving the required
LIFO ordering of enter_write_scope and leave_write_scope. Propagate any
necessary mutability changes to callers while leaving the existing polling and
guard behavior unchanged.
In `@arceos/modules/axfs/src/fs/ext4/inode.rs`:
- Around line 427-447: 统一 lock_directory_pair 与 link、unlink 的实例判等标准，使用
core::ptr::eq 判断 self 和 other 是否为同一 Inode；仅在指针相同的情况下只获取一把锁，否则按稳定顺序获取两个
mutation_lock，避免不同对象但相同 ino 时漏锁。
In `@arceos/modules/axfs/src/fs/ext4/mod.rs`:
- Around line 126-136: 简化 ReadRangeGuards::len 中的 Self::One 分支，删除无作用的 let _ =
&**guard;，并将未使用的 guard 绑定改为通配符模式，同时保持返回值为 1 不变。
- Around line 1102-1107: 更新 Ext4 缓存的 set_block_size：在修改 block_size 并清空
block_cache 前，断言不存在脏块，防止静默丢失未写回数据；同时清理 flushing_evicted 和 dirty_owners
中与旧块大小相关的状态，确保缓存及其辅助索引不会残留旧偏移信息。
In `@arceos/modules/axfs/src/highlevel/file.rs`:
- Around line 2192-2203: 在 CachedFileShared::page_fill_lock_indices 中增加
page_count >= PAGE_ACCESS_LOCK_STRIPES 的快速路径，直接返回 0 到 PAGE_ACCESS_LOCK_STRIPES
的全部条带索引，避免逐页哈希、排序去重和大 Vec 分配；保留较小 page_count 的现有逐页处理逻辑不变。
In `@arceos/modules/axmm/src/backend/file.rs`:
- Around line 846-852: Update the test
readahead_resets_after_nonsequential_fault to include a case where max_pages
exceeds COLD_FILE_FAULT_AROUND_PAGES, and assert the initial cold-start plan is
capped at the cold-start limit while subsequent sequential plans can use the
larger max_pages window.
In `@crates/axdriver_block/src/lib.rs`:
- Around line 142-148: 将 OWNED_READ_BUFFERS 从 Vec 改为按起始地址索引的有序结构（如
BTreeMap<usize, Arc<OwnedReadBufferRange>>），并更新 register_owned_read_buffer 与
claim_owned_read_buffer 的访问逻辑。使用 range(..=start).next_back()
定位候选范围，注册时仅检查相邻前后条目的重叠，避免全量线性扫描，同时保持现有资源冲突与范围匹配行为。
In `@pulse_core/src/trap.rs`:
- Around line 14-25: Update TrapIrqEnableGuard to include PhantomData<*mut ()>
state, preventing automatic Send/Sync implementations and ensuring the guard
cannot move across CPUs before Drop::drop restores interrupt state. Initialize
the marker in TrapIrqEnableGuard::new while preserving the existing
restore_disabled behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a9562bd-024d-449d-b306-498130fcafda
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (20)
arceos/modules/axdriver/src/prelude.rsarceos/modules/axdriver/src/virtio.rsarceos/modules/axfs/Cargo.tomlarceos/modules/axfs/src/disk.rsarceos/modules/axfs/src/fs/ext4/fs.rsarceos/modules/axfs/src/fs/ext4/inode.rsarceos/modules/axfs/src/fs/ext4/mod.rsarceos/modules/axfs/src/fs/loop_dev.rsarceos/modules/axfs/src/highlevel/file.rsarceos/modules/axfs/src/lib.rsarceos/modules/axmm/src/backend/file.rscrates/axdriver_block/Cargo.tomlcrates/axdriver_block/src/lib.rscrates/axdriver_virtio/src/blk.rscrates/axfs-ng-vfs/src/node/dir.rscrates/virtio-drivers/src/device/blk.rscrates/virtio-drivers/src/lib.rspulse_core/src/trap.rspulse_syscalls/src/handler.rspulse_syscalls/src/impls/fs/io.rs
| #[derive(Debug)] | ||
| pub struct PageCache { | ||
| struct PageCacheFrame { | ||
| addr: VirtAddr, | ||
| } | ||
|
|
||
| impl Drop for PageCacheFrame { | ||
| fn drop(&mut self) { | ||
| let paddr = virt_to_phys(self.addr); | ||
| if let Some(ref_count) = axalloc::frame_table().try_get_ref(paddr) { | ||
| if ref_count == 0 { | ||
| global_allocator().dealloc_pages(self.addr.as_usize(), 1); | ||
| } else if axalloc::frame_table().dec_ref(paddr) == 0 { | ||
| global_allocator().dealloc_pages(self.addr.as_usize(), 1); | ||
| } | ||
| } else { | ||
| global_allocator().dealloc_pages(self.addr.as_usize(), 1); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# 查明 frame_table 引用计数与页释放路径的所有者约定
fd -t f 'frameinfo.rs' arceos/modules/axalloc | xargs -r cat -n
ast-grep run --pattern 'fn dealloc_frame($$$) { $$$ }' --lang rust arceos/modules/axmm
rg -nP -C4 '\b(dealloc_frame|inc_ref|dec_ref|mark_used)\s*\(' --type=rust arceos/modules/axmm arceos/modules/axfsRepository: muou000/PulseOS
Length of output: 18923
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== axfs file.rs relevant sections =="
sed -n '620,705p' arceos/modules/axfs/src/highlevel/file.rs
echo
sed -n '720,760p' arceos/modules/axfs/src/highlevel/file.rs
echo
sed -n '1630,1690p' arceos/modules/axfs/src/highlevel/file.rs
echo
echo "== axalloc frameinfo references and exports =="
rg -n -C3 'pub (struct|fn)|FrameTable|frame_table|PinForMapping|pin_for_mapping|PageCacheFrame' arceos/modules/axalloc arceos/modules/axmm arceos/modules/axfs/src/highlevel/file.rsRepository: muou000/PulseOS
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read-only verifier equivalent of PageCacheFrame::drop behavior from the source.
python3 - <<'PY'
def current_drop(ref_count):
if ref_count is None:
return "dealloc"
if ref_count == 0:
return "dealloc"
remaining = ref_count - 1
if remaining == 0:
return "dec_ref_zero_dealloc"
return "ref_count_reduced_to_" + str(remaining)
def requested_drop(ref_count):
if ref_count is None:
return "dealloc"
if ref_count == 0:
return "dealloc"
return "no_dealloc"
for n in [None, 0, 1, 2]:
print(f"entry ref_count = {n}: current={current_drop(n)}, requested={requested_drop(n)}")
PYRepository: muou000/PulseOS
Length of output: 414
避免在 PageCacheFrame::drop 中把用户映射引用计入页引用计数。
FrameTable 的 dec_ref 不允许将 0 减到负值；pin_for_mapping 当前在原来为 0 时调用 mark_used，原为 1 时只调用 inc_ref，因此 PageCacheFrame 可能并不持有引用。若用户映射存在，ref_count == 1 的 dec_ref 会触发 panic，并且缓存解除/回写路径不应替代映射解除逻辑。保持 ref_count > 0 时不释放，除非 PageCacheFrame 能确保自己显式持有计数中的一份。
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@arceos/modules/axfs/src/highlevel/file.rs` around lines 674 - 692, 更新
PageCacheFrame::drop 的引用计数处理：不要将用户映射持有的引用计入缓存页释放逻辑，也不要对仅由映射持有的 ref_count == 1 调用
dec_ref。仅在 PageCacheFrame 明确持有独立引用时递减并在计数归零后释放；只要仍有用户映射或其他引用，就保留页面，避免触发
FrameTable::dec_ref 的非法减计数。
| loop { | ||
| let (dirty_pns, page_start, data) = { | ||
| let mut guard = self.page_cache.lock(); | ||
| let mut dirty_pns = guard | ||
| .iter() | ||
| .filter(|(_, page)| page.dirty) | ||
| .map(|(pn, _)| *pn) | ||
| .collect::<Vec<_>>(); | ||
| dirty_pns.sort_unstable(); | ||
| let Some(&pn_start) = dirty_pns.first() else { | ||
| return Ok(()); | ||
| }; | ||
| let mut count = 1usize; | ||
| while count < dirty_pns.len() | ||
| && count < MAX_WRITEBACK_PAGES | ||
| && dirty_pns[count] == pn_start + count as u32 | ||
| { | ||
| count += 1; | ||
| } | ||
| dirty_pns.truncate(count); | ||
|
|
||
| let pn_end = *dirty_pns.last().unwrap(); | ||
| let page_start = pn_start as u64 * PAGE_SIZE as u64; | ||
| let last_page_start = pn_end as u64 * PAGE_SIZE as u64; | ||
| let last_len = file_len | ||
| .saturating_sub(last_page_start) | ||
| .min(PAGE_SIZE as u64) as usize; | ||
| let mut data = Vec::new(); | ||
| if last_len != 0 { | ||
| let total_len = (pn_end - pn_start) as usize * PAGE_SIZE + last_len; | ||
| data.reserve(total_len); | ||
| for &pn in &dirty_pns { | ||
| let page = guard.get_mut(&pn).ok_or(VfsError::Io)?; | ||
| let curr_page_start = pn as u64 * PAGE_SIZE as u64; | ||
| let curr_len = file_len | ||
| .saturating_sub(curr_page_start) | ||
| .min(PAGE_SIZE as u64) as usize; | ||
| data.extend_from_slice(&page.data()[..curr_len]); | ||
| page.dirty = false; | ||
| } | ||
| } else { | ||
| for &pn in &dirty_pns { | ||
| if let Some(page) = guard.get_mut(&pn) { | ||
| page.dirty = false; | ||
| } | ||
| while next_batch < page_batches.len() && pending.len() < WRITEBACK_CONCURRENCY { | ||
| let pages = &page_batches[next_batch]; | ||
| let page_start = pages[0] as u64 * PAGE_SIZE as u64; | ||
| let data = { | ||
| let mut cache = self.page_cache.lock(); | ||
| let mut data = Vec::new(); | ||
| for &pn in pages { | ||
| let page = cache.get_mut(&pn).ok_or(VfsError::Io)?; | ||
| let current_start = pn as u64 * PAGE_SIZE as u64; | ||
| let len = | ||
| file_len.saturating_sub(current_start).min(PAGE_SIZE as u64) as usize; | ||
| data.extend_from_slice(&page.data()[..len]); | ||
| } | ||
| } | ||
| (dirty_pns, page_start, data) | ||
| }; | ||
| data | ||
| }; | ||
| pending.push(submit_writeback_batch( | ||
| file, | ||
| WritebackBatch { | ||
| pages: pages.clone(), | ||
| offset: page_start, | ||
| data, | ||
| }, | ||
| )); | ||
| next_batch += 1; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
批次收集失败会取消已在飞行中的写回。
第 1088 行在批次组装阶段使用 ? 直接返回。此时 pending 中仍可能存在未完成的 submit_writeback_batch future。返回会丢弃 FuturesUnordered，从而取消这些正在进行的写回，并且这些页的脏标记既不会被清除也不会被确认落盘。请把该错误记录到 first_error，然后停止提交新批次并继续排空 pending。
♻️ 建议的处理方式
- let data = {
+ let data = {
let mut cache = self.page_cache.lock();
let mut data = Vec::new();
+ let mut missing = false;
for &pn in pages {
- let page = cache.get_mut(&pn).ok_or(VfsError::Io)?;
+ let Some(page) = cache.get_mut(&pn) else {
+ missing = true;
+ break;
+ };
let current_start = pn as u64 * PAGE_SIZE as u64;
let len =
file_len.saturating_sub(current_start).min(PAGE_SIZE as u64) as usize;
data.extend_from_slice(&page.data()[..len]);
}
- data
+ if missing {
+ if first_error.is_none() {
+ first_error = Some(VfsError::Io);
+ }
+ next_batch = page_batches.len();
+ break;
+ }
+ data
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| loop { | |
| let (dirty_pns, page_start, data) = { | |
| let mut guard = self.page_cache.lock(); | |
| let mut dirty_pns = guard | |
| .iter() | |
| .filter(|(_, page)| page.dirty) | |
| .map(|(pn, _)| *pn) | |
| .collect::<Vec<_>>(); | |
| dirty_pns.sort_unstable(); | |
| let Some(&pn_start) = dirty_pns.first() else { | |
| return Ok(()); | |
| }; | |
| let mut count = 1usize; | |
| while count < dirty_pns.len() | |
| && count < MAX_WRITEBACK_PAGES | |
| && dirty_pns[count] == pn_start + count as u32 | |
| { | |
| count += 1; | |
| } | |
| dirty_pns.truncate(count); | |
| let pn_end = *dirty_pns.last().unwrap(); | |
| let page_start = pn_start as u64 * PAGE_SIZE as u64; | |
| let last_page_start = pn_end as u64 * PAGE_SIZE as u64; | |
| let last_len = file_len | |
| .saturating_sub(last_page_start) | |
| .min(PAGE_SIZE as u64) as usize; | |
| let mut data = Vec::new(); | |
| if last_len != 0 { | |
| let total_len = (pn_end - pn_start) as usize * PAGE_SIZE + last_len; | |
| data.reserve(total_len); | |
| for &pn in &dirty_pns { | |
| let page = guard.get_mut(&pn).ok_or(VfsError::Io)?; | |
| let curr_page_start = pn as u64 * PAGE_SIZE as u64; | |
| let curr_len = file_len | |
| .saturating_sub(curr_page_start) | |
| .min(PAGE_SIZE as u64) as usize; | |
| data.extend_from_slice(&page.data()[..curr_len]); | |
| page.dirty = false; | |
| } | |
| } else { | |
| for &pn in &dirty_pns { | |
| if let Some(page) = guard.get_mut(&pn) { | |
| page.dirty = false; | |
| } | |
| while next_batch < page_batches.len() && pending.len() < WRITEBACK_CONCURRENCY { | |
| let pages = &page_batches[next_batch]; | |
| let page_start = pages[0] as u64 * PAGE_SIZE as u64; | |
| let data = { | |
| let mut cache = self.page_cache.lock(); | |
| let mut data = Vec::new(); | |
| for &pn in pages { | |
| let page = cache.get_mut(&pn).ok_or(VfsError::Io)?; | |
| let current_start = pn as u64 * PAGE_SIZE as u64; | |
| let len = | |
| file_len.saturating_sub(current_start).min(PAGE_SIZE as u64) as usize; | |
| data.extend_from_slice(&page.data()[..len]); | |
| } | |
| } | |
| (dirty_pns, page_start, data) | |
| }; | |
| data | |
| }; | |
| pending.push(submit_writeback_batch( | |
| file, | |
| WritebackBatch { | |
| pages: pages.clone(), | |
| offset: page_start, | |
| data, | |
| }, | |
| )); | |
| next_batch += 1; | |
| } | |
| loop { | |
| while next_batch < page_batches.len() && pending.len() < WRITEBACK_CONCURRENCY { | |
| let pages = &page_batches[next_batch]; | |
| let page_start = pages[0] as u64 * PAGE_SIZE as u64; | |
| let data = { | |
| let mut cache = self.page_cache.lock(); | |
| let mut data = Vec::new(); | |
| let mut missing = false; | |
| for &pn in pages { | |
| let Some(page) = cache.get_mut(&pn) else { | |
| missing = true; | |
| break; | |
| }; | |
| let current_start = pn as u64 * PAGE_SIZE as u64; | |
| let len = | |
| file_len.saturating_sub(current_start).min(PAGE_SIZE as u64) as usize; | |
| data.extend_from_slice(&page.data()[..len]); | |
| } | |
| if missing { | |
| if first_error.is_none() { | |
| first_error = Some(VfsError::Io); | |
| } | |
| next_batch = page_batches.len(); | |
| break; | |
| } | |
| data | |
| }; | |
| pending.push(submit_writeback_batch( | |
| file, | |
| WritebackBatch { | |
| pages: pages.clone(), | |
| offset: page_start, | |
| data, | |
| }, | |
| )); | |
| next_batch += 1; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@arceos/modules/axfs/src/highlevel/file.rs` around lines 1080 - 1105, Update
the batch assembly loop around submit_writeback_batch so page-cache lookup or
data extraction errors are stored in first_error instead of returned
immediately. Stop submitting additional batches after the first error, then
continue polling and draining all existing pending futures so in-flight
writebacks complete and their page state is handled normally.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
arceos/modules/axfs/src/fs/ext4/inode.rs (3)
1119-1150: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win观察阶段可复用
dir_cache的 lookup 缓存，减少每次unlink的双倍目录 I/O。当前实现在每次
unlink中执行两轮完整的目录查找：1122-1132 行的观察阶段和 1140-1147 行的重确认阶段。每轮包含一次Inode::read和一次dir.get_entry。在无并发的常见路径上，这使unlink的元数据 I/O 翻倍。
self.dir_cache.get_lookup(name)已能在缓存命中时直接给出inode_num。观察阶段只需要一个候选 ino 来决定加锁顺序，缓存值即使陈旧也不影响正确性，因为 1148 行的重确认会检测不匹配并重试。建议在观察阶段先尝试缓存，仅在未命中时回退到目录读取。
♻️ 建议的快路径
loop { - let observation_nodes = [self]; - let observation_guards = Self::lock_mutation_set(&observation_nodes).await; - let observed_dir_inode = ext4plus::inode::Inode::read(&fs, dir_idx) - .await - .map_err(into_vfs_err)?; - let observed_dir = - ext4plus::dir::Dir::open_inode(&fs, observed_dir_inode).map_err(into_vfs_err)?; - let observed_name = - ext4plus::DirEntryName::try_from(name).map_err(|_| VfsError::InvalidInput)?; - let observed_child = observed_dir - .get_entry(observed_name) - .await - .map_err(into_vfs_err)?; - let observed_child_ino = observed_child.index.get(); - drop(observation_guards); + let observed_child_ino = match self.dir_cache.get_lookup(name) { + Some(Some(cached)) => cached.inode_num, + Some(None) => return Err(VfsError::NotFound), + None => { + let observation_nodes = [self]; + let observation_guards = Self::lock_mutation_set(&observation_nodes).await; + let observed_dir_inode = ext4plus::inode::Inode::read(&fs, dir_idx) + .await + .map_err(into_vfs_err)?; + let observed_dir = ext4plus::dir::Dir::open_inode(&fs, observed_dir_inode) + .map_err(into_vfs_err)?; + let observed_name = ext4plus::DirEntryName::try_from(name) + .map_err(|_| VfsError::InvalidInput)?; + let observed_child = observed_dir + .get_entry(observed_name) + .await + .map_err(into_vfs_err)?; + let ino = observed_child.index.get(); + drop(observation_guards); + ino + } + };注意：采用该快路径后，1148 行的
child_ino != observed_child_ino判断从“并发检测”变为同时承担“缓存校验”。请确认重试次数在缓存长期陈旧时仍能收敛。🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/inode.rs` around lines 1119 - 1150, 在 unlink 循环的观察阶段复用 self.dir_cache.get_lookup(name) 获取候选 inode；缓存命中时跳过 Inode::read、Dir::open_inode 和 observed_dir.get_entry，仅在未命中时执行现有目录读取回退。保留重确认阶段及 child_ino != observed_child_ino 的校验，使陈旧缓存通过 continue 重试并最终收敛。
1338-1342: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win用解构消除
expect，避免在写路径上留下 panic 点。1338-1340 行的
expect("rename destination lock target disappeared")依赖一条跨约 100 行的隐式推理：1299 行dst_inode为Some，1274 行校验dst_inode.index.get() == observed_dst_ino，因此 1242 行由observed_dst_ino.map(...)构造的dst_node也必为Some。该推理目前成立，
expect不会触发。但它把一个 panic 点放在文件系统写路径中间，并且依赖三处远距离代码保持同步。建议在 1299 行同时绑定
dst_inode与dst_node，让类型系统保证两者同时存在。♻️ 建议的解构
- if let Some(dst_inode) = dst_inode { + if let (Some(dst_inode), Some(local_dst)) = (dst_inode, dst_node.as_ref()) {if dst_inode.links_count() == 0 { - let local_dst = dst_node - .as_ref() - .expect("rename destination lock target disappeared"); let has_other_active = Self::mark_unlinked_and_has_external_refs(local_dst);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/inode.rs` around lines 1338 - 1342, 在重命名写路径中更新 `dst_inode` 的匹配逻辑，同时解构绑定对应的 `dst_node`，让两者在同一分支内被类型系统证明同时存在。随后移除 `dst_node.as_ref().expect(...)`，直接使用已绑定的节点引用，并保持 `mark_unlinked_and_has_external_refs` 的现有处理不变。
448-455: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Arc::strong_count(&inode) > 2依赖调用点的隐式引用计数约定，建议补充注释。阈值 2 的来源是：调用点持有 1 个强引用（
unlink中的child，rename中的dst_node），weak.upgrade()在循环内产生第 2 个强引用。只有超过 2 才表示存在第三方引用。该约定没有在代码中说明。如果将来某个调用点额外克隆了
Arc<Inode>，该函数会把本地实例误判为外部引用，立即删除分支将不再执行，删除退回后台process_pending_deletions。这不损坏数据，但会静默改变删除时机，且很难定位。建议在函数上方注释说明该不变量，或改为显式传入本地持有的强引用数量。
♻️ 建议的注释
+ /// 判断 `local` 对应的 inode 是否存在本函数调用方之外的活跃引用，并为这些 + /// 外部实例设置 `is_unlinked`。 + /// + /// 不变量：调用方必须恰好持有 `local` 的 1 个强引用。循环内 `weak.upgrade()` + /// 产生第 2 个强引用，因此 `strong_count > 2` 才代表存在第三方引用。 fn mark_unlinked_and_has_external_refs(local: &Arc<Self>) -> bool {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@arceos/modules/axfs/src/fs/ext4/inode.rs` around lines 448 - 455, 在包含 is_local 和 Arc::strong_count(&inode) > 2 判断的函数上方补充注释，明确说明阈值 2 依赖调用点持有一个强引用、循环内 weak.upgrade() 持有第二个强引用，超过 2 才表示存在第三方引用；同时记录 unlink 的 child 和 rename 的 dst_node 是该约定的一部分，避免未来额外克隆 Arc<Inode> 后误判本地实例。
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@arceos/modules/axfs/src/fs/ext4/inode.rs`:
- Around line 1366-1394: 更新 rename 流程中由 link_result 和 unlink_result
驱动的回滚逻辑：当源目录的 src_dir.unlink 失败时，立即在目标目录 dst_dir_obj 中撤销刚创建的 dst_name
条目，使链接计数恢复到 link 前状态。同步执行目标目录及 inode 元数据、目录缓存失效处理，并继续返回源条目删除错误；成功路径保持现有缓存更新行为不变。
---
Nitpick comments:
In `@arceos/modules/axfs/src/fs/ext4/inode.rs`:
- Around line 1119-1150: 在 unlink 循环的观察阶段复用 self.dir_cache.get_lookup(name) 获取候选
inode；缓存命中时跳过 Inode::read、Dir::open_inode 和
observed_dir.get_entry，仅在未命中时执行现有目录读取回退。保留重确认阶段及 child_ino != observed_child_ino
的校验，使陈旧缓存通过 continue 重试并最终收敛。
- Around line 1338-1342: 在重命名写路径中更新 `dst_inode` 的匹配逻辑，同时解构绑定对应的
`dst_node`，让两者在同一分支内被类型系统证明同时存在。随后移除
`dst_node.as_ref().expect(...)`，直接使用已绑定的节点引用，并保持
`mark_unlinked_and_has_external_refs` 的现有处理不变。
- Around line 448-455: 在包含 is_local 和 Arc::strong_count(&inode) > 2
判断的函数上方补充注释，明确说明阈值 2 依赖调用点持有一个强引用、循环内 weak.upgrade() 持有第二个强引用，超过 2
才表示存在第三方引用；同时记录 unlink 的 child 和 rename 的 dst_node 是该约定的一部分，避免未来额外克隆 Arc<Inode>
后误判本地实例。
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fdcf9bca-ec63-418b-9cf1-55431dac768f
📒 Files selected for processing (9)
arceos/modules/axdriver/src/virtio.rsarceos/modules/axfs/src/disk.rsarceos/modules/axfs/src/fs/ext4/fs.rsarceos/modules/axfs/src/fs/ext4/inode.rsarceos/modules/axfs/src/fs/ext4/mod.rsarceos/modules/axfs/src/highlevel/file.rsarceos/modules/axmm/src/backend/file.rscrates/axdriver_block/src/lib.rspulse_core/src/trap.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- pulse_core/src/trap.rs
- arceos/modules/axmm/src/backend/file.rs
- arceos/modules/axdriver/src/virtio.rs
- arceos/modules/axfs/src/fs/ext4/fs.rs
- crates/axdriver_block/src/lib.rs
- arceos/modules/axfs/src/fs/ext4/mod.rs
- arceos/modules/axfs/src/highlevel/file.rs
* feat(block): add request-owned asynchronous I/O * fix(runtime): enable IRQs across sleepable user traps * feat(fs): propagate async I/O through ext4 * perf(fs): parallelize page cache and directory operations * perf(fs): coalesce global filesystem flushes * fix(fs): harden async I/O concurrency invariants * fix(fs): harden ext4 mutation recovery
1 participant
Summary by CodeRabbit
新功能
问题修复