Rework directory block enumeration accounting

[RoyWFHuang]

__file_lookup() and simplefs_remove_from_dir() both walk a directory's extents block by block, but bound the walk differently:

• __file_lookup() subtracts a block's nr_files before scanning the block.
• simplefs_remove_from_dir() bounds its inner loop by the extent length (ee_len) instead of by the number of files left to find.

Neither skips blocks whose nr_files is 0, so an empty block is still walked entry by entry.

• Bound both inner loops by nr_bi_files, and decrement it only after a block has been scanned.
• Skip blocks whose nr_files is 0.
• In simplefs_try_remove_entry(), zero the freed slot's nr_blk after merging it into the preceding entry, and drop the blk_nr_files guard that the caller's count-based loop makes redundant.

---

Summary by cubic

Unifies directory block enumeration in __file_lookup() and simplefs_remove_from_dir() to use consistent file-count bounds and skip empty blocks. This fixes incorrect walk limits and reduces unnecessary reads during directory scans.

• Bug Fixes
  • Bound inner loops by remaining files (nr_bi_files); decrement only after scanning each block.
  • Skip blocks with nr_files == 0.
  • In simplefs_try_remove_entry(), zero the freed entry’s nr_blk after merging and remove the redundant blk_nr_files guard.

^{Written for commit 19654ba. Summary will update on new commits.}

[cubic-dev-ai]

1 issue found across 1 file

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="inode.c">

<violation number="1" location="inode.c:701">
P2: The removal path's inner loop now terminates and gets its “full extent coverage” entirely from the on-disk file-count counters (`eblock->extents[ei].nr_files` and `dirblk->nr_files`) instead of the structural extent length `ee_len`. Under *consistent* counters this works, but those counters are the very metadata being mutated during removals, and if they ever fall out of sync with the actual entries — e.g. a block whose `dirblk->nr_files` is 0 while it still holds the entry being unlinked, or an extent count larger than the sum of its per-block counts — the loop either reaches 0 early and silently skips the block that contains the target (so `simplefs_remove_from_dir` returns success-without-removal, leaving a stale directory entry / failed unlink/rename), or it never reaches 0 and keeps wrapping `bi` through the ring (`CHECK_AND_SET_RING_INDEX(bi, ee_len)`) re-reading blocks indefinitely, i.e. an unbounded loop / soft lockup. The previous `ee_len` bound guaranteed termination and that every physical block in the extent was visited regardless of the counters. Consider retaining a hard cap alongside the counter so an inconsistent counter can only cause a bounded miss rather than a hang.</violation>
</file>

<sub>Reply with feedback, questions, or to request a fix.

Re-trigger cubic</sub>

[cubic-dev-ai]

P2: The removal path's inner loop now terminates and gets its “full extent coverage” entirely from the on-disk file-count counters (eblock->extents[ei].nr_files and dirblk->nr_files) instead of the structural extent length ee_len. Under consistent counters this works, but those counters are the very metadata being mutated during removals, and if they ever fall out of sync with the actual entries — e.g. a block whose dirblk->nr_files is 0 while it still holds the entry being unlinked, or an extent count larger than the sum of its per-block counts — the loop either reaches 0 early and silently skips the block that contains the target (so simplefs_remove_from_dir returns success-without-removal, leaving a stale directory entry / failed unlink/rename), or it never reaches 0 and keeps wrapping bi through the ring (CHECK_AND_SET_RING_INDEX(bi, ee_len)) re-reading blocks indefinitely, i.e. an unbounded loop / soft lockup. The previous ee_len bound guaranteed termination and that every physical block in the extent was visited regardless of the counters. Consider retaining a hard cap alongside the counter so an inconsistent counter can only cause a bounded miss rather than a hang.

Prompt for AI agents

Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 701:

<comment>The removal path's inner loop now terminates and gets its “full extent coverage” entirely from the on-disk file-count counters (`eblock->extents[ei].nr_files` and `dirblk->nr_files`) instead of the structural extent length `ee_len`. Under *consistent* counters this works, but those counters are the very metadata being mutated during removals, and if they ever fall out of sync with the actual entries — e.g. a block whose `dirblk->nr_files` is 0 while it still holds the entry being unlinked, or an extent count larger than the sum of its per-block counts — the loop either reaches 0 early and silently skips the block that contains the target (so `simplefs_remove_from_dir` returns success-without-removal, leaving a stale directory entry / failed unlink/rename), or it never reaches 0 and keeps wrapping `bi` through the ring (`CHECK_AND_SET_RING_INDEX(bi, ee_len)`) re-reading blocks indefinitely, i.e. an unbounded loop / soft lockup. The previous `ee_len` bound guaranteed termination and that every physical block in the extent was visited regardless of the counters. Consider retaining a hard cap alongside the counter so an inconsistent counter can only cause a bounded miss rather than a hang.</comment>

<file context>
@@ -692,10 +695,10 @@ static int simplefs_remove_from_dir(struct inode *dir,
             /* simplefs_extent */
-            for (idx_bi = 0; idx_bi < eblock->extents[ei].ee_len;
-                 bi++, idx_bi++) {
+            for (idx_bi = 0; nr_bi_files; bi++, idx_bi++) {
                 CHECK_AND_SET_RING_INDEX(bi, eblock->extents[ei].ee_len);
                 bh2 = sb_bread(sb, eblock->extents[ei].ee_start + bi);
</file context>