[PR #379] [MERGED] fix: Fix AvailSlots(AtomicSharedPtrTable) dealloc() slot cleanup logic #401

New issue

Closed

opened 2026-10-26 09:49:55 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:49:55 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/379
Author: @qieqieplus
Created: 1/14/2026
Status: ✅ Merged
Merged: 1/14/2026
Merged by: @SF-Zhou

Base: main ← Head: main

📝 Commits (1)

847edc6 fix(AtomicSharedPtrTable): Fix AvailSlots dealloc() slot cleanup logic

📊 Changes

1 file changed (+12 additions, -14 deletions)

View changed files

📝 src/common/utils/AtomicSharedPtrTable.h (+12 -14)

📄 Description

Bug: AvailSlots::dealloc fails to erase reclaimed indices from free set
The original code

if (idx == nextAvail - 1) {
  while (free.find(--nextAvail) != free.end()) {
    // move back next avail as much as possible
    ;
  }
  ...

Issue 1: Loop condition checks the wrong index
The loop checks free.find(--nextAvail). But after the decrement, nextAvail == idx (the index being deallocated right now), which should never be in free. The condition is always false on first iteration, so the loop never executes.

Issue 2: Never erases from free
Even if the loop did execute, it only calls find() without erase(). Indices would remain in free while also being implicitly available (below nextAvail), leading to double allocation.

These two errors together created a state where the code technically avoided double allocations but failed its primary purpose. Because the loop exited immediately, it only decremented nextAvail by one. The other contiguous free slots (e.g., 0 and 1) remained in the free set.

Minimal demo to reproduce the issue:

#include "common/utils/AtomicSharedPtrTable.h"
#include <iostream>


int main() {
    hf3fs::AvailSlots slots(10);
    
    slots.alloc();
    slots.alloc();
    slots.alloc();
    
    slots.dealloc(0);
    slots.dealloc(1);
    // Expected: nextAvail=3, free={0,1}
    
    slots.dealloc(2);
    // Expected: nextAvail=0, free={}
    // Actual BUG: nextAvail=2, free={0,1}
    
    std::cout << "After dealloc:\n";
    std::cout << "  nextAvail = " << slots.nextAvail 
              << " (expected 0)\n";
    std::cout << "  free.size = " << slots.free.size() 
              << " (expected 0)\n";
    return 0;
}

Other Minor Fixes:

Strict Range Check: dealloc idx can never go beyond nextAvail
Redundant Atomicity Removal: already protected by mutex

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/379 **Author:** [@qieqieplus](https://github.com/qieqieplus) **Created:** 1/14/2026 **Status:** ✅ Merged **Merged:** 1/14/2026 **Merged by:** [@SF-Zhou](https://github.com/SF-Zhou) **Base:** `main` ← **Head:** `main` --- ### 📝 Commits (1) - [`847edc6`](https://github.com/deepseek-ai/3FS/commit/847edc6839423c530cc6f0ed3a402249af1b4bca) fix(AtomicSharedPtrTable): Fix AvailSlots dealloc() slot cleanup logic ### 📊 Changes **1 file changed** (+12 additions, -14 deletions) <details> <summary>View changed files</summary> 📝 `src/common/utils/AtomicSharedPtrTable.h` (+12 -14) </details> ### 📄 Description Bug: AvailSlots::dealloc fails to erase reclaimed indices from free set The original code ``` if (idx == nextAvail - 1) { while (free.find(--nextAvail) != free.end()) { // move back next avail as much as possible ; } ... ``` Issue 1: Loop condition checks the wrong index The loop checks free.find(--nextAvail). But after the decrement, nextAvail == idx (the index being deallocated right now), which should never be in free. The condition is always false on first iteration, so the loop never executes. Issue 2: Never erases from free Even if the loop did execute, it only calls find() without erase(). Indices would remain in free while also being implicitly available (below nextAvail), leading to double allocation. These two errors together created a state where the code technically avoided double allocations but failed its primary purpose. Because the loop exited immediately, it only decremented nextAvail by one. The other contiguous free slots (e.g., 0 and 1) remained in the free set. Minimal demo to reproduce the issue: ``` #include "common/utils/AtomicSharedPtrTable.h" #include <iostream> int main() { hf3fs::AvailSlots slots(10); slots.alloc(); slots.alloc(); slots.alloc(); slots.dealloc(0); slots.dealloc(1); // Expected: nextAvail=3, free={0,1} slots.dealloc(2); // Expected: nextAvail=0, free={} // Actual BUG: nextAvail=2, free={0,1} std::cout << "After dealloc:\n"; std::cout << " nextAvail = " << slots.nextAvail << " (expected 0)\n"; std::cout << " free.size = " << slots.free.size() << " (expected 0)\n"; return 0; } ``` Other Minor Fixes: - Strict Range Check: dealloc idx can never go beyond nextAvail - Redundant Atomicity Removal: already protected by mutex --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 09:49:55 +00:00