Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions lib/fs/async_fs.rs
Original file line number Diff line number Diff line change
Expand Up @@ -447,9 +447,11 @@ impl<DP: FsDataProvider> AsyncFs<DP> {
/// Asynchronously look up an inode by name within a parent directory.
///
/// Resolution order:
/// 1. Directory cache (synchronous fast path)
/// 2. Lookup cache (`get_or_try_init` — calls `dp.lookup()` only on a true miss)
/// 3. On success, populates inode table and directory cache
/// 1. Directory cache hit — returns immediately.
/// 2. Directory cache miss in a fully-populated directory — returns `ENOENT`
/// immediately without hitting the remote data provider.
/// 3. Lookup cache (`get_or_try_init` — calls `dp.lookup()` only on a true miss)
/// 4. On success, populates inode table and directory cache
pub async fn lookup(
&self,
parent: LoadedAddr,
Expand All @@ -469,6 +471,11 @@ impl<DP: FsDataProvider> AsyncFs<DP> {
// entry so the slow path calls dp.lookup() fresh.
self.lookup_cache
.remove_sync(&(parent.addr(), Arc::from(name)));
} else if self.directory_cache.is_populated(parent) {
// The directory has been fully populated (e.g. by readdir) and this
// name was not found — the file does not exist. Short-circuit
// without hitting the remote data provider.
return Err(std::io::Error::from_raw_os_error(libc::ENOENT));
}

// Note: get_or_try_init deduplicates successful lookups but NOT
Expand Down
12 changes: 12 additions & 0 deletions lib/fs/dcache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,18 @@ impl DCache {
children.get(name).cloned()
}

/// Returns `true` if the directory at `parent_ino` has been fully
/// populated (i.e. [`finish_populate`](Self::finish_populate) completed
/// successfully and no eviction has since reset the flag).
#[must_use]
pub fn is_populated(&self, parent_ino: LoadedAddr) -> bool {
self.dirs
.read_sync(&parent_ino, |_, state| {
state.populated.load(Ordering::Acquire) == POPULATE_DONE
})
.unwrap_or(false)
}

/// Atomically inserts or overwrites a child entry in the cache.
///
/// Handles two kinds of stale-entry cleanup before the insert:
Expand Down
79 changes: 79 additions & 0 deletions tests/async_fs_correctness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -880,6 +880,50 @@ async fn readdir_evict_all_readdir_returns_same_entries() {
);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn lookup_returns_enoent_without_remote_call_when_dir_populated() {
let root = make_inode(1, INodeType::Directory, 0, None);
let child = make_inode(10, INodeType::File, 42, Some(1));

let mut state = MockFsState::default();
// Configure readdir to return one child. Do NOT configure a lookup
// entry for "nonexistent.txt". If lookup hits the remote, the mock
// will return ENOENT anyway — but we need to verify it does NOT
// call dp.lookup() at all.
state
.directories
.insert(1, vec![(OsString::from("file.txt"), child)]);
// Crucially, also register "nonexistent.txt" in lookups so that if
// the slow path fires, lookup would SUCCEED — proving the test
// catches the bug.
let fake_child = make_inode(99, INodeType::File, 1, Some(1));
state
.lookups
.insert((1, "nonexistent.txt".into()), fake_child);
let dp = MockFsDataProvider::new(state);

let table = Arc::new(FutureBackedCache::default());
let fs = AsyncFs::new(dp, root, Arc::clone(&table)).await;

// Populate the directory via readdir.
fs.readdir(LoadedAddr::new_unchecked(1), 0, |_, _| false)
.await
.unwrap();

// Lookup a name that was NOT in the readdir results.
// With the fix, this should return ENOENT from the dcache
// without ever calling dp.lookup().
let err = fs
.lookup(LoadedAddr::new_unchecked(1), OsStr::new("nonexistent.txt"))
.await
.unwrap_err();
assert_eq!(
err.raw_os_error(),
Some(libc::ENOENT),
"lookup for missing file in populated dir should return ENOENT"
);
}

/// Verify that `IndexedLookupCache::evict_addr` removes only entries
/// referencing the evicted inode (parent or child), leaving unrelated
/// entries intact. This is the O(k) replacement for the old O(N) scan.
Expand Down Expand Up @@ -1139,6 +1183,41 @@ async fn remove_sync_cleans_cache_and_parent_reverse_index() {
);
}

#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
async fn lookup_falls_through_to_remote_after_eviction_resets_populated() {
let root = make_inode(1, INodeType::Directory, 0, None);
let child_a = make_inode(10, INodeType::File, 42, Some(1));
let child_b = make_inode(11, INodeType::File, 99, Some(1));

let mut state = MockFsState::default();
state
.directories
.insert(1, vec![(OsString::from("a.txt"), child_a)]);
// "b.txt" is only reachable via lookup, NOT readdir.
state.lookups.insert((1, "b.txt".into()), child_b);
let dp = MockFsDataProvider::new(state);

let table = Arc::new(FutureBackedCache::default());
let fs = AsyncFs::new(dp, root, Arc::clone(&table)).await;

// Populate via readdir — directory is now DONE.
fs.readdir(LoadedAddr::new_unchecked(1), 0, |_, _| false)
.await
.unwrap();

// Evict child_a — this resets populate flag to UNCLAIMED.
fs.directory_cache().evict(LoadedAddr::new_unchecked(10));

// Now lookup "b.txt" — directory is no longer DONE, so the
// short-circuit must NOT fire. The slow path should call
// dp.lookup() and find "b.txt".
let result = fs
.lookup(LoadedAddr::new_unchecked(1), OsStr::new("b.txt"))
.await
.unwrap();
assert_eq!(result.inode.addr, 11);
}

/// Verify that `evict_addr(old_child)` does not spuriously remove a
/// freshly-indexed reverse entry for a *new* child that reuses the same
/// `LookupKey` `(parent, name)`.
Expand Down
59 changes: 59 additions & 0 deletions tests/dcache_correctness.rs
Original file line number Diff line number Diff line change
Expand Up @@ -740,3 +740,62 @@ async fn evict_generation_bump_prevents_stale_finish_populate() {
PopulateStatus::InProgress => panic!("unexpected InProgress"),
}
}

#[tokio::test]
async fn is_populated_returns_false_for_unknown_parent() {
let cache = DCache::new();
assert!(
!cache.is_populated(LoadedAddr::new_unchecked(1)),
"unknown parent should not be populated"
);
}

#[tokio::test]
async fn is_populated_returns_false_before_finish_populate() {
let cache = DCache::new();
let parent = LoadedAddr::new_unchecked(1);
let PopulateStatus::Claimed(_gen) = cache.try_claim_populate(parent) else {
panic!("should claim");
};
assert!(
!cache.is_populated(parent),
"in-progress directory should not be populated"
);
}

#[tokio::test]
async fn is_populated_returns_true_after_finish_populate() {
let cache = DCache::new();
let parent = LoadedAddr::new_unchecked(1);
let PopulateStatus::Claimed(claim_gen) = cache.try_claim_populate(parent) else {
panic!("should claim");
};
cache.finish_populate(parent, claim_gen);
assert!(
cache.is_populated(parent),
"directory should be populated after finish_populate"
);
}

#[tokio::test]
async fn is_populated_returns_false_after_eviction() {
let cache = DCache::new();
let parent = LoadedAddr::new_unchecked(1);
cache.insert(
parent,
OsString::from("child"),
LoadedAddr::new_unchecked(10),
false,
);
let PopulateStatus::Claimed(claim_gen) = cache.try_claim_populate(parent) else {
panic!("should claim");
};
cache.finish_populate(parent, claim_gen);
assert!(cache.is_populated(parent));

cache.evict(LoadedAddr::new_unchecked(10));
assert!(
!cache.is_populated(parent),
"eviction should reset populated flag"
);
}