[GH-ISSUE #2791] CleanupCacheDir(): Clarify lock semantics #1297

New issue

Closed

opened 2026-03-04 01:52:55 +03:00 by kerem · 4 comments

kerem commented

2026-03-04 01:52:55 +03:00

Owner

Originally created by @CarstenGrohmann on GitHub (Jan 24, 2026).
Original GitHub issue: https://github.com/s3fs-fuse/s3fs-fuse/issues/2791

Current code

github.com/s3fs-fuse/s3fs-fuse@30c90b6459/src/fdcache.cpp (L827-L844)

Observation

The lock/unlock pairing is correct, but the code structure is confusing at first glance because the unlock() is placed outside both branches.

The else-branch acquires the lock (waiting for the other thread) and immediately releases it without performing any cleanup. The comment says "wait for other thread to finish cache cleanup" - so this appears intentional.

Questions

Is the waiting behavior intentional? The else-branch waits for another thread to finish cleanup, then returns without doing anything. Is this the desired behavior, or should the else-branch be removed entirely?
Would a simpler pattern be acceptable? If we don't need to wait for the other thread:

  if(FdManager::cache_cleanup_lock.try_lock()){
      CleanupCacheDirInternal("");
      FdManager::cache_cleanup_lock.unlock();
  }
  // else: Another thread is already doing cleanup - skip

Originally created by @CarstenGrohmann on GitHub (Jan 24, 2026). Original GitHub issue: https://github.com/s3fs-fuse/s3fs-fuse/issues/2791 ## Current code https://github.com/s3fs-fuse/s3fs-fuse/blob/30c90b6459a7b4a7737ab2626bf72437ace0362f/src/fdcache.cpp#L827-L844 ## Observation The lock/unlock pairing is correct, but the code structure is confusing at first glance because the unlock() is placed outside both branches. The else-branch acquires the lock (waiting for the other thread) and immediately releases it without performing any cleanup. The comment says "wait for other thread to finish cache cleanup" - so this appears intentional. ## Questions 1. Is the waiting behavior intentional? The else-branch waits for another thread to finish cleanup, then returns without doing anything. Is this the desired behavior, or should the else-branch be removed entirely? 2. Would a simpler pattern be acceptable? If we don't need to wait for the other thread: ```cpp if(FdManager::cache_cleanup_lock.try_lock()){ CleanupCacheDirInternal(""); FdManager::cache_cleanup_lock.unlock(); } // else: Another thread is already doing cleanup - skip ```

kerem closed this issue

2026-03-04 01:52:55 +03:00

kerem commented

2026-03-04 01:52:56 +03:00

Author

Owner

@ggtakec commented on GitHub (Jan 24, 2026):

@CarstenGrohmann
It makes sense that this else statement waits for the other thread to finish (after acquiring the lock) and then immediately exits.

The cache_cleanup_lock prevents the CleanupCacheDirInternal() method from being invoked multiple times.
If the lock cannot be acquired immediately, it means that CleanupCacheDirInternal() is being executed in another thread.
Since cleanup was executed immediately before (by another thread) after acquiring the lock, there should be no need to execute CleanupCacheDirInternal().
Thus, the reason for waiting on the lock is to delegate cleanup to the other thread and simply wait for it to complete.

@ggtakec commented on GitHub (Jan 24, 2026): @CarstenGrohmann It makes sense that this else statement waits for the other thread to finish (after acquiring the lock) and then immediately exits. The `cache_cleanup_lock` prevents the `CleanupCacheDirInternal()` method from being invoked multiple times. If the lock cannot be acquired immediately, it means that `CleanupCacheDirInternal()` is being executed in another thread. Since cleanup was executed immediately before (by another thread) after acquiring the lock, there should be no need to execute `CleanupCacheDirInternal()`. Thus, the reason for waiting on the lock is to delegate cleanup to the other thread and simply wait for it to complete.

kerem commented

2026-03-04 01:52:56 +03:00

Author

Owner

@CarstenGrohmann commented on GitHub (Jan 24, 2026):

Yes, that makes sense. What do you think about updating the comment at line 840 slightly:

github.com/s3fs-fuse/s3fs-fuse@30c90b6459/src/fdcache.cpp (L839-L843)

// Another thread is already executing CleanupCacheDirInternal().
// Wait for it to complete by blocking on the lock - no need to run
// cleanup again since it was just performed by the other thread.

@CarstenGrohmann commented on GitHub (Jan 24, 2026): Yes, that makes sense. What do you think about updating the comment at line 840 slightly: https://github.com/s3fs-fuse/s3fs-fuse/blob/30c90b6459a7b4a7737ab2626bf72437ace0362f/src/fdcache.cpp#L839-L843 ``` // Another thread is already executing CleanupCacheDirInternal(). // Wait for it to complete by blocking on the lock - no need to run // cleanup again since it was just performed by the other thread. ```

kerem commented

2026-03-04 01:52:56 +03:00

Author

Owner

@ggtakec commented on GitHub (Jan 25, 2026):

@CarstenGrohmann Thank you. I've posted #2793 because it would be better to be detailed comments like your suggestion.

@ggtakec commented on GitHub (Jan 25, 2026): @CarstenGrohmann Thank you. I've posted #2793 because it would be better to be detailed comments like your suggestion.

kerem commented

2026-03-04 01:52:56 +03:00

Author

Owner

@CarstenGrohmann commented on GitHub (Feb 7, 2026):

Closed with #2793

@CarstenGrohmann commented on GitHub (Feb 7, 2026): Closed with #2793

kerem referenced this issue

2026-03-04 02:02:46 +03:00

[PR #1305] [MERGED] Ignore case when comparing ETags #1912