Skip to content

Rework directory block enumeration accounting #97

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RoyWFHuang wants to merge 1 commit into
base: master
Choose a base branch
from
+16 −8
Open

Rework directory block enumeration accounting #97

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 16 additions & 8 deletions inode.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ static int __file_lookup(struct inode *dir,
if (!eblock->extents[_ei].ee_start)
continue;

nr_ei_files -= eblock->extents[_ei].nr_files;
/* Iterate blocks in extent */
int nr_bi_files = eblock->extents[_ei].nr_files;
for (idx_bi = 0; nr_bi_files; _bi++, idx_bi++) {
Expand All @@ -176,7 +175,10 @@ static int __file_lookup(struct inode *dir,

dblock = (struct simplefs_dir_block *) (*ret_bi_bh)->b_data;
/* Search file in ei_block */
nr_bi_files -= dblock->nr_files;
if (dblock->nr_files == 0) {
RELEASE_BUFFER_HEAD(*ret_bi_bh);
continue;
}
for (_fi = 0; _fi < SIMPLEFS_FILES_PER_BLOCK;) {
f = &dblock->files[_fi];
if (f->inode && !strncmp(f->filename, dentry->d_name.name,
Expand All @@ -188,8 +190,10 @@ static int __file_lookup(struct inode *dir,
}
_fi += dblock->files[_fi].nr_blk;
}
nr_bi_files -= dblock->nr_files;
RELEASE_BUFFER_HEAD(*ret_bi_bh);
}
nr_ei_files -= eblock->extents[_ei].nr_files;
_bi = 0;
}
file_search_end:
Expand Down Expand Up @@ -468,9 +472,8 @@ static bool simplefs_try_remove_entry(struct simplefs_dir_block *dblock,
const char *name)
{
int fi, i;
int blk_nr_files = dblock->nr_files;

for (fi = 0; blk_nr_files && fi < SIMPLEFS_FILES_PER_BLOCK;) {
for (fi = 0; fi < SIMPLEFS_FILES_PER_BLOCK;) {
if (dblock->files[fi].inode) {
if (dblock->files[fi].inode == ino &&
!strcmp(dblock->files[fi].filename, name)) {
Expand All @@ -479,6 +482,7 @@ static bool simplefs_try_remove_entry(struct simplefs_dir_block *dblock,
for (i = fi - 1; i >= 0; i--) {
if (dblock->files[i].inode != 0 || i == 0) {
dblock->files[i].nr_blk += dblock->files[fi].nr_blk;
dblock->files[fi].nr_blk = 0;
break;
}
}
Expand All @@ -487,7 +491,6 @@ static bool simplefs_try_remove_entry(struct simplefs_dir_block *dblock,
eblock->nr_files--;
return true;
}
blk_nr_files--;
}
fi += dblock->files[fi].nr_blk;
}
Expand Down Expand Up @@ -692,10 +695,10 @@ static int simplefs_remove_from_dir(struct inode *dir,
CHECK_AND_SET_RING_INDEX(ei, SIMPLEFS_MAX_EXTENTS);

if (eblock->extents[ei].ee_start) {
dir_nr_files -= eblock->extents[ei].nr_files;
int nr_bi_files = eblock->extents[ei].nr_files;
dir_nr_files -= nr_bi_files;
/* 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++) {

@cubic-dev-ai cubic-dev-ai Bot Aug 9, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

CHECK_AND_SET_RING_INDEX(bi, eblock->extents[ei].ee_len);
bh2 = sb_bread(sb, eblock->extents[ei].ee_start + bi);
if (!bh2) {
Expand All @@ -704,6 +707,10 @@ static int simplefs_remove_from_dir(struct inode *dir,
}
/* simplefs_dir_block */
dirblk = (struct simplefs_dir_block *) bh2->b_data;
if (dirblk->nr_files == 0) {
RELEASE_BUFFER_HEAD(bh2);
continue;
}
if (simplefs_try_remove_entry(dirblk, eblock, ei, inode->i_ino,
dentry->d_name.name)) {
mark_buffer_dirty(bh2);
Expand All @@ -712,6 +719,7 @@ static int simplefs_remove_from_dir(struct inode *dir,
*ret_ei = ei;
goto found_data;
}
nr_bi_files -= dirblk->nr_files;
RELEASE_BUFFER_HEAD(bh2);
}
}
Expand Down
Loading