filer: opt-in inode->path index with shell nfs.enable / nfs.disable#10277
Conversation
…ions The NFS gateway resolves filehandles by reversing an inode to its path, which the deterministic hash inode cannot do. Bring back the reverse index, but scoped: rows are written only for entries under a filer.conf location with inode_index set, so with no such rule the filer does no extra work. All filers pick the rule up through the usual filer.conf reload, with no per-process flags to keep in sync. Claude-Session: https://claude.ai/code/session_01AQDAwEYZsGsmGZKo3myZxB
One command to make a filer path exportable over NFS: nfs.enable marks the path with inode_index in filer.conf and backfills index rows for the entries already under it, assigning inodes where missing, so existing files resolve by filehandle right away. nfs.disable clears the mark and deletes the rows, leaving no index data behind. Claude-Session: https://claude.ai/code/session_01AQDAwEYZsGsmGZKo3myZxB
📝 WalkthroughWalkthroughThis PR adds an inode-to-path reverse index for NFS filehandle resolution. It extends proto schemas with an ChangesInode Index for NFS Filehandle Resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ShellUser
participant NfsEnableCommand
participant Filer
participant FilerStoreWrapper
participant KVStore
ShellUser->>NfsEnableCommand: nfs.enable -path /exports -apply
NfsEnableCommand->>Filer: read filer.conf
NfsEnableCommand->>Filer: save updated filer.conf (inode_index=true)
NfsEnableCommand->>Filer: TraverseBfs export path
loop each entry
NfsEnableCommand->>Filer: LookupDirectoryEntry / UpdateEntry (assign inode)
NfsEnableCommand->>KVStore: KvGet inode index key
NfsEnableCommand->>KVStore: KvPut updated InodeIndexRecord
end
NfsEnableCommand-->>ShellUser: totals (indexed rows, assigned inodes)
ShellUser->>FilerStoreWrapper: InsertEntry / UpdateEntry / DeleteEntry
FilerStoreWrapper->>Filer: check FilerConf.InodeIndexActiveUnder(path)
FilerStoreWrapper->>KVStore: recordInodeIndexWrite / recordInodeIndexRemoval
KVStore-->>FilerStoreWrapper: ack
FilerStoreWrapper-->>ShellUser: operation result
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request introduces an opt-in secondary inode_index mechanism to maintain reverse inode-to-path lookups, supporting the NFS gateway. It updates the protobuf definitions, modifies FilerStoreWrapper to handle index updates during entry mutations, and adds nfs.enable and nfs.disable shell commands for configuration and index backfilling/cleanup. Feedback highlights two key issues: a prefix-matching bug in InodeIndexActiveUnder that could incorrectly match sibling directories sharing a common prefix, and a missing context cancellation check in the recursive directory walk loop of collectInodeIndexEntriesRecursive.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | ||
| if value.InodeIndex && strings.HasPrefix(string(key), dirPath) { | ||
| active = true | ||
| return false | ||
| } | ||
| return true | ||
| }) |
There was a problem hiding this comment.
The current prefix check strings.HasPrefix(string(key), dirPath) can lead to incorrect matches for sibling directories that share a common prefix (e.g., /exports-other would match when checking for /exports). Since all paths are absolute, we should ensure we only match exact paths or true subdirectories by checking for a trailing slash / boundary.
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | |
| if value.InodeIndex && strings.HasPrefix(string(key), dirPath) { | |
| active = true | |
| return false | |
| } | |
| return true | |
| }) | |
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | |
| keyStr := string(key) | |
| if value.InodeIndex && (dirPath == "/" || keyStr == dirPath || strings.HasPrefix(keyStr, dirPath+"/")) { | |
| active = true | |
| return false | |
| } | |
| return true | |
| }) |
| for { | ||
| page := make([]*Entry, 0, PaginationSize) |
There was a problem hiding this comment.
The comment in collectInodeIndexEntries states that caller cancellation is honored during the walk to prevent DoS vectors on pathological directories. However, the recursive helper collectInodeIndexEntriesRecursive does not check ctx.Err() or ctx.Done() in its loop. We should check ctx.Err() at the start of each iteration to actually honor the cancellation.
for {
if err := ctx.Err(); err != nil {
return err
}
page := make([]*Entry, 0, PaginationSize)There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
weed/filer/filer_inode_index.go (1)
262-276: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffConsider streaming instead of accumulating the whole subtree in memory.
collectInodeIndexEntriesbuffers every in-scope entry of the subtree into a single[]inodeIndexEntrybefore the caller processes it. For a large indexed directory tree (aDeleteFolderChildrenon a big-path=/exports), this is an unbounded allocation proportional to the number of descendants. A visitor/callback that removes index rows as entries are discovered would bound memory. Deferrable, but worth considering before this backs the NFS gateway at scale.🤖 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 `@weed/filer/filer_inode_index.go` around lines 262 - 276, The inode index cleanup path currently buffers the entire subtree in collectInodeIndexEntries, which can grow without bound for large directories. Refactor the traversal in collectInodeIndexEntries and collectInodeIndexEntriesRecursive to stream entries to a visitor/callback that processes and deletes each inodeIndexEntry as it is discovered, instead of appending everything into a single slice. Keep the existing context-cancellation behavior, but make the caller consume entries incrementally so memory use stays bounded.
🤖 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 `@weed/filer/filer_conf.go`:
- Around line 241-254: `FilerConf.InodeIndexActiveUnder` is using a raw prefix
match that can treat sibling paths as descendants. Update the walk logic in
`InodeIndexActiveUnder` to only consider paths that are actually under `dirPath`
by checking a path boundary after the prefix (for example, exact match or
separator-delimited descendant), while keeping the existing `MatchStorageRule`
fast path intact. Reference the `InodeIndexActiveUnder` method and its
`fc.rules.Walk` callback when making the fix.
In `@weed/filer/filer_inode_index.go`:
- Around line 278-310: The recursive inode-index walk in
collectInodeIndexEntriesRecursive should stop promptly when the caller cancels,
since this path bypasses context.WithoutCancel and may keep paging in backends
that do not poll ctx. Add a ctx.Err() check in the paging loop (before listing
and/or before recursing) and return that error immediately so the walk exits
cleanly on cancellation. Keep the fix localized to
collectInodeIndexEntriesRecursive and preserve the existing pagination and
recursion behavior otherwise.
In `@weed/shell/command_nfs_enable.go`:
- Around line 183-219: The inode row update path in upsertInodeIndexRow is racy
because TraverseBfs can invoke it concurrently for the same inode, allowing
overlapping KvGet/AddPath/KvPut cycles to overwrite one another. Serialize the
read-modify-write per inode by adding a per-inode lock around the
WithFilerClient block in upsertInodeIndexRow, or otherwise make the traversal
single-threaded before calling it. Also ensure the shared writer used by the
progress output is protected with synchronization if it remains enabled during
concurrent traversal.
---
Nitpick comments:
In `@weed/filer/filer_inode_index.go`:
- Around line 262-276: The inode index cleanup path currently buffers the entire
subtree in collectInodeIndexEntries, which can grow without bound for large
directories. Refactor the traversal in collectInodeIndexEntries and
collectInodeIndexEntriesRecursive to stream entries to a visitor/callback that
processes and deletes each inodeIndexEntry as it is discovered, instead of
appending everything into a single slice. Keep the existing context-cancellation
behavior, but make the caller consume entries incrementally so memory use stays
bounded.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd19cde7-81af-4fe2-b434-5ee32bac12e2
⛔ Files ignored due to path filters (1)
weed/pb/filer_pb/filer.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
other/java/client/src/main/proto/filer.protoweed/filer/filer.goweed/filer/filer_conf.goweed/filer/filer_conf_test.goweed/filer/filer_inode_index.goweed/filer/filer_inode_index_test.goweed/filer/filerstore_wrapper.goweed/pb/filer.protoweed/shell/command_nfs_disable.goweed/shell/command_nfs_enable.go
| func (fc *FilerConf) InodeIndexActiveUnder(dirPath string) bool { | ||
| if fc.MatchStorageRule(dirPath).InodeIndex { | ||
| return true | ||
| } | ||
| active := false | ||
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | ||
| if value.InodeIndex && strings.HasPrefix(string(key), dirPath) { | ||
| active = true | ||
| return false | ||
| } | ||
| return true | ||
| }) | ||
| return active | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
InodeIndexActiveUnder prefix check doesn't respect path boundaries.
strings.HasPrefix(string(key), dirPath) will produce false positives for sibling paths. For example, if a rule exists for /exports_old with InodeIndex=true, calling InodeIndexActiveUnder("/exports") would incorrectly return true because strings.HasPrefix("/exports_old", "/exports") is true.
This won't cause data corruption (the per-entry MatchStorageRule check would still return false for entries under /exports), but it causes unnecessary index maintenance code paths to be entered for out-of-scope directories.
🛡️ Proposed fix for path boundary check
func (fc *FilerConf) InodeIndexActiveUnder(dirPath string) bool {
if fc.MatchStorageRule(dirPath).InodeIndex {
return true
}
active := false
fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool {
- if value.InodeIndex && strings.HasPrefix(string(key), dirPath) {
+ if value.InodeIndex && (dirPath == "/" || strings.HasPrefix(string(key), dirPath+"/")) {
active = true
return false
}
return true
})
return active
}📝 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.
| func (fc *FilerConf) InodeIndexActiveUnder(dirPath string) bool { | |
| if fc.MatchStorageRule(dirPath).InodeIndex { | |
| return true | |
| } | |
| active := false | |
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | |
| if value.InodeIndex && strings.HasPrefix(string(key), dirPath) { | |
| active = true | |
| return false | |
| } | |
| return true | |
| }) | |
| return active | |
| } | |
| func (fc *FilerConf) InodeIndexActiveUnder(dirPath string) bool { | |
| if fc.MatchStorageRule(dirPath).InodeIndex { | |
| return true | |
| } | |
| active := false | |
| fc.rules.Walk(func(key []byte, value *filer_pb.FilerConf_PathConf) bool { | |
| if value.InodeIndex && (dirPath == "/" || strings.HasPrefix(string(key), dirPath+"/")) { | |
| active = true | |
| return false | |
| } | |
| return true | |
| }) | |
| return active | |
| } |
🤖 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 `@weed/filer/filer_conf.go` around lines 241 - 254,
`FilerConf.InodeIndexActiveUnder` is using a raw prefix match that can treat
sibling paths as descendants. Update the walk logic in `InodeIndexActiveUnder`
to only consider paths that are actually under `dirPath` by checking a path
boundary after the prefix (for example, exact match or separator-delimited
descendant), while keeping the existing `MatchStorageRule` fast path intact.
Reference the `InodeIndexActiveUnder` method and its `fc.rules.Walk` callback
when making the fix.
| func (fsw *FilerStoreWrapper) collectInodeIndexEntriesRecursive(ctx context.Context, dirPath util.FullPath, collected *[]inodeIndexEntry) error { | ||
| actualStore := fsw.getActualStore(dirPath + "/") | ||
|
|
||
| lastFileName := "" | ||
| includeStartFile := false | ||
| for { | ||
| page := make([]*Entry, 0, PaginationSize) | ||
| nextLastFileName, err := actualStore.ListDirectoryEntries(ctx, dirPath, lastFileName, includeStartFile, PaginationSize, func(entry *Entry) (bool, error) { | ||
| page = append(page, entry) | ||
| return true, nil | ||
| }) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| for _, entry := range page { | ||
| if entry.Attr.Inode != 0 { | ||
| *collected = append(*collected, inodeIndexEntry{path: entry.FullPath, inode: entry.Attr.Inode}) | ||
| } | ||
| if entry.IsDirectory() { | ||
| if err := fsw.collectInodeIndexEntriesRecursive(ctx, entry.FullPath, collected); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(page) < PaginationSize { | ||
| return nil | ||
| } | ||
| lastFileName = nextLastFileName | ||
| includeStartFile = false | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether ListDirectoryEntries implementations honor ctx cancellation.
rg -nP '\bfunc\b.*\bListDirectoryEntries\b' --type=go -A15Repository: seaweedfs/seaweedfs
Length of output: 157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file context =="
sed -n '240,340p' weed/filer/filer_inode_index.go
echo
echo "== all ListDirectoryEntries references =="
rg -n --hidden --glob '!**/*vendor*' 'ListDirectoryEntries' .
echo
echo "== all ctx.Err or cancellation checks in filer inode index file =="
rg -n 'ctx\.Err|context\.Canceled|context\.DeadlineExceeded' weed/filer/filer_inode_index.go
echo
echo "== definitions of directory listing methods/interfaces =="
rg -n --hidden --glob '!**/*vendor*' 'type .*ListDirectoryEntries|func .*ListDirectoryEntries|ListDirectoryEntries\(' weedRepository: seaweedfs/seaweedfs
Length of output: 23445
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== interface and wrapper =="
sed -n '1,120p' weed/filer/filerstore.go
echo
sed -n '340,460p' weed/filer/filerstore_wrapper.go
echo
echo "== filer search/listing =="
sed -n '1,180p' weed/filer/filer_search.go
echo
echo "== leveldb3 implementation cancellation =="
sed -n '300,420p' weed/filer/leveldb3/leveldb3_store.go
echo
echo "== foundationdb implementation cancellation =="
sed -n '560,700p' weed/filer/foundationdb/foundationdb_store.go
echo
echo "== cassandra implementation cancellation =="
sed -n '220,320p' weed/filer/cassandra2/cassandra_store.goRepository: seaweedfs/seaweedfs
Length of output: 21537
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== getActualStore and related helpers =="
rg -n 'func \(fsw \*FilerStoreWrapper\) getActualStore|func \(fsw \*FilerStoreWrapper\) inodeIndexUnder|func \(fsw \*FilerStoreWrapper\) inodeIndexInScope|collectInodeIndexEntries\(' weed/filer/filer_inode_index.go weed/filer/filerstore_wrapper.go weed/filer -A40
echo
echo "== DeleteFolderChildren call path =="
rg -n 'collectInodeIndexEntries\(|DeleteFolderChildren\(' weed/filer -A20
echo
echo "== any explicit ctx.Err in directory listing implementations =="
rg -n 'ctx\.Err\(\)' weed/filer/{leveldb,leveldb2,leveldb3,rocksdb,cassandra,cassandra2,foundationdb,elastic,v7,redis,redis2,redis3,tikv,arangodb,mongodb,hbase,abstract_sql,etcd,ydb} -g '*.go'Repository: seaweedfs/seaweedfs
Length of output: 50375
Add a ctx.Err() guard to the inode-index walk. This path bypasses the wrapper’s context.WithoutCancel, so cancellation depends on the backend polling ctx; some listing backends don’t, and the loop can keep paging after the caller has canceled.
🤖 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 `@weed/filer/filer_inode_index.go` around lines 278 - 310, The recursive
inode-index walk in collectInodeIndexEntriesRecursive should stop promptly when
the caller cancels, since this path bypasses context.WithoutCancel and may keep
paging in backends that do not poll ctx. Add a ctx.Err() check in the paging
loop (before listing and/or before recursing) and return that error immediately
so the walk exits cleanly on cancellation. Keep the fix localized to
collectInodeIndexEntriesRecursive and preserve the existing pagination and
recursion behavior otherwise.
| func upsertInodeIndexRow(commandEnv *CommandEnv, fullPath util.FullPath, inode uint64) (changed bool, err error) { | ||
| key := filer.InodeIndexKey(inode) | ||
| err = commandEnv.WithFilerClient(false, func(client filer_pb.SeaweedFilerClient) error { | ||
| resp, kvErr := client.KvGet(context.Background(), &filer_pb.KvGetRequest{Key: key}) | ||
| if kvErr != nil { | ||
| return kvErr | ||
| } | ||
| if resp.GetError() != "" { | ||
| return errors.New(resp.GetError()) | ||
| } | ||
| record, decodeErr := filer.DecodeInodeIndexRecord(resp.GetValue()) | ||
| if decodeErr != nil { | ||
| // Unreadable row: rebuild it from this path. | ||
| record = &filer.InodeIndexRecord{} | ||
| } | ||
| if !record.AddPath(fullPath) { | ||
| return nil | ||
| } | ||
| value, encodeErr := record.Encode() | ||
| if encodeErr != nil { | ||
| return encodeErr | ||
| } | ||
| putResp, putErr := client.KvPut(context.Background(), &filer_pb.KvPutRequest{Key: key, Value: value}) | ||
| if putErr != nil { | ||
| return putErr | ||
| } | ||
| if putResp.GetError() != "" { | ||
| return errors.New(putResp.GetError()) | ||
| } | ||
| changed = true | ||
| return nil | ||
| }) | ||
| if err != nil { | ||
| return false, fmt.Errorf("index inode %d for %s: %v", inode, fullPath, err) | ||
| } | ||
| return changed, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -t f filer_client_bfs.go
rg -nP 'func TraverseBfs' -A30 weed/pb/filer_pb/Repository: seaweedfs/seaweedfs
Length of output: 2174
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- filer_client_bfs.go outline ---'
ast-grep outline weed/pb/filer_pb/filer_client_bfs.go --view expanded || true
echo
echo '--- relevant TraverseBfs body ---'
sed -n '1,220p' weed/pb/filer_pb/filer_client_bfs.go
echo
echo '--- command_nfs_enable.go around the reviewed lines ---'
sed -n '100,250p' weed/shell/command_nfs_enable.go
echo
echo '--- search for writer synchronization in command_nfs_enable.go ---'
rg -n 'Fprintf|writer|mutex|lock|sync\.|atomic' weed/shell/command_nfs_enable.goRepository: seaweedfs/seaweedfs
Length of output: 8405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- inode index record definitions ---'
rg -n 'type InodeIndexRecord|func \(.*AddPath|func \(.*Encode|DecodeInodeIndexRecord|InodeIndexKey' weed -g '!**/*_test.go'
echo
echo '--- likely implementations ---'
files=$(rg -l 'type InodeIndexRecord|DecodeInodeIndexRecord|InodeIndexKey' weed -g '!**/*_test.go' | tr '\n' ' ')
for f in $files; do
echo "### $f"
sed -n '1,260p' "$f"
echo
doneRepository: seaweedfs/seaweedfs
Length of output: 29452
Serialize inode row updates before writing them back
TraverseBfs runs this callback on multiple workers, so two hard-linked entries can race on the same inode row: both KvGet, both AddPath, and the later KvPut can drop the other path. Guard the read-modify-write per inode (or make this traversal single-threaded). The shared writer also needs synchronization if concurrent progress output stays enabled.
🤖 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 `@weed/shell/command_nfs_enable.go` around lines 183 - 219, The inode row
update path in upsertInodeIndexRow is racy because TraverseBfs can invoke it
concurrently for the same inode, allowing overlapping KvGet/AddPath/KvPut cycles
to overwrite one another. Serialize the read-modify-write per inode by adding a
per-inode lock around the WithFilerClient block in upsertInodeIndexRow, or
otherwise make the traversal single-threaded before calling it. Also ensure the
shared writer used by the progress output is protected with synchronization if
it remains enabled during concurrent traversal.
|
This may work, but the additional inode->path index overhead is still unnecessary. Not planning to support this in the long term. The enterprise version uses kernel module VFS, which can avoid this persistent inode problem from NFS. |
Groundwork for bringing back the
weed nfsgateway. The gateway resolves NFS filehandles by reversing an inode to its path, which the deterministic hash inode cannot do, so the filer needs a reverse index — but nobody should pay for it unless they export something.PathConfgains aninode_indexoption. The store wrapper writes inode->path rows only for entries under a marked location; with no rule (the default) no index work happens at all. Every filer picks the rule up through the normal filer.conf reload, so there is no per-process flag to keep in sync across a cluster.nfs.enable -path=/exports -applymarks the path and backfills rows for the entries already under it, assigning inodes to entries that predate filer-assigned inodes, so pre-existing files resolve by filehandle immediately. Re-running is idempotent.nfs.disable -path=/exports -applyremoves the mark and deletes the rows, so turning NFS off leaves no index data behind.Verified against a live server: enable backfills exactly the in-scope entries, files created afterwards get rows through the filer-side hook, out-of-scope paths get nothing, and disable clears every row.
The gateway restore itself comes as a follow-up on top of this.
https://claude.ai/code/session_01AQDAwEYZsGsmGZKo3myZxB
Summary by CodeRabbit
New Features
Bug Fixes