Skip to content

Conversation

cpegeric
Copy link
Contributor

@cpegeric cpegeric commented Jul 15, 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:

single mutex with ghost


PR Type

Bug fix, Enhancement


Description

  • Fix S3-FIFO cache race conditions with single mutex

  • Add ghost queue for improved cache hit rates

  • Implement proper reference counting for cache data

  • Add configuration option to disable S3-FIFO


Changes diagram

flowchart LR
  A["Cache Operations"] --> B["Single Mutex Protection"]
  B --> C["Reference Counting"]
  C --> D["Ghost Queue"]
  D --> E["S3-FIFO Algorithm"]
  F["Configuration"] --> G["DisableS3Fifo Option"]
  G --> E
Loading

Changes walkthrough 📝

Relevant files
Configuration changes
7 files
config.go
Add disable S3-FIFO parameter to meta cache init                 
+1/-1     
config.go
Add disable S3-FIFO parameter to meta cache init                 
+1/-1     
cache.go
Add DisableS3Fifo configuration option                                     
+1/-0     
disk_cache.go
Add disable S3-FIFO parameter to disk cache                           
+2/-0     
local_fs.go
Add disable S3-FIFO parameter to cache initialization       
+4/-1     
s3_fs.go
Add disable S3-FIFO parameter to S3 cache initialization 
+2/-0     
cache.go
Add disable S3-FIFO configuration and update meta cache   
+7/-5     
Bug fix
7 files
bytes.go
Replace atomic operations with mutex for thread safety     
+40/-19 
data_cache.go
Implement proper reference counting and path deletion       
+16/-22 
fifo.go
Rewrite S3-FIFO with single mutex and ghost queue               
+299/-197
queue.go
Add thread safety and size tracking to queue                         
+31/-4   
data.go
Change Release method to return boolean                                   
+1/-1     
io_vector.go
Handle cache data release return value                                     
+7/-2     
mem_cache.go
Remove redundant retain/release calls in post functions   
+3/-10   
Tests
11 files
bytes_test.go
Add panic tests for double free scenarios                               
+21/-0   
cache_test.go
Update test calls with new disable parameter                         
+1/-1     
disk_cache_test.go
Update test calls with new disable parameter                         
+14/-10 
bench2_test.go
Add comprehensive cache performance benchmarks                     
+133/-0 
bench_test.go
Update benchmark tests with disable parameter                       
+6/-6     
data_cache_test.go
Update tests and fix release method signature                       
+6/-2     
fifo_test.go
Add comprehensive S3-FIFO algorithm tests                               
+124/-11
shardmap_test.go
Add tests for sharded hash map operations                               
+45/-0   
local_fs_test.go
Update test calls with new disable parameter                         
+2/-0     
mem_cache_test.go
Update tests and add double free check                                     
+9/-2     
memory_fs_test.go
Update benchmark tests with disable parameter                       
+2/-0     
Enhancement
2 files
ghost.go
Implement thread-safe ghost queue for S3-FIFO                       
+90/-0   
shardmap.go
Implement thread-safe sharded hash map                                     
+136/-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.
  • Copy link

    You are nearing your monthly Qodo Merge usage quota. For more information, please visit here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent reference counter underflow

    The reference counter can become negative if Release() is called more times than
    Retain(). Add a check to prevent refs from going below zero to avoid potential
    memory corruption or unexpected behavior.

    pkg/fileservice/bytes.go [71-88]

     func (b *Bytes) Release() bool {
     	b.mu.Lock()
     	defer b.mu.Unlock()
     
     	if b.bytes == nil {
     		panic("fileservice.Bytes.Release() double free")
    +	}
    +
    +	if b.refs <= 0 {
    +		panic("fileservice.Bytes.Release() reference counter underflow")
     	}
     
     	b.refs -= 1
     	if b.refs == 0 {
     		if b.deallocator != nil {
     			b.deallocator.Deallocate(malloc.NoHints)
     			b.bytes = nil
     			return true
     		}
     	}
     	return false
     }
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that the refs counter could become negative, and adds a check to panic in this case, which improves robustness and aids debugging.

    Medium
    • More

    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 4/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