Use hash func to boost file creation and lookup #79
Conversation
|
How can you determine which hash function is the most suitable? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
visitorckw
left a comment
visitorckw
left a comment
There was a problem hiding this comment.
I saw that your PR description includes some performance benchmarks, but the commit message lacks any performance numbers to support your improvements. Please improve the commit message.
RoyWFHuang
force-pushed
the
feature/op_perf
branch
from
b928f62 to
308bb4c
Compare
RoyWFHuang
force-pushed
the
feature/op_perf
branch
from
308bb4c to
1645d00
Compare
I’m not sure if "fnv" is the most suitable, but index in SimpleFS is relatively small, using a more complex algorithm might not provide significant benefits. I think fnv is a reasonable balance between simplicity and performance. |
RoyWFHuang
force-pushed
the
feature/op_perf
branch
2 times, most recently
from
ca74c03 to
51e0478
Compare
visitorckw
left a comment
visitorckw
left a comment
There was a problem hiding this comment.
You ignored many of my comments without making any changes or providing any replies. You still retained many irrelevant changes, making the review difficult. Additionally, a single-line commit message saying only "optimize the file search process" is way too vague. Please improve the git commit message.
RoyWFHuang
force-pushed
the
feature/op_perf
branch
2 times, most recently
from
0519d61 to
864a9a1
Compare
Added all hash results into the commit. |
RoyWFHuang
force-pushed
the
feature/op_perf
branch
2 times, most recently
from
56c0522 to
0176a4b
Compare
RoyWFHuang
force-pushed
the
feature/op_perf
branch
from
0176a4b to
c2316df
Compare
visitorckw
left a comment
visitorckw
left a comment
There was a problem hiding this comment.
Quoted from patch 2:
Align the print function with the Simplefs print format for consistency.
Also adjust variable declarations to fix compiler warnings when building
under the C90 standard.
I'm unsure which Linux kernel versions simplefs currently intends to support, but AFAIK, the Linux kernel currently uses gnu c11 as its standard.
Furthermore, the word "Also" is often a sign that the change should be in a separate patch. In my view, you are performing two distinct actions here:
a) Changing printk -> pr_err.
b) Fixing a compiler warning.
I also remain confused as to whether the printk to pr_err change is truly warranted, and what relevance it has to the PR's title, which is "Use hash func to boost file creation and lookup".
RoyWFHuang
force-pushed
the
feature/op_perf
branch
from
c2316df to
c51cbb1
Compare
RoyWFHuang
force-pushed
the
feature/op_perf
branch
2 times, most recently
from
211edce to
d02e46a
Compare
|
Hmmm, I see my comments were marked as resolved, but I don't see any changes or replies. Since you re-requested a review, did you perhaps forget to push the new changes, or am I missing something? |
I replied to the comments to clarify why I added ei_nr and why I am still using simplefs_hash. Could you please help check it? |
|
I checked my email and didn't see any replies between the two review requests. I also checked the gihub web page and expanded the 'show resolved' threads on my last two comments, but I still don't see a response there. t seems like somehow your reply didn't go through, or maybe I just can't see it on my end? |
Sorry, I forgot to submit these comments |
|
|
||
| for (idx_ei = 0; idx_ei < SIMPLEFS_MAX_EXTENTS; _ei++, idx_ei++) { | ||
| if (unlikely(_ei >= SIMPLEFS_MAX_EXTENTS)) | ||
| _ei %= SIMPLEFS_MAX_EXTENTS; |
There was a problem hiding this comment.
The wraparound search (_ei %= SIMPLEFS_MAX_EXTENTS) handles collisions, but the worst case is still O(n) if many files hash to the same bucket.
Consider documenting expected collision rates with your benchmark files.
There was a problem hiding this comment.
This pattern repeats 6+ times. A helper function would reduce the 300+ line.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Each `simplefs_extent` structure contains a counter that records the total number of files within that extent. When the counter matches the expected file number, it indicates there are no more files after this index, allowing the iterator to skip directly to the next extension block. This reduces unnecessary scanning and improves traversal efficiency.
RoyWFHuang
force-pushed
the
feature/op_perf
branch
2 times, most recently
from
bb3ff57 to
cebc007
Compare
Introduce a hash-based mechanism to speed up file creation and lookup operations. The hash function enables faster access to extent and logical block extent index, improving overall filesystem performance. hash_code = file_hash(file_name); extent index = hash_code / SIMPLEFS_MAX_BLOCKS_PER_EXTENT block index = hash_code % SIMPLEFS_MAX_BLOCKS_PER_EXTENT; Use perf to measure: 1. File Creation (random) Legacy: 259.842753513 seconds time elapsed 23.000247000 seconds user 150.380145000 seconds sys full_name_hash: 222.274028604 seconds time elapsed 20.794966000 seconds user 151.941876000 seconds sys 2. File Listing (random) Legacy: min time: 0.00171 s max time: 0.03799 s avg time: 0.00423332 s tot time: 129.539510 s full_name_hash: min time: 0.00171 s max time: 0.03588 s avg time: 0.00305601 s tot time: 93.514040 s 3. files Removal (Random) Legacy: 106.921706288 seconds time elapsed 16.987883000 seconds user 91.268661000 seconds sys full_name_hash: 86.132655220 seconds time elapsed 19.180209000 seconds user 68.476075000 seconds sys
RoyWFHuang
force-pushed
the
feature/op_perf
branch
from
cebc007 to
304f288
Compare
There was a problem hiding this comment.
3 issues found across 5 files
Prompt for AI agents (all 3 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="inode.c">
<violation number="1" location="inode.c:742">
Clearing an empty extent in unlink never marks the parent directory block dirty (and the buffer is leaked), so the reclaimed extent is not persisted and can lead to metadata corruption.</violation>
<violation number="2" location="inode.c:900">
Rename now fails for directories that are already full, even when the rename occurs within the same directory and should succeed.</violation>
<violation number="3" location="inode.c:957">
bh_fei_blk_src acquired during rename is never released, leaking a buffer_head and pinning the source directory block.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| goto rm_new; | ||
|
|
||
| /* if extent[i] file number is 0, reclaim the extent[i] block*/ | ||
| bh_fei_blk_src = sb_bread(sb, ci_src->ei_block); |
There was a problem hiding this comment.
bh_fei_blk_src acquired during rename is never released, leaking a buffer_head and pinning the source directory block.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 957:
<comment>bh_fei_blk_src acquired during rename is never released, leaking a buffer_head and pinning the source directory block.</comment>
<file context>
@@ -753,186 +833,187 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+ goto rm_new;
+
+ /* if extent[i] file number is 0, reclaim the extent[i] block*/
+ bh_fei_blk_src = sb_bread(sb, ci_src->ei_block);
+ if (!bh_fei_blk_src) {
+ ret = -EIO;
</file context>
|
|
||
| /* If new directory is full, fail */ | ||
| if (new_pos < 0 && eblock_new->nr_files == SIMPLEFS_FILES_PER_EXT) { | ||
| if (eblk_dest->nr_files == SIMPLEFS_MAX_SUBFILES) { |
There was a problem hiding this comment.
Rename now fails for directories that are already full, even when the rename occurs within the same directory and should succeed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 900:
<comment>Rename now fails for directories that are already full, even when the rename occurs within the same directory and should succeed.</comment>
<file context>
@@ -753,186 +833,187 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+
/* If new directory is full, fail */
- if (new_pos < 0 && eblock_new->nr_files == SIMPLEFS_FILES_PER_EXT) {
+ if (eblk_dest->nr_files == SIMPLEFS_MAX_SUBFILES) {
ret = -EMLINK;
goto release_new;
</file context>
| if (eblk_dest->nr_files == SIMPLEFS_MAX_SUBFILES) { | |
| if (dest_dir != src_dir && eblk_dest->nr_files == SIMPLEFS_MAX_SUBFILES) { |
| eblk = (struct simplefs_file_ei_block *) bh->b_data; | ||
| if (!eblk->extents[ei].nr_files) { | ||
| put_blocks(sbi, eblk->extents[ei].ee_start, eblk->extents[ei].ee_len); | ||
| memset(&eblk->extents[ei], 0, sizeof(struct simplefs_extent)); |
There was a problem hiding this comment.
Clearing an empty extent in unlink never marks the parent directory block dirty (and the buffer is leaked), so the reclaimed extent is not persisted and can lead to metadata corruption.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 742:
<comment>Clearing an empty extent in unlink never marks the parent directory block dirty (and the buffer is leaked), so the reclaimed extent is not persisted and can lead to metadata corruption.</comment>
<file context>
@@ -656,9 +727,20 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+ eblk = (struct simplefs_file_ei_block *) bh->b_data;
+ if (!eblk->extents[ei].nr_files) {
+ put_blocks(sbi, eblk->extents[ei].ee_start, eblk->extents[ei].ee_len);
+ memset(&eblk->extents[ei], 0, sizeof(struct simplefs_extent));
+ }
</file context>
| bh = sb_bread(sb, ci_dir->ei_block); | ||
| if (!bh) | ||
| return ERR_PTR(-EIO); | ||
|
|
||
| eblock = (struct simplefs_file_ei_block *) bh->b_data; | ||
| bh2 = sb_bread(sb, eblock->extents[ei].ee_start + bi); | ||
| if (!bh2) | ||
| return ERR_PTR(-EIO); |
There was a problem hiding this comment.
Buffer head leak in simplefs_lookup(): When bh2 read fails, bh is never released.
| goto release_new; | ||
|
|
||
| rm_new: | ||
| simplefs_remove_from_dir(dest_dir, dest_dentry, &dest_ei); |
There was a problem hiding this comment.
dest_dentry was never inserted - it has no inode. simplefs_remove_from_dir() calls d_inode(dentry) unconditionally. Kernel panic on any rename
error after simplefs_put_new_ext().
| hash_code = simplefs_hash(dentry) % | ||
| (SIMPLEFS_MAX_EXTENTS * SIMPLEFS_MAX_BLOCKS_PER_EXTENT); |
There was a problem hiding this comment.
full_name_hash() is designed for VFS dentry caching, not for on-disk indexing. It uses a per-boot salt that changes on reboot. Consider: what happens to hash placement after reboot?
If full_name_hash() returns different values after reboot (it does), lookups will start from wrong positions. The linear probe fallback saves correctness, but destroys the performance benefit.
There was a problem hiding this comment.
Use a deterministic hash like FNV-1a or djb2.
|
@cubic-dev-ai Continue analyzing and warning until:
|
There was a problem hiding this comment.
4 issues found across 5 files
Prompt for AI agents (all 4 issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="inode.c">
<violation number="1" location="inode.c:735">
The buffer head fetched for the parent directory in unlink is never brelse’d before bh is reused, so every unlink leaks a buffer and can leave it pinned on error paths.</violation>
<violation number="2" location="inode.c:742">
After freeing an empty extent in unlink, the code zeros the extent fields but never marks the buffer dirty, so the on-disk directory index still references the freed blocks.</violation>
<violation number="3" location="inode.c:957">
The source directory extent buffer acquired in rename (bh_fei_blk_src) is never released, leading to a persistent buffer-head leak for every cross-directory rename.</violation>
<violation number="4" location="inode.c:1006">
Rolling back a failed rename calls simplefs_remove_from_dir on dest_dentry even though it has no inode, causing simplefs_remove_from_dir to dereference NULL and crash.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| goto rm_new; | ||
|
|
||
| /* if extent[i] file number is 0, reclaim the extent[i] block*/ | ||
| bh_fei_blk_src = sb_bread(sb, ci_src->ei_block); |
There was a problem hiding this comment.
The source directory extent buffer acquired in rename (bh_fei_blk_src) is never released, leading to a persistent buffer-head leak for every cross-directory rename.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 957:
<comment>The source directory extent buffer acquired in rename (bh_fei_blk_src) is never released, leading to a persistent buffer-head leak for every cross-directory rename.</comment>
<file context>
@@ -753,186 +833,187 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+ goto rm_new;
+
+ /* if extent[i] file number is 0, reclaim the extent[i] block*/
+ bh_fei_blk_src = sb_bread(sb, ci_src->ei_block);
+ if (!bh_fei_blk_src) {
+ ret = -EIO;
</file context>
| if (ret != 0) | ||
| return ret; | ||
| /* if extent[i] file number is 0, reclaim the extent[i] block*/ | ||
| bh = sb_bread(sb, SIMPLEFS_INODE(dir)->ei_block); |
There was a problem hiding this comment.
The buffer head fetched for the parent directory in unlink is never brelse’d before bh is reused, so every unlink leaks a buffer and can leave it pinned on error paths.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 735:
<comment>The buffer head fetched for the parent directory in unlink is never brelse’d before bh is reused, so every unlink leaks a buffer and can leave it pinned on error paths.</comment>
<file context>
@@ -656,9 +727,20 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
if (ret != 0)
return ret;
+ /* if extent[i] file number is 0, reclaim the extent[i] block*/
+ bh = sb_bread(sb, SIMPLEFS_INODE(dir)->ei_block);
+ if (!bh)
+ return -EIO;
</file context>
| goto release_new; | ||
|
|
||
| rm_new: | ||
| simplefs_remove_from_dir(dest_dir, dest_dentry, &dest_ei); |
There was a problem hiding this comment.
Rolling back a failed rename calls simplefs_remove_from_dir on dest_dentry even though it has no inode, causing simplefs_remove_from_dir to dereference NULL and crash.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 1006:
<comment>Rolling back a failed rename calls simplefs_remove_from_dir on dest_dentry even though it has no inode, causing simplefs_remove_from_dir to dereference NULL and crash.</comment>
<file context>
@@ -753,186 +833,187 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+ goto release_new;
+
+rm_new:
+ simplefs_remove_from_dir(dest_dir, dest_dentry, &dest_ei);
put_block:
</file context>
| eblk = (struct simplefs_file_ei_block *) bh->b_data; | ||
| if (!eblk->extents[ei].nr_files) { | ||
| put_blocks(sbi, eblk->extents[ei].ee_start, eblk->extents[ei].ee_len); | ||
| memset(&eblk->extents[ei], 0, sizeof(struct simplefs_extent)); |
There was a problem hiding this comment.
After freeing an empty extent in unlink, the code zeros the extent fields but never marks the buffer dirty, so the on-disk directory index still references the freed blocks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At inode.c, line 742:
<comment>After freeing an empty extent in unlink, the code zeros the extent fields but never marks the buffer dirty, so the on-disk directory index still references the freed blocks.</comment>
<file context>
@@ -656,9 +727,20 @@ static int simplefs_unlink(struct inode *dir, struct dentry *dentry)
+ eblk = (struct simplefs_file_ei_block *) bh->b_data;
+ if (!eblk->extents[ei].nr_files) {
+ put_blocks(sbi, eblk->extents[ei].ee_start, eblk->extents[ei].ee_len);
+ memset(&eblk->extents[ei], 0, sizeof(struct simplefs_extent));
+ }
</file context>
3 participants
Previously, SimpleFS used a sequential insertion method to create files, which worked efficiently when the filesystem contained only a small number of files.
However, in real-world use cases, filesystems often manage a large number of files, making sequential search and insertion inefficient.
Inspired by Ext4’s hash-based directory indexing, this change adopts a hash function to accelerate file indexing and improve scalability.
Change:
Implemented hash-based file index lookup
Improved scalability for large directory structures
hash_code = file_hash(file_name);
extent index = hash_code / SIMPLEFS_MAX_BLOCKS_PER_EXTENT
block index = hash_code % SIMPLEFS_MAX_BLOCKS_PER_EXTENT;
Performance test
legacy:
full_name_hash
legacy:
full_name_hash
Use perf stat ls -la to measure the query time for each file and sum up all elapsed times to calculate the total lookup cost.
Legacy :
min time: 0.00171 s
max time: 0.03799 s
avg time: 0.00423332 s
tot time: 129.539510 s
full_name_hash:
min time: 0.00171 s
max time: 0.03588 s
avg time: 0.00305601 s
tot time: 93.514040 s
Summary by cubic
Switched SimpleFS to hash-based directory indexing using full_name_hash to speed up file creation and lookup by mapping filenames to extent/block slots. On 30.6k files: create ~33% faster, delete ~12% faster, lookup ~41% faster.
New Features
Refactors
Written for commit 304f288. Summary will update automatically on new commits.