Skip to content

perf(chunkserver): Protect IO performance during massive delete LS #79 #739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
GigaCronos wants to merge 2 commits into
base: dev
Choose a base branch
from
+232 −39
Draft

perf(chunkserver): Protect IO performance during massive delete LS #79 #739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fc45523
perf(chunkserver): Protect IO performance during massive delete
GigaCronos Feb 2, 2026
06a78b6
fix: Make a threadpool for trash-remover threads.
GigaCronos Feb 28, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions doc/sfschunkserver.cfg.5.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ defined at build time), /usr/local/lib/saunafs/plugins/chunkserver, and

*CHUNK_TRASH_ENABLED (EXPERIMENTAL)*:: enables or disables the chunk trash
feature. When enabled, deleted chunks are moved to a trash directory instead of
being immediately removed. (Default: 0)
being immediately removed. (Default: 1)

*CHUNK_TRASH_EXPIRATION_SECONDS (EXPERIMENTAL)*:: specifies the timeout in
seconds for chunks to remain in the trash before being permanently deleted.
Expand All @@ -198,9 +198,11 @@ threshold, the system will start deleting older chunks from the trash to free
up space. A suggested value is about the 15% of the capacity of the smaller
configured disk. (Default: 0)

*CHUNK_TRASH_GC_BATCH_SIZE (EXPERIMENTAL)*:: defines the bulk size for the
garbage collector when processing chunks in the trash. This determines how many
files are processed in each garbage collection cycle. (Default: 1000)
*CHUNK_TRASH_GC_BATCH_SIZE (EXPERIMENTAL)*:: defines the maximum bulk size
per disk for the garbage collector when processing chunks in the trash.
This value is used when no I/O pressure is detected and is dynamically
scaled down based on current disk load.
(Default: 500)

*CHUNK_TRASH_GC_SPACE_RECOVERY_BATCH_SIZE (EXPERIMENTAL)*:: [ADVANCED] The
number of files to remove from the trash in a single GC cycle, in case the disk
Expand Down
4 changes: 2 additions & 2 deletions src/admin/dump_config_command.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,10 @@ const static std::unordered_map<std::string, std::string> defaultOptionsCS = {
{"REPLICATION_CONNECTION_TIMEOUT_MS", "1000"},
{"REPLICATION_WAVE_TIMEOUT_MS", "500"},
{"PLUGINS_DIR", ""},
{"CHUNK_TRASH_ENABLED", "0"},
{"CHUNK_TRASH_ENABLED", "1"},
{"CHUNK_TRASH_EXPIRATION_SECONDS", "259200"},
Copy link

@gemini-code-assist gemini-code-assist bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The default value for CHUNK_TRASH_EXPIRATION_SECONDS appears to be outdated. It is set to "259200" here, but in src/chunkserver/chunkserver-common/chunk_trash_manager_impl.h, the corresponding default kDefaultTrashTimeLimitSeconds has been changed to 0. To ensure consistency across the system, this default value should be updated to match.

    {"CHUNK_TRASH_EXPIRATION_SECONDS", "0"},

{"CHUNK_TRASH_FREE_SPACE_THRESHOLD_GB", "0"},
{"CHUNK_TRASH_GC_BATCH_SIZE", "1000"},
{"CHUNK_TRASH_GC_BATCH_SIZE", "500"},
{"CHUNK_TRASH_GC_SPACE_RECOVERY_BATCH_SIZE", "100"},
};

Expand Down
4 changes: 1 addition & 3 deletions src/chunkserver/chunkserver-common/chunk_trash_index.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,10 +80,8 @@ ChunkTrashIndex::TrashIndexDiskEntries ChunkTrashIndex::getExpiredFilesInternal(
const time_t &timeLimit, size_t bulkSize) {
TrashIndexDiskEntries expiredFiles;

size_t count = 0;
for (const auto &diskEntry : trashIndex) {
if (bulkSize != 0 && count >= bulkSize) { break; }
count += getExpiredFilesInternal(diskEntry.first, timeLimit, expiredFiles, bulkSize);
getExpiredFilesInternal(diskEntry.first, timeLimit, expiredFiles, bulkSize);
}

return expiredFiles;
Expand Down
54 changes: 40 additions & 14 deletions src/chunkserver/chunkserver-common/chunk_trash_manager.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,11 @@ int ChunkTrashManager::moveToTrash(const std::filesystem::path &filePath,
if (!isEnabled) { return 0; }

// Protect against concurrent access
std::lock_guard<std::mutex> lock(implMutex);

auto &impl = getImpl();
ImplementationPtr impl;
{
std::lock_guard<std::mutex> lock(implMutex);
impl = getImpl();
}
if (!impl) {
safs::log_error_code(SAUNAFS_ERROR_EINVAL,
"ChunkTrashManager implementation not initialized");
Expand All @@ -69,9 +71,11 @@ int ChunkTrashManager::init(const std::string &diskPath) {
reloadConfig();

// Protect against concurrent access
std::lock_guard<std::mutex> lock(implMutex);

auto &impl = getImpl();
ImplementationPtr impl;
{
std::lock_guard<std::mutex> lock(implMutex);
impl = getImpl();
}
if (!impl) {
safs::log_error_code(SAUNAFS_ERROR_EINVAL,
"ChunkTrashManager implementation not initialized");
Expand All @@ -80,13 +84,33 @@ int ChunkTrashManager::init(const std::string &diskPath) {
return impl->init(diskPath);
}

void ChunkTrashManager::terminate(){
if (!isEnabled) { return; }

// Protect against concurrent access
ImplementationPtr impl;
{
std::lock_guard<std::mutex> lock(implMutex);
impl = getImpl();
}
if (!impl) {
safs::log_error_code(SAUNAFS_ERROR_EINVAL,
"ChunkTrashManager implementation not initialized");
return;
}
impl->terminate();

}

void ChunkTrashManager::collectGarbage() {
if (!isEnabled) { return; }

// Protect against concurrent access
std::lock_guard<std::mutex> lock(implMutex);

auto &impl = getImpl();
ImplementationPtr impl;
{
std::lock_guard<std::mutex> lock(implMutex);
impl = getImpl();
}
if (!impl) {
safs::log_error_code(SAUNAFS_ERROR_EINVAL,
"ChunkTrashManager implementation not initialized");
Expand All @@ -96,17 +120,19 @@ void ChunkTrashManager::collectGarbage() {
}

void ChunkTrashManager::reloadConfig() {
// Protect against concurrent access
std::lock_guard<std::mutex> lock(implMutex);

auto &impl = getImpl();
// Protect against concurrent access
ImplementationPtr impl;
{
std::lock_guard<std::mutex> lock(implMutex);
impl = getImpl();
}
if (!impl) {
safs::log_error_code(SAUNAFS_ERROR_EINVAL,
"ChunkTrashManager implementation not initialized");
return;
}

isEnabled = cfg_get("CHUNK_TRASH_ENABLED", static_cast<u_short>(0));
isEnabled = cfg_get("CHUNK_TRASH_ENABLED", static_cast<u_short>(1));
safs::log_info("Chunk trash manager is {}", isEnabled ? "enabled" : "disabled");
impl->reloadConfig();
}
7 changes: 7 additions & 0 deletions src/chunkserver/chunkserver-common/chunk_trash_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@ class IChunkTrashManagerImpl {

virtual int init(const std::string &) = 0;

virtual void terminate() = 0;

virtual void collectGarbage() = 0;

virtual void reloadConfig() = 0;
Expand Down Expand Up @@ -73,6 +75,11 @@ class ChunkTrashManager {
*/
static int init(const std::string &diskPath);

/**
* @brief Join the background threads.
*/
static void terminate();

/**
* @brief Moves a file to the trash directory.
*
Expand Down
123 changes: 121 additions & 2 deletions src/chunkserver/chunkserver-common/chunk_trash_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,17 @@

#include <sys/statvfs.h>
#include <algorithm>
#include <atomic>
#include <cstdint>
#include <ctime>
#include <utility>

#include "chunkserver-common/chunk_trash_manager_impl.h"
#include "config/cfg.h"
#include "errors/saunafs_error_codes.h"
#include "global_shared_resources.h"
#include "hdd_stats.h"
#include "hdd_utils.h"
#include "slogger/slogger.h"

namespace fs = std::filesystem;
Expand All @@ -36,9 +41,21 @@ size_t ChunkTrashManagerImpl::trashGarbageCollectorBulkSize = kDefaultTrashGarba
size_t ChunkTrashManagerImpl::garbageCollectorSpaceRecoveryStep =
kDefaultGarbageCollectorSpaceRecoveryStep;

uint64_t ChunkTrashManagerImpl::maxBytesReadPerDisk = 1024 * 1024; //1MiB
uint64_t ChunkTrashManagerImpl::maxBytesWritePerDisk = 1024 * 1024; //1MiB

uint64_t ChunkTrashManagerImpl::previousBytesReadPerDisk = 1024 * 1024; //1MiB
uint64_t ChunkTrashManagerImpl::previousBytesWritePerDisk = 1024 * 1024; //1MiB

const std::string ChunkTrashManagerImpl::kTrashGuardString =
std::string("/") + ChunkTrashManager::kTrashDirname + "/";

std::vector<std::thread> ChunkTrashManagerImpl::removeFromTrashThreads{};
std::unique_ptr<ProducerConsumerQueue> ChunkTrashManagerImpl::removeFromTrashJobQueue =
std::make_unique<ProducerConsumerQueue>();

std::atomic<uint32_t> ChunkTrashManagerImpl::NotIdleThreadCount{0};

void ChunkTrashManagerImpl::reloadConfig() {
availableThresholdGB =
cfg_get("CHUNK_TRASH_FREE_SPACE_THRESHOLD_GB", kDefaultAvailableThresholdGB);
Expand Down Expand Up @@ -160,11 +177,49 @@ int ChunkTrashManagerImpl::moveToTrash(const fs::path &filePath, const fs::path
void ChunkTrashManagerImpl::removeTrashFiles(
const ChunkTrashIndex::TrashIndexDiskEntries &filesToRemove) const {
for (const auto &[diskPath, fileEntries] : filesToRemove) {
for (const auto &fileEntry : fileEntries) {
removeFromTrashJobQueue->put(
0, 1,
reinterpret_cast<uint8_t *>(
new std::pair<ChunkTrashIndex::TrashIndexFileEntries, std::string>(fileEntries,
diskPath)),
Comment on lines +182 to +184
Copy link

@gemini-code-assist gemini-code-assist bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Using new with reinterpret_cast is unsafe and not idiomatic C++. It's prone to memory leaks if not handled carefully in all code paths in the consumer thread. Also, passing fileEntries by value to the std::pair constructor causes the entire std::multimap to be copied for each job, which can be inefficient.

Consider using smart pointers like std::unique_ptr to manage the lifetime of the job data automatically. This would make the code safer and more aligned with modern C++ practices.

For example, you could define a struct for the job data:

struct TrashRemovalJob {
    ChunkTrashIndex::TrashIndexFileEntries files;
    std::string diskPath;
};

Then, you can use std::make_unique<TrashRemovalJob>(...) and pass the raw pointer to the queue, and reconstruct the unique_ptr in the worker thread to ensure automatic cleanup.

References
  1. Prefer passing individual parameters to low-level job functions instead of a pointer to a high-level operation object. Encapsulating job data in a dedicated struct and managing it with smart pointers aligns with this principle by providing a clear, safe, and efficient way to pass job-specific parameters.

1);
}

while(NotIdleThreadCount && !removeFromTrashJobQueue->isEmpty()){

}
Comment on lines +188 to +190
Copy link

@gemini-code-assist gemini-code-assist bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

The removeTrashFiles function contains a busy-wait loop that consumes 100% of a CPU core, posing a performance and Denial of Service (DoS) risk. The condition NotIdleThreadCount && !removeFromTrashJobQueue->isEmpty() is logically flawed; it can cause the loop to exit prematurely, leading to inefficient space recovery and potential resource exhaustion as callers might incorrectly assume jobs are complete. It's recommended to replace this busy-wait with a proper synchronization mechanism, such as a std::condition_variable, to efficiently wait for job completion.

Suggested change
while(NotIdleThreadCount && !removeFromTrashJobQueue->isEmpty()){
}
while(!removeFromTrashJobQueue->isEmpty() || NotIdleThreadCount > 0) {
std::this_thread::yield();
}

}

void ChunkTrashManagerImpl::removeTrashFilesFromDiskThread(uint8_t workerId) {
std::string threadName ="removeTrashFilesFromDisk_worker_" + std::to_string(workerId);
pthread_setname_np(pthread_self(), threadName.c_str());

uint32_t jobId;
uint32_t operation;
uint8_t *jobPtrArg;

while (true) {
removeFromTrashJobQueue->get(&jobId, &operation, &jobPtrArg, nullptr);

if(operation == 0){
break;
}

NotIdleThreadCount ++;
auto tempTuple = reinterpret_cast<std::pair<ChunkTrashIndex::TrashIndexFileEntries, std::string> *>(jobPtrArg);

ChunkTrashIndex::TrashIndexFileEntries filesToRemove = tempTuple->first;
std::string diskPath = tempTuple->second;

for (const auto &fileEntry : filesToRemove) {
if (removeFileFromTrash(fileEntry.second) != SAUNAFS_STATUS_OK) { continue; }
HddStats::gStatsOperationsGCPurge++;
getTrashIndex().remove(fileEntry.first, fileEntry.second, diskPath);
}

delete tempTuple;

NotIdleThreadCount --;
}
}

Expand Down Expand Up @@ -210,9 +265,30 @@ int ChunkTrashManagerImpl::init(const std::string &diskPath) {
}
}

if (ChunkTrashManagerImpl::removeFromTrashThreads.size() < 5) {
Copy link

@gemini-code-assist gemini-code-assist bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The number of worker threads for removing trash files is hardcoded to 5. This should be a named constant to improve readability and maintainability. Consider making it configurable or basing it on std::thread::hardware_concurrency().

    static constexpr uint8_t kRemoveFromTrashThreadCount = 5;
    if (ChunkTrashManagerImpl::removeFromTrashThreads.size() < kRemoveFromTrashThreadCount) {

ChunkTrashManagerImpl::removeFromTrashThreads.emplace_back(
&ChunkTrashManagerImpl::removeTrashFilesFromDiskThread,
uint8_t(ChunkTrashManagerImpl::removeFromTrashThreads.size()));
}

return SAUNAFS_STATUS_OK;
}

void ChunkTrashManagerImpl::terminate() {
for(uint8_t i=0; i < ChunkTrashManagerImpl::removeFromTrashThreads.size(); i++){
ChunkTrashManagerImpl::removeFromTrashJobQueue->put(0, 0, nullptr, 1);
}


for (auto &thread : ChunkTrashManagerImpl::removeFromTrashThreads) {
if (thread.joinable()) { thread.join(); }
}

ChunkTrashManagerImpl::removeFromTrashThreads.clear();

}


bool ChunkTrashManagerImpl::isValidTimestampFormat(const std::string &timestamp) {
return timestamp.size() == kTimeStampLength && std::ranges::all_of(timestamp, ::isdigit);
}
Expand Down Expand Up @@ -256,8 +332,51 @@ void ChunkTrashManagerImpl::collectGarbage() {
if (!ChunkTrashManager::isEnabled) { return; }
std::time_t const currentTime = std::time(nullptr);
std::time_t const expirationTime = currentTime - trashTimeLimitSeconds;
removeExpiredFiles(expirationTime, trashGarbageCollectorBulkSize);

uint64_t currentBytesWrite = HddStats::gBytesWrittenSinceLastGCSweep.exchange(0);
uint64_t currentBytesRead = HddStats::gBytesReadSinceLastGCSweep.exchange(0);
uint64_t currentDiskCount = 1;
{
std::lock_guard disksLockGuard(gDisksMutex);
currentDiskCount = std::max(currentDiskCount, gDisks.size());
}
currentBytesRead /= currentDiskCount;
currentBytesWrite /= currentDiskCount;

// 0.99997 ^ (30 cycles/min * 60 minutes an hour * 72 hours a day) = 0.02 (2%)
maxBytesReadPerDisk =
std::max({uint64_t(maxBytesReadPerDisk * 0.99997), currentBytesRead, uint64_t(1'000'000)});
maxBytesWritePerDisk = std::max(
{uint64_t(maxBytesWritePerDisk * 0.99997), currentBytesWrite, uint64_t(1'000'000)});

double totalIOPercentage =
static_cast<double>(currentBytesRead) * 100.0 / maxBytesReadPerDisk +
static_cast<double>(currentBytesWrite) * 100.0 / maxBytesWritePerDisk;

auto invertedSigmoid = [](double val) -> double {
const double steepness = 10.0; // steepness
const double center = 0.15; // center in [0,1]
Comment on lines +357 to +358
Copy link

@gemini-code-assist gemini-code-assist bot Feb 28, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The parameters for the invertedSigmoid function (steepness = 10.0, center = 0.15) do not match the values mentioned in the pull request description (steepness=12, center=0.3). Please clarify if this discrepancy is intentional.

Additionally, these magic numbers should be defined as named constants for better readability and maintainability.

        const double kSigmoidSteepness = 12.0; // As per PR description
        const double kSigmoidCenter = 0.3;     // As per PR description

const double valnorm = val / 100.0; // normalize so that 100% total I/O maps to 1.0

const double res = 1.0 / (1.0 + std::exp(-steepness * (valnorm - center)));
return 1.0 - res;
};

uint64_t bulksizeScaled = trashGarbageCollectorBulkSize * invertedSigmoid(totalIOPercentage);
if ((currentBytesRead + 1) / (previousBytesReadPerDisk + 1) +
(currentBytesWrite + 1) / (previousBytesWritePerDisk + 1) >=
10) {
bulksizeScaled = 0;
}

static constexpr uint64_t kMinGCBulkSizeForActivation = 5;

if (bulksizeScaled >= kMinGCBulkSizeForActivation) {
removeExpiredFiles(expirationTime, bulksizeScaled);
}
makeSpace(availableThresholdGB, garbageCollectorSpaceRecoveryStep);
previousBytesReadPerDisk = currentBytesRead;
previousBytesWritePerDisk = currentBytesWrite;
}

bool ChunkTrashManagerImpl::isTrashPath(const std::string &filePath) {
Expand Down
Loading
Loading