feat(mount): Implement negative cache#724
Conversation
Summary of ChangesHello @ZaRdEr15, 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 significantly enhances the file system's performance by implementing a negative caching mechanism. This feature aims to minimize redundant network requests to the master server for files that have previously been determined not to exist, thereby reducing latency and improving overall responsiveness, especially in environments with frequent lookups for non-existent paths. 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 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 counter productive. 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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a negative cache for file lookups, which is a great feature for improving performance on lookups for non-existent files. The implementation is well-structured, with a new NegativeCache class, corresponding unit tests, and integration into the existing lookup logic.
I've found one critical issue in the NegativeCache implementation that could lead to undefined behavior if the cache size is set to zero. I've also provided a few medium-severity suggestions to improve a misleading comment about the cache eviction policy and to add a unit test for the aforementioned edge case.
Overall, the changes are good and the feature is valuable. After addressing the critical issue, this PR should be in good shape to merge.
dmga44
left a comment
There was a problem hiding this comment.
Please consider my comments.
rolysr
left a comment
There was a problem hiding this comment.
Very nice addition @ZaRdEr15. Please consider checking my suggestions. When final version of changes is reached, I recommend to you to squash most of changes to more general commits, specially before merging, for a cleaner commit history.
704f633 to
f638476
Compare
rolysr
left a comment
There was a problem hiding this comment.
LGTM. Just a minor suggestion for test style. Very nice to have this feature 👍. Consider when merging, to squash these commits as all these changes should be consider as one atomic change in the history.
It may be better to keep the commits instead of squashing, as they are very well done and atomic themselves. |
f638476 to
af60f49
Compare
22b6e61 to
c115603
Compare
Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
Implements a thread-safe negative cache to store failed lookups. The cache monitors gNegativeCacheTimeoutMs and gNegativeCacheMaxSize every insertion. If the options are changed in Tweaks, the whole cache is cleared. Because the options are not changed frequently, it is okay to clear for consistency. - Uses a transparently hashed unordered_map for performance. - Supports configurable timeout and maximum entry capacity. - When max size is reached removes oldest entry before adding new. - Updates the position in LRU list and timeout if adding an existing entry. - Check if negative cache options max size and timeout are set before addition or lookup Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
Add sfsnegativecachetimeout and sfsnegativecachesize options for sfsmount binary. Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
sauna_client.cc lookup() reduces network latency by replying early with fuse_entry_param with fields .ino=0 and .entry_timeout=sfsnegativecachetimeout. Kernel may cache negative entries, but with internal negative cache implementation it is guaranteed if both options sfsnegativecachesize and sfsnegativecachetimeout are set. - Add negative cache timeout and max size to Tweaks. Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
Signed-off-by: Anton Jastsuk <antonjastsuk66@gmail.com>
c115603 to
6a9fdee
Compare
All lookups must go through the network to master which is time consuming. This is especially a problem if it constantly asks for files that don't exist.
Implement a thread-safe negative cache that reduces network latency by avoiding repeated calls to the master server by replying early with fuse_entry_param with fields .ino=0 and .entry_timeout=sfsnegativecachetimeout.
Kernel may cache negative entries, but with internal negative cache implementation it is guaranteed if both options sfsnegativecachesize and sfsnegativecachetimeout are set.
When any option equals to 0, both internal and Linux kernel-level negative caching are disabled.
Refactor lookup() in sauna_client.cc to improve readability and code style.