Skip to content

bug fix: s3fifo with ghost #22149

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 39 commits into
base: main
Choose a base branch
from

Conversation

cpegeric
Copy link
Contributor

@cpegeric cpegeric commented Jul 10, 2025

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #21838

What this PR does / why we need it:

  • The fix includes adding deleted status to avoid double postEvict/remove from hashtable.
  • Gemini AI also review and didn't detect any race condition
  • run test and benchmark with -race flag.
  • set the dellocated buffer to nil and panic when try to access deallocated buffer
  • defer Bytes.Release() at the end of postEvict() function
  • remove retain/release from post functions. Do retain/release inside the cache.

PR Type

Bug fix, Enhancement


Description

  • Fix S3-FIFO cache race conditions and memory management

  • Add proper reference counting for cache data

  • Implement thread-safe operations with mutex protection

  • Add ghost queue for S3-FIFO algorithm optimization


Changes diagram

flowchart LR
  A["Cache Operations"] --> B["Thread Safety"]
  B --> C["Reference Counting"]
  C --> D["Memory Management"]
  A --> E["S3-FIFO Algorithm"]
  E --> F["Ghost Queue"]
  F --> G["Eviction Policy"]
Loading

Changes walkthrough 📝

Relevant files
Configuration changes
4 files
config.go
Add disable S3-FIFO parameter to config                                   
+1/-1     
config.go
Add disable S3-FIFO parameter to config                                   
+1/-1     
cache.go
Add disable S3-FIFO configuration option                                 
+1/-0     
cache.go
Add disable S3-FIFO configuration support                               
+7/-5     
Bug fix
6 files
bytes.go
Add mutex protection and panic checks                                       
+44/-8   
data_cache.go
Refactor with thread-safe operations                                         
+14/-22 
fifo.go
Complete S3-FIFO implementation with race fixes                   
+263/-192
queue.go
Add mutex protection to queue operations                                 
+23/-4   
io_vector.go
Update Release method with null checks                                     
+7/-2     
mem_cache.go
Remove manual retain/release calls                                             
+3/-10   
Tests
11 files
bytes_test.go
Add panic test cases for bytes                                                     
+19/-0   
cache_test.go
Update test with disable S3-FIFO parameter                             
+1/-1     
disk_cache_test.go
Update tests with disable S3-FIFO parameter                           
+14/-10 
bench2_test.go
Add comprehensive cache benchmark tests                                   
+133/-0 
bench_test.go
Update benchmarks with disable S3-FIFO parameter                 
+6/-6     
data_cache_test.go
Update tests and fix release method                                           
+6/-2     
fifo_test.go
Add comprehensive S3-FIFO algorithm tests                               
+124/-11
shardmap_test.go
Add shardmap functionality tests                                                 
+50/-0   
local_fs_test.go
Update tests with disable S3-FIFO parameter                           
+2/-0     
mem_cache_test.go
Update tests and add double free check                                     
+9/-2     
memory_fs_test.go
Update tests with disable S3-FIFO parameter                           
+2/-0     
Enhancement
6 files
disk_cache.go
Add disable S3-FIFO parameter support                                       
+2/-0     
ghost.go
Implement ghost queue for S3-FIFO                                               
+90/-0   
shardmap.go
Implement thread-safe sharded hash map                                     
+140/-0 
data.go
Change Release method to return boolean                                   
+1/-1     
local_fs.go
Add disable S3-FIFO parameter support                                       
+4/-1     
s3_fs.go
Add disable S3-FIFO parameter support                                       
+2/-0     

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @cpegeric cpegeric requested a review from LeftHandCold as a code owner July 10, 2025 11:23
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/bug Something isn't working Review effort 5/5 size/XL Denotes a PR that changes [1000, 1999] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants