Feature/tiered storage lru policy - #960
Conversation
…eature/tiered_storage-lru_policy
foodprocessor
left a comment
foodprocessor
left a comment
There was a problem hiding this comment.
Very good work! Good design, solid choices.
The function pointers are a very clever way to maintain modularity. Nice touch!
| // eviction | ||
|
|
||
| //check du , do stat file before, based on difference between DU and | ||
|
|
||
| //1. check if we need eviction | ||
| curSize, err := common.GetUsage(q.cachePath) | ||
| if err != nil { | ||
| log.Err("lruPolicy::capacityChecker : failed to get usage: %v", err) | ||
| continue | ||
| } | ||
| if curSize/q.maxCacheSize <= q.threshold { | ||
| break | ||
| } | ||
|
|
||
| //targetRatio should always be less than thresholdRatio | ||
|
|
||
| //find difference to evict down to 60% | ||
| difference := curSize - q.maxCacheSize*q.targetRatio | ||
| curEvictedSpace := 0 | ||
| for curEvictedSpace < int(difference) { | ||
| nodeSize, evicted := q.eviction() | ||
| if !evicted { | ||
| break | ||
| } | ||
| curEvictedSpace += int(nodeSize) | ||
| } |
There was a problem hiding this comment.
This looks good!
Later on, when we're looking at concurrency, let's make sure this code can't run more than once at a time (if the ticker is faster than eviction, we don't begin another eviction loop in parallel).
| q.extractNode(nodeToEvict) | ||
| q.nodeMap.Delete(nodeToEvict.name) |
There was a problem hiding this comment.
Is there a rule we want to follow, for what node membership in the map and the linked list mean? In other words, should we update our records and then execute the action (upload & delete), or visa versa? Which is better for error handling?
| //create node | ||
| newNode := &lruNode{name: name} | ||
| val, found := q.nodeMap.LoadOrStore(name, newNode) | ||
| node := val.(*lruNode) | ||
|
|
||
| if found { | ||
| // touch | ||
| q.extractNode(node) | ||
| } else { | ||
| // brand new node — update tail if list was empty | ||
| if q.tail == nil { | ||
| q.tail = node | ||
| } | ||
| } | ||
| q.setHead(node) |
There was a problem hiding this comment.
This creates a new node before searching for an existing one. It still does what we expect in the end, but it smells off.
| //create node | ||
| newNode := &lruNode{name: name} | ||
| val, found := q.nodeMap.LoadOrStore(name, newNode) | ||
| node := val.(*lruNode) |
There was a problem hiding this comment.
Also, dereferencing / type asserting val before we know if val was found makes me nervous. I figure it's probably fine and just returns nil in practice, but from my experience with C, I see echoes of "nil pointer dereference" here.
jfantinhardesty
left a comment
jfantinhardesty
left a comment
There was a problem hiding this comment.
Great work! This is a strong start to the LRU policy. Just have a few comments.
| } | ||
| nodeSize := fileInfo.Size() | ||
|
|
||
| //remove node from queue and map |
There was a problem hiding this comment.
In the eviction code here, you delete from the LRU, but the file will still remain in the local cache? Do we want the file to stay in the cache or should we delete it when we evict?
| } | ||
|
|
||
| // 6. Eviction, file with open handle, file with no open handle, | ||
| func (suite *lruPolicyTestSuite) TestCapacityCheckerEvictionOpenHandle() { |
There was a problem hiding this comment.
This test is a bit flaky. The sleep could probably be a bit longer. Here is how you can replicate it.
go test -run TestLRUPolicyTestSuite ./component/tiered_storage --tags=unittest,fuse3 -count=50
You should see by running the unit tests 50 times that this test and a few others may fail.
| } | ||
|
|
||
| //find the first applicable node | ||
| for nodeToEvict != nil && q.FileHasOpenFileHandle(nodeToEvict.name) { |
There was a problem hiding this comment.
I think there could be a race condition here. After this check, it is possible that a new request comes in that opens the file before it gets uploaded.
| localPath := filepath.Join(q.cachePath, fileName) | ||
| fileInfo, err := os.Stat(localPath) | ||
| if err != nil { | ||
| log.Err("lruPolicy::capacityChecker : failed to stat file: %v", err) |
There was a problem hiding this comment.
If there is an error here, then the next line would have a nil panic (since the fileInfo would be null we couldn't get the size. This could maybe happen if the file was deleted before the worker uploaded it.
| threshold: 0.8, | ||
| targetRatio: 0.6, | ||
| numWorkers: 8, | ||
| tickerUnit: time.Millisecond, |
There was a problem hiding this comment.
This ticker time is too small. This would check capacity every millisecond. This should be relatively large.
| options.Handle.Cleanup() | ||
| // update LRU add to queue because cleaning up the file should be handled once the file is uploaded in LRU policy logic | ||
| if c.policy != nil { | ||
| c.policy.Enqueue(options.Handle.Path) |
There was a problem hiding this comment.
There is a possible deadlock here. In release file, you hold flock, and then in Enqueue you take the mu lock. However, in the eviction code in lru_policy, eviction first locks mu and then calls flock. This would cause deadlock. The fix is to ensure when taking multiple locks, everyone takes them in the same order.
| } else { | ||
| //local only then just close the file, update LRU add to queue, we will get to this later | ||
| options.Handle.Cleanup() | ||
| // update LRU add to queue because cleaning up the file should be handled once the file is uploaded in LRU policy logic |
There was a problem hiding this comment.
Ok here is a potential deadlock, James' comment got moved somewhere, but basically the order in which we take locks is inconsistent between locking the file and accessing the queue, for release/enqueue we hold flock then take mu lock in enqueue, but for eviction we do the opposite we lock mu then do fileLock, soooo we can either change ReleaseFile and DeleteFile, to unlock before enqueue and dequeue is called, or just change the eviction code
| err := q.uploadandCleanFn(fileName) | ||
| flock.Unlock() | ||
| //handle when file doesn't exist during upload we do not requeue otherwise we do enqueue | ||
| if err != nil { |
There was a problem hiding this comment.
Ok this existence error check might not be entirely useful because we lock the file prior to upload and unlock after, so deletion during upload shouldn't happen, because deleteFile waits on a file lock as well. Just a heads up in case you want to delete this code!
3 participants
What type of Pull Request is this? (check all applicable)
Describe your changes in brief
LRU initial implementation
Checklist
Related Issues