feat(caching): implement negative stat cache to optimize polling of missing files#4729
feat(caching): implement negative stat cache to optimize polling of missing files#4729alleaditya wants to merge 87 commits into
Conversation
…issing files This change adds negative caching to StatCache, short-circuits LookUpChild on negative hits, and adds metrics/integration tests to verify.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces negative stat caching to optimize performance in scenarios where applications frequently poll for non-existent files. By caching negative results (404s) and short-circuiting lookups in the filesystem layer, the change significantly reduces redundant backend network traffic. The implementation includes configurable TTL settings and comprehensive integration tests to ensure correctness and efficiency. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements negative stat-cache functionality to optimize lookups for non-existent files and directories, reducing redundant backend GCS requests. Key changes include updates to LookUpChild in dir.go to short-circuit on confirmed negative hits, and logic in fast_stat_bucket.go to manage negative cache entries. Documentation and tests were also added to support this feature. Review feedback suggests removing speculative negative caching in insertListing to avoid correctness issues with paginated GCS listings and adding a warning in the documentation regarding the risks of using an infinite TTL for negative entries.
…s blocking CI - Remove speculative negative caching from listing path to avoid pagination bugs (addressed review comment). - Add warning about infinite negative TTL usage in semantics.md (addressed review comment). - Fix deadlock in createFile's defer in fs.go when error occurs before child inode is minted. - Fix data race in downloader Job by deep-copying MinObject, preventing race with reader thread. - Fix missing lock in fake bucket's MoveObject, resolving race with StatObject.
…hing, add doc warning - Revert data race fixes in downloader Job (moved to separate PR). - Revert name length checks and deadlock fix in fs.go (moved to separate PR). - Revert name length checks in dir.go (moved to separate PR), keeping only LookUpChild short-circuiting. - Revert MoveObject lock fix in fake bucket (moved to separate PR), keeping FetchOnlyFromCache checks. - Remove speculative negative caching on empty directory listing to avoid pagination bugs (addressed review comment). - Add warning about infinite TTL usage for negative caching in semantics.md (addressed review comment).
alleaditya
added
execute-perf-test
…_bucket Address PR comments by avoiding changes to inode/dir.go and limiting negative caching to fast_stat_bucket. Skip negative caching for HNS buckets.
| setup.SaveGCSFuseLogFileInCaseOfFailure(s.T()) | ||
| // Wait for the metadata cache TTL to expire so that negatively cached entries | ||
| // don't pollute subsequent tests. | ||
| time.Sleep(2500 * time.Millisecond) |
There was a problem hiding this comment.
Why do we need this change? Also, kind of brittle if someone change the value of metadata-cache-ttl.
There was a problem hiding this comment.
Prince, removing this is causing denrty flat bucket test failures
"Running tests with flags: [--implicit-dirs --experimental-enable-dentry-cache --metadata-cache-ttl-secs=2 --client-protocol=grpc --metadata-cache-negative-ttl-secs=2]"
Why time.Sleep(2100 * time.Millisecond) is required in TestStatWithDentryCacheEnabled:
- Stale Cache Prevention: The test suite configures a 2-second stat cache TTL (
--metadata-cache-ttl-secs=2). Because the test updates the file out-of-band directly on GCS (client.WriteToObject), GCSFuse is unaware of the modification. Sleeping>2sensures the cached metadata expires so the subsequentstatcall fetches the updated object size from GCS rather than returning a stale cache hit. - Preventing Cascading Rate Limits & Cache Pollution: Removing the sleep causes the test to fail immediately and disrupt timing intervals between test iterations (
#01). This leads to GCS backend throttling (HTTP 429 Too Many Requests) during directory setup and causes negative dentry cache lookups (ENOENT) from preceding cleanup phases to bleed into subsequent test runs.
There was a problem hiding this comment.
Why we were not seeing the failure before? Is it because of negative implicit-directory caching? And if the test is not implemented with negative implicit-directory caching then we can explicitly disable it.
raj-prince
left a comment
raj-prince
left a comment
There was a problem hiding this comment.
Changes looks good, few minor comments.
…lify inode lookup logic
…lify inode lookup logic
…ing for objects without trailing slashes
alleaditya
added
execute-perf-test
alleaditya
force-pushed
the
add-negative-stat-cache
branch
from
962835f to
f36031a
Compare
alleaditya
removed
execute-integration-tests-on-zb
alleaditya
added
execute-integration-tests-on-zb
alleaditya
force-pushed
the
add-negative-stat-cache
branch
from
5d74924 to
4d4fc09
Compare
2 participants
Description
This change implements negative entry caching (non-existent path caching) to optimize workloads that aggressively poll missing files (e.g., JupyterLab).
• Activation: Controlled by
metadata-cache: negative-ttl-secsconfig parameter (or--metadata-cache-negative-ttl-secsflag). It is enabled by default with a 5-second TTL. Setting it to0disables the feature.• Proactive Listing Cache: Empty directory listings (
ListObjectsreturning 0 results) are proactively cached as negative directory entries to fully protect against implicit directory network probes.• VFS Routing:
LookUpChildtracks definitive negative cache hits and short-circuits immediately in memory, avoiding network fallback.Benchmark Results
A custom sustained benchmarking script (
bench.sh) was executed against a locally mounted test bucket to measure performance over sustained 60-second polling windows.The metrics demonstrate that negative caching successfully eliminates redundant network traffic across all lookup scenarios, including those requiring implicit directory checking.
1. Implicit Dirs vs Explicit Dirs (Throughput & Network Calls)
In
--implicit-dirsscenarios, FUSE previously required both aStatObjectand a fallbackListObjectscall per lookup. With this change, both are aggressively short-circuited:StatObjectcalls: 1,544ListObjectscalls: 1StatObjectcalls: 24ListObjectscalls: 1Throughput: +109,591% (>1,000x)
StatObjectcalls: 736ListObjectscalls: 737StatObjectcalls: 12ListObjectscalls: 15Throughput: +118,912% (>1,000x)
(Note: Network calls exactly match mathematically expected TTL expiration rates - approximately 1 network call per 5 seconds).
Link to the issue in case of a bug fix.
https://b.corp.google.com/issues/511786738
Testing details
--implicit-dirs, validating exact TTL expiration counts and throughput improvements.TestLookUpChild_NegativeCacheHitindir_test.goandTestGCSMetrics_RequestCount_NegativeCachingShortCircuitingcs_metrics_test.go. Verified negative caching upon empty listings inListObjectsTest_InsertListing.EmptyListing.Any backward incompatible change? If so, please explain.
No.