From 28fcc0023b9ec0d0ae5ce0f62d3a5d07f73543da Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 20 Sep 2024 15:44:28 -0600 Subject: [PATCH 1/2] Use LoadAndDelete whenever possible to avoid race conditions when using a sync map. --- component/file_cache/file_cache.go | 5 ++--- component/libfuse/libfuse2_handler.go | 3 +-- internal/handlemap/handle_map.go | 9 +++++++++ 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/component/file_cache/file_cache.go b/component/file_cache/file_cache.go index 87d5cc8a0..32ee8bbd7 100644 --- a/component/file_cache/file_cache.go +++ b/component/file_cache/file_cache.go @@ -1211,11 +1211,10 @@ func (fc *FileCache) FlushFile(options internal.FlushFileOptions) error { // If chmod was done on the file before it was uploaded to container then setting up mode would have been missed // Such file names are added to this map and here post upload we try to set the mode correctly - _, found := fc.missedChmodList.Load(options.Handle.Path) + // Delete the entry from map so that any further flush do not try to update the mode again + _, found := fc.missedChmodList.LoadAndDelete(options.Handle.Path) if found { // If file is found in map it means last chmod was missed on this - // Delete the entry from map so that any further flush do not try to update the mode again - fc.missedChmodList.Delete(options.Handle.Path) // When chmod on container was missed, local file was updated with correct mode // Here take the mode from local cache and update the container accordingly diff --git a/component/libfuse/libfuse2_handler.go b/component/libfuse/libfuse2_handler.go index 403fac3cf..99b6634e4 100644 --- a/component/libfuse/libfuse2_handler.go +++ b/component/libfuse/libfuse2_handler.go @@ -426,7 +426,7 @@ func (cf *CgofuseFS) Opendir(path string) (int, uint64) { // Releasedir opens the handle for the directory at the path. func (cf *CgofuseFS) Releasedir(path string, fh uint64) int { // Get the filehandle - handle, exists := handlemap.Load(handlemap.HandleID(fh)) + handle, exists := handlemap.LoadAndDelete(handlemap.HandleID(fh)) if !exists { log.Trace("Libfuse::Releasedir : Failed to release %s, handle: %d", path, fh) return -fuse.EBADF @@ -435,7 +435,6 @@ func (cf *CgofuseFS) Releasedir(path string, fh uint64) int { log.Trace("Libfuse::Releasedir : %s, handle: %d", handle.Path, handle.ID) handle.Cleanup() - handlemap.Delete(handle.ID) return 0 } diff --git a/internal/handlemap/handle_map.go b/internal/handlemap/handle_map.go index 1ea37b0a5..ba82ba3c0 100644 --- a/internal/handlemap/handle_map.go +++ b/internal/handlemap/handle_map.go @@ -166,6 +166,15 @@ func Delete(key HandleID) { defaultHandleMap.Delete(key) } +// Delete : Remove handle object from map, and return the entry (if any) +func LoadAndDelete(key HandleID) (*Handle, bool) { + val, found := defaultHandleMap.LoadAndDelete(key) + if !found { + return nil, false + } + return val.(*Handle), true +} + func CreateCacheObject(capacity int64, handle *Handle) { handle.CacheObj = &Cache{ sync.RWMutex{}, From e13e48e074da56434de3bbbc170a851af2599211 Mon Sep 17 00:00:00 2001 From: Michael Habinsky Date: Fri, 20 Sep 2024 15:44:44 -0600 Subject: [PATCH 2/2] Fix file cache deletion race condition --- component/file_cache/lru_policy.go | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/component/file_cache/lru_policy.go b/component/file_cache/lru_policy.go index 2b902b684..4a9e34bf3 100644 --- a/component/file_cache/lru_policy.go +++ b/component/file_cache/lru_policy.go @@ -215,7 +215,13 @@ func (p *lruPolicy) asyncCacheValid() { for { select { case name := <-p.validateChan: - p.cacheValidate(name) + // validateChan only gets names that are already cached + // if the file is not in the map anymore, then it was deleted, + // which means calling cacheValidate now would be a bug + _, found := p.nodeMap.Load(name) + if found { + p.cacheValidate(name) + } case <-p.closeSignalValidate: return