From c39e1b0b09d84cf9e1cbd12018e1488e3880f68c Mon Sep 17 00:00:00 2001 From: chuandew Date: Tue, 15 Sep 2026 16:05:11 +0800 Subject: [PATCH 1/7] [refactor][utils] Use Folly for thread execution; compose the executor with the native timer. --- CMakeLists.txt | 3 + README.md | 6 + src/client/cmake/dingofsConfig.cmake.in | 2 + src/common/CMakeLists.txt | 1 + src/utils/executor/CMakeLists.txt | 1 + src/utils/executor/thread/executor_impl.cc | 31 +- src/utils/executor/thread/executor_impl.h | 42 +-- src/utils/executor/thread/thread_pool_impl.cc | 83 ++--- src/utils/executor/thread/thread_pool_impl.h | 39 +-- src/utils/executor/timer/timer_impl.cc | 28 +- src/utils/executor/timer/timer_impl.h | 28 +- test/unit/utils/executor/test_timer_impl.cc | 57 +--- test/unit/utils/test_executor_impl.cc | 316 +++++++++++++++--- 13 files changed, 410 insertions(+), 227 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index cb442c242..1b4c0f752 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -296,6 +296,9 @@ message("Using RocksDB ${RocksDB_VERSION}") find_package(rados REQUIRED) message("Using rados: ${RADOS_LIBRARIES}") +find_package(folly CONFIG REQUIRED) +message("Using folly: ${FOLLY_LIBRARIES}, include_dir:${FOLLY_INCLUDE_DIRS}") + find_package(dingosdk REQUIRED) message(STATUS "Found dingosdk: ${dingosdk_VERSION}") diff --git a/README.md b/README.md index 39f08d3e0..ce1021c80 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,12 @@ We recommend using GCC 13 as the primary compiled language. - [Dingo-eureka](https://github.com/dingodb/dingo-eureka): A Necessary Service Components for DingoFS. - [Dingo-sdk](https://github.com/dingodb/dingo-sdk): A Unified Software Development Kit (SDK) required for DingoFS. +Dingo-eureka must be built with Folly enabled (`-DWITH_FOLLY=ON`). Rebuild older +installations that do not provide the Folly CMake package, and set +`THIRD_PARTY_INSTALL_PATH` to the updated Dingo-eureka installation when configuring DingoFS. +Folly must also be configured with `FOLLY_NO_EXCEPTION_TRACER=ON` in Dingo-eureka +to support DingoFS's static libstdc++ linkage; C++ exceptions remain enabled. + ### 2. Install jemalloc ```shell wget https://github.com/jemalloc/jemalloc/releases/download/5.3.0/jemalloc-5.3.0.tar.bz2 diff --git a/src/client/cmake/dingofsConfig.cmake.in b/src/client/cmake/dingofsConfig.cmake.in index cd30c5ecd..0a1573eea 100644 --- a/src/client/cmake/dingofsConfig.cmake.in +++ b/src/client/cmake/dingofsConfig.cmake.in @@ -63,6 +63,7 @@ find_package(AWSSDK REQUIRED CONFIG COMPONENTS s3 s3-crt) # → aws- find_package(spdlog REQUIRED CONFIG) # → spdlog::spdlog find_package(jsoncpp REQUIRED CONFIG) # → JsonCpp::JsonCpp find_package(Threads REQUIRED) # → Threads::Threads (CMake built-in) +find_package(folly REQUIRED CONFIG) # → Folly::folly # uuid: cmake config at non-standard location; resolve via find_library instead if(NOT TARGET uuid::uuid_static) @@ -101,6 +102,7 @@ aws-cpp-sdk-s3-crt;\ aws-cpp-sdk-s3;\ spdlog::spdlog;\ JsonCpp::JsonCpp;\ +Folly::folly;\ uring::uring;\ uuid::uuid_static;\ -Wl,--end-group;\ diff --git a/src/common/CMakeLists.txt b/src/common/CMakeLists.txt index 46c7ae37d..9cb8cfa35 100644 --- a/src/common/CMakeLists.txt +++ b/src/common/CMakeLists.txt @@ -47,6 +47,7 @@ target_link_libraries(dingofs_common brpc::brpc protobuf::libprotobuf fmt::fmt + Folly::folly gflags::gflags glog::glog dingofs_open_trace diff --git a/src/utils/executor/CMakeLists.txt b/src/utils/executor/CMakeLists.txt index 3d8e2bad8..ed41e5fa9 100644 --- a/src/utils/executor/CMakeLists.txt +++ b/src/utils/executor/CMakeLists.txt @@ -22,6 +22,7 @@ add_library(dingofs_executor target_link_libraries(dingofs_executor brpc::brpc + Folly::folly glog::glog gflags::gflags ) diff --git a/src/utils/executor/thread/executor_impl.cc b/src/utils/executor/thread/executor_impl.cc index a22c485b1..d63738b27 100644 --- a/src/utils/executor/thread/executor_impl.cc +++ b/src/utils/executor/thread/executor_impl.cc @@ -16,8 +16,6 @@ #include -#include - #include "utils/executor/thread/thread_pool_impl.h" #include "utils/executor/timer/timer_impl.h" @@ -26,27 +24,46 @@ namespace dingofs { DEFINE_int32(executor_impl_bg_thread_num, 8, "background thread number for executor"); +ExecutorImpl::ExecutorImpl(const std::string& name) + : ExecutorImpl(name, FLAGS_executor_impl_bg_thread_num) {} + +ExecutorImpl::ExecutorImpl(const std::string& name, int thread_num) + : name_(name), thread_num_(thread_num) {} + +ExecutorImpl::~ExecutorImpl() { Stop(); } + bool ExecutorImpl::Start() { + if (running_.load(std::memory_order_relaxed)) { + return false; + } + CHECK_GT(thread_num_, 0); + pool_ = std::make_unique(name_, thread_num_); pool_->Start(); timer_ = std::make_unique(pool_.get()); - CHECK(timer_->Start()); + timer_->Start(); running_.store(true, std::memory_order_release); return true; } bool ExecutorImpl::Stop() { + // Close admission before waiting for existing submitters, so continuous + // producers cannot starve the writer side of the admission lock. if (!running_.exchange(false, std::memory_order_acq_rel)) { return false; } - CHECK(timer_->Stop()); + // Timer::Stop releases undispatched delayed-task captures; ThreadPool::Stop + // joins workers after all timer dispatches have completed. + timer_->Stop(); + timer_.reset(); pool_->Stop(); + pool_.reset(); return true; } bool ExecutorImpl::Execute(std::function func) { - CHECK(running_); + CHECK(running_.load(std::memory_order_relaxed)); pool_->Execute(std::move(func)); return true; } @@ -58,4 +75,6 @@ bool ExecutorImpl::Schedule(std::function func, int delay_ms) { return timer_->Add(std::move(func), delay_ms); } -} // namespace dingofs \ No newline at end of file +int ExecutorImpl::TaskNum() const { return pool_ ? pool_->GetTaskNum() : 0; } + +} // namespace dingofs diff --git a/src/utils/executor/thread/executor_impl.h b/src/utils/executor/thread/executor_impl.h index 38e165cb3..4fd7a4be2 100644 --- a/src/utils/executor/thread/executor_impl.h +++ b/src/utils/executor/thread/executor_impl.h @@ -30,44 +30,48 @@ namespace dingofs { DECLARE_int32(executor_impl_bg_thread_num); +// Composes a folly-backed ThreadPoolImpl for immediate tasks with a +// folly-backed TimerImpl for delayed tasks. The owner serializes Start, Stop +// and destruction. Stop must not run on this executor's worker or timer +// thread. Tasks must not throw; violations terminate. class ExecutorImpl final : public Executor { public: - ExecutorImpl(const std::string& name) - : ExecutorImpl(name, FLAGS_executor_impl_bg_thread_num) {} - - ExecutorImpl(const std::string& name, int thread_num) - : name_(name), - thread_num_(thread_num), - timer_(nullptr), - pool_(nullptr), - running_(false) {} - - ~ExecutorImpl() override { Stop(); } + explicit ExecutorImpl(const std::string& name); + ExecutorImpl(const std::string& name, int thread_num); + ~ExecutorImpl() override; + // thread_num must be positive. Returns false if already started. bool Start() override; + // Closes admission, discards undispatched delayed tasks via Timer::Stop, + // then drains accepted ready work via ThreadPool::Stop. Cancelled captures + // are destroyed before returning. A completed Stop permits a subsequent + // Start. bool Stop() override; + // Must be called while started. Concurrent submissions are supported. bool Execute(std::function func) override; + // May race Stop. Rejected tasks are not retained. Accepted tasks run on CPU + // workers, no earlier than the deadline measured at entry, or are cancelled + // by Stop. A nonpositive delay means ready asynchronously. bool Schedule(std::function func, int delay_ms) override; - int ThreadNum() const override { return pool_->GetBackgroundThreads(); } - - int TaskNum() const override { return pool_->GetTaskNum(); } - + int ThreadNum() const override { return thread_num_; } + int TaskNum() const override; std::string Name() const override { return InternalName(); } - static std::string InternalName() { return "ExecutorImpl"; } private: const std::string name_; const int thread_num_; - std::unique_ptr timer_; + + std::atomic running_{false}; + std::unique_ptr pool_; - std::atomic_bool running_; + std::unique_ptr timer_; }; } // namespace dingofs -#endif // DINGOFS_UITLS_THREAD_EXECUTOR_IMPL_H_ \ No newline at end of file +#endif // DINGOFS_UITLS_THREAD_EXECUTOR_IMPL_H_ diff --git a/src/utils/executor/thread/thread_pool_impl.cc b/src/utils/executor/thread/thread_pool_impl.cc index 71df0d9fa..8dedd5f97 100644 --- a/src/utils/executor/thread/thread_pool_impl.cc +++ b/src/utils/executor/thread/thread_pool_impl.cc @@ -14,94 +14,49 @@ #include "utils/executor/thread/thread_pool_impl.h" -#include +#include -#include "glog/logging.h" +#include namespace dingofs { -void ThreadPoolImpl::ThreadProc(size_t thread_id) { - VLOG(12) << "Thread " << thread_id << " started."; +ThreadPoolImpl::ThreadPoolImpl(const std::string& name, int num_threads) + : name_(name), thread_num_(num_threads) {} - pthread_setname_np(pthread_self(), name_.substr(0, 15).c_str()); - - while (true) { - std::function task; - - { - std::unique_lock lock(mutex_); - condition_.wait(lock, [this] { return !tasks_.empty() || !running_; }); - - if (!running_ && tasks_.empty()) { - break; - } - - if (!tasks_.empty()) { - task = std::move(tasks_.front()); - tasks_.pop(); - } - } // end lock scope - - CHECK(task); - (task)(); - } // end of while loop - - VLOG(12) << "Thread " << thread_id << " exit."; -} +ThreadPoolImpl::~ThreadPoolImpl() { Stop(); } void ThreadPoolImpl::Start() { - std::unique_lock lg(mutex_); - if (running_) { + if (pool_) { return; } - running_ = true; - - threads_.resize(thread_num_); - for (size_t i = 0; i < thread_num_; i++) { - threads_[i] = std::thread([this, i] { ThreadProc(i); }); - } + pool_ = std::make_unique( + std::make_pair(thread_num_, thread_num_), + folly::CPUThreadPoolExecutor::makeLifoSemQueue(), + std::make_shared(name_)); } void ThreadPoolImpl::Stop() { - { - std::unique_lock lock(mutex_); - if (!running_) { - return; - } - - running_ = false; - condition_.notify_all(); + if (!pool_) { + return; } - for (auto& thread : threads_) { - if (thread.joinable()) { - thread.join(); - } - } + pool_->join(); + pool_.reset(); } -int ThreadPoolImpl::GetBackgroundThreads() { - std::lock_guard lock(mutex_); - return thread_num_; -} +int ThreadPoolImpl::GetBackgroundThreads() { return thread_num_; } int ThreadPoolImpl::GetTaskNum() const { - std::lock_guard lock(mutex_); - return tasks_.size(); + return pool_ ? static_cast(pool_->getTaskQueueSize()) : 0; } void ThreadPoolImpl::Execute(const std::function& task) { - auto cp(task); - std::lock_guard lock(mutex_); - tasks_.push(std::move(cp)); - condition_.notify_one(); + pool_->add([task]() { task(); }); } void ThreadPoolImpl::Execute(std::function&& task) { - std::lock_guard lock(mutex_); - tasks_.push(std::move(task)); - condition_.notify_one(); + pool_->add([task = std::move(task)]() mutable noexcept { task(); }); } -} // namespace dingofs \ No newline at end of file +} // namespace dingofs diff --git a/src/utils/executor/thread/thread_pool_impl.h b/src/utils/executor/thread/thread_pool_impl.h index e63a718d9..16cb196f9 100644 --- a/src/utils/executor/thread/thread_pool_impl.h +++ b/src/utils/executor/thread/thread_pool_impl.h @@ -15,53 +15,38 @@ #ifndef DINGOFS_UITLS_THREAD_POOL_IMPL_H_ #define DINGOFS_UITLS_THREAD_POOL_IMPL_H_ -#include +#include -#include -#include -#include +#include #include -#include #include "utils/executor/thread_pool.h" namespace dingofs { -class ThreadPoolImpl : public ThreadPool { +// Fixed-size thread pool backed by folly CPUThreadPoolExecutor. Execute +// enqueues immediately; Stop drains all accepted work via join(). +class ThreadPoolImpl final : public ThreadPool { public: - ThreadPoolImpl(const std::string& name, int num_threads) - : name_(name), thread_num_(num_threads) {} + ThreadPoolImpl(const std::string& name, int num_threads); - ~ThreadPoolImpl() override { Stop(); } + ~ThreadPoolImpl() override; void Start() override; - void Stop() override; int GetBackgroundThreads() override; - - // Get the number of task scheduled in the ThreadPoolImpl int GetTaskNum() const override; - // Submit a fire and forget jobs - // This allows to submit the same job multiple times - void Execute(const std::function&) override; - - // This moves the function in for efficiency - void Execute(std::function&&) override; + void Execute(const std::function& task) override; + void Execute(std::function&& task) override; private: - void ThreadProc(size_t thread_id); - - mutable std::mutex mutex_; const std::string name_; - int thread_num_{0}; - bool running_{false}; - std::condition_variable condition_; - std::vector threads_; - std::queue> tasks_; + const int thread_num_; + std::unique_ptr pool_; }; } // namespace dingofs -#endif // DINGOFS_UITLS_THREAD_POOL_IMPL_H_ \ No newline at end of file +#endif // DINGOFS_UITLS_THREAD_POOL_IMPL_H_ diff --git a/src/utils/executor/timer/timer_impl.cc b/src/utils/executor/timer/timer_impl.cc index 087bce227..65efbb4c3 100644 --- a/src/utils/executor/timer/timer_impl.cc +++ b/src/utils/executor/timer/timer_impl.cc @@ -15,15 +15,11 @@ #include "utils/executor/timer/timer_impl.h" -#include +#include -#include -#include - -#include "glog/logging.h" - -DEFINE_int32(timer_bg_thread_default_num, 8, - "background thread number for timer"); +#include +#include +#include namespace dingofs { @@ -86,7 +82,14 @@ bool TimerImpl::Add(std::function func, int delay_ms) { } heap_.push(std::move(fn_info)); - cv_.notify_all(); + + // Run only needs to reconsider its wait when the earliest deadline changes. + // This avoids waking the timer thread (and contending its cache lines) for + // tasks that do not affect the current minimum. + const bool wake = heap_.size() == 1 || next < heap_.top().next_run_time_us; + if (wake) { + cv_.notify_one(); + } return true; } @@ -104,8 +107,13 @@ void TimerImpl::Run() { .count(); if (cur_fn.next_run_time_us <= now) { std::function fn = cur_fn.fn; - thread_pool_->Execute(std::move(fn)); heap_.pop(); + // Submit to the thread pool without holding mutex_: pool admission may + // contend on its own queue, and holding the timer lock here would block + // concurrent producers calling Add(). + lk.unlock(); + thread_pool_->Execute(std::move(fn)); + lk.lock(); } else { cv_.wait_for(lk, microseconds(cur_fn.next_run_time_us - now)); } diff --git a/src/utils/executor/timer/timer_impl.h b/src/utils/executor/timer/timer_impl.h index 1842f7335..3c4234cfb 100644 --- a/src/utils/executor/timer/timer_impl.h +++ b/src/utils/executor/timer/timer_impl.h @@ -12,22 +12,26 @@ // See the License for the specific language governing permissions and // limitations under the License. -#ifndef DINGOFS_SRC_BASE_TIMER_TIMER_IMPL_H_ -#define DINGOFS_SRC_BASE_TIMER_TIMER_IMPL_H_ +#ifndef DINGOFS_UTILS_TIMER_TIMER_IMPL_H_ +#define DINGOFS_UTILS_TIMER_TIMER_IMPL_H_ #include +#include +#include +#include #include #include #include -#include "gflags/gflags_declare.h" #include "utils/executor/thread_pool.h" #include "utils/executor/timer/timer.h" namespace dingofs { -class TimerImplTestPeer; -class TimerImpl : public Timer { +// Simple priority-queue timer on a dedicated thread. Producers touch only the +// mutex and heap; the timer thread sleeps when idle, so it does not contend +// for producer cache lines. Only notifies when the earliest deadline changes. +class TimerImpl final : public Timer { public: // caller owns the thread pool TimerImpl(ThreadPool* thread_pool); @@ -35,18 +39,15 @@ class TimerImpl : public Timer { ~TimerImpl() override; bool Start() override; - bool Stop() override; + // Returns true only when the timer accepts ownership of func. The timer does + // not retain a rejected function. bool Add(std::function func, int delay_ms) override; bool IsStopped() override; private: - friend class TimerImplTestPeer; - - void Run(); - struct FunctionInfo { std::function fn; uint64_t next_run_time_us; @@ -62,16 +63,17 @@ class TimerImpl : public Timer { } }; + void Run(); + + ThreadPool* thread_pool_; std::mutex mutex_; std::condition_variable cv_; std::unique_ptr thread_{nullptr}; std::priority_queue, RunTimeOrder> heap_; bool running_{false}; - - ThreadPool* thread_pool_; }; } // namespace dingofs -#endif // DINGOFS_SRC_BASE_TIMER_TIMER_IMPL_H_ +#endif // DINGOFS_UTILS_TIMER_TIMER_IMPL_H_ diff --git a/test/unit/utils/executor/test_timer_impl.cc b/test/unit/utils/executor/test_timer_impl.cc index 565423468..6a211d818 100644 --- a/test/unit/utils/executor/test_timer_impl.cc +++ b/test/unit/utils/executor/test_timer_impl.cc @@ -15,11 +15,11 @@ #include #include -#include -#include +#include // NOLINT +#include // NOLINT #include -#include -#include +#include // NOLINT +#include // NOLINT #include "glog/logging.h" #include "gtest/gtest.h" @@ -27,25 +27,6 @@ #include "utils/executor/timer/timer_impl.h" namespace dingofs { -class TimerImplTestPeer { - public: - static bool CanAcquireMutex(TimerImpl* timer) { - std::atomic acquired{false}; - std::thread checker([&] { - for (int i = 0; i < 100; ++i) { - if (timer->mutex_.try_lock()) { - acquired.store(true, std::memory_order_release); - timer->mutex_.unlock(); - return; - } - std::this_thread::yield(); - } - }); - checker.join(); - return acquired.load(std::memory_order_acquire); - } -}; - namespace utils { namespace unit_test { @@ -56,27 +37,10 @@ class TimerImplTest : public ::testing::Test { pool->Start(); } - ~TimerImplTest() override = default; + ~TimerImplTest() override { pool->Stop(); } std::unique_ptr pool{nullptr}; }; -struct TimerCaptureProbe { - TimerImpl* timer; - std::atomic* destroyed; - std::atomic* destroyed_outside_mutex; - - TimerCaptureProbe(TimerImpl* timer, std::atomic* destroyed, - std::atomic* destroyed_outside_mutex) - : timer(timer), - destroyed(destroyed), - destroyed_outside_mutex(destroyed_outside_mutex) {} - - ~TimerCaptureProbe() { - destroyed_outside_mutex->store(TimerImplTestPeer::CanAcquireMutex(timer), - std::memory_order_release); - destroyed->store(true, std::memory_order_release); - } -}; TEST_F(TimerImplTest, BaseTest) { auto timer = std::make_unique(pool.get()); @@ -119,6 +83,7 @@ TEST_F(TimerImplTest, Add) { } EXPECT_EQ(count.load(), 0); + timer->Stop(); } TEST_F(TimerImplTest, StopDestroysPendingFunctionsOutsideMutex) { @@ -127,17 +92,15 @@ TEST_F(TimerImplTest, StopDestroysPendingFunctionsOutsideMutex) { std::atomic ran{false}; std::atomic destroyed{false}; - std::atomic destroyed_outside_mutex{false}; - auto probe = std::make_shared(timer.get(), &destroyed, - &destroyed_outside_mutex); - + auto probe = std::make_shared(1); + std::weak_ptr weak_probe = probe; ASSERT_TRUE(timer->Add([probe, &ran] { ran.store(true); }, 60 * 60 * 1000)); probe.reset(); + ASSERT_FALSE(weak_probe.expired()); ASSERT_TRUE(timer->Stop()); EXPECT_FALSE(ran.load()); - EXPECT_TRUE(destroyed.load(std::memory_order_acquire)); - EXPECT_TRUE(destroyed_outside_mutex.load(std::memory_order_acquire)); + EXPECT_TRUE(weak_probe.expired()); } } // namespace unit_test diff --git a/test/unit/utils/test_executor_impl.cc b/test/unit/utils/test_executor_impl.cc index 4e5e46dd7..df45ca5bc 100644 --- a/test/unit/utils/test_executor_impl.cc +++ b/test/unit/utils/test_executor_impl.cc @@ -17,9 +17,12 @@ #include #include // NOLINT #include // NOLINT +#include #include #include // NOLINT #include // NOLINT +#include +#include #include "utils/executor/bthread/bthread_executor.h" #include "utils/executor/thread/executor_impl.h" @@ -120,82 +123,313 @@ TEST(BthreadExecutorTest, ScheduleAfterStopRejectsAndDestroysTask) { EXPECT_TRUE(weak_lifetime.expired()); } +namespace { + +// A timed-out Stop must not hang the suite or retain references to test locals. +// On that failure path the thread retains the executor until Stop finishes. +class AsyncStop { + public: + explicit AsyncStop(std::shared_ptr executor, + std::shared_future begin = {}) { + std::packaged_task task([executor, begin] { + if (begin.valid()) { + begin.wait(); + } + return executor->Stop(); + }); + result_ = task.get_future(); + thread_ = std::thread(std::move(task)); + } + + ~AsyncStop() { + if (thread_.joinable()) { + thread_.detach(); + } + } + + bool IsReady(std::chrono::milliseconds timeout) { + return result_.wait_for(timeout) == std::future_status::ready; + } + + bool Finish() { + if (!IsReady(std::chrono::seconds(5))) { + return false; + } + thread_.join(); + return result_.get(); + } + + private: + std::future result_; + std::thread thread_; +}; + +} // namespace + TEST(ExecutorImplTest, StartStopReportsThreadCount) { ExecutorImpl executor("unit_test_exec", 3); - EXPECT_TRUE(executor.Start()); + ASSERT_TRUE(executor.Start()); + EXPECT_FALSE(executor.Start()); EXPECT_EQ(executor.ThreadNum(), 3); - EXPECT_EQ(executor.TaskNum(), 0); - EXPECT_EQ(executor.Name(), "ExecutorImpl"); EXPECT_TRUE(executor.Stop()); EXPECT_FALSE(executor.Stop()); } -TEST(ExecutorImplTest, ExecuteRunsTask) { - ExecutorImpl executor("unit_test_exec", 2); - ASSERT_TRUE(executor.Start()); - - std::atomic ran(false); - EXPECT_TRUE(executor.Execute([&] { ran.store(true); })); - - for (int i = 0; i < 200 && !ran.load(); ++i) { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); +TEST(ExecutorImplTest, StopDrainsImmediateQueueAfterBlockedWorker) { + auto executor = std::make_shared("unit_test_exec", 1); + ASSERT_TRUE(executor->Start()); + + auto entered = std::make_shared>(); + auto entered_future = entered->get_future(); + std::promise release; + auto released = release.get_future().share(); + auto completed = std::make_shared>(0); + EXPECT_TRUE(executor->Execute([entered, released] { + entered->set_value(); + released.wait(); + })); + // No fatal assertions until the blocker has been released. + EXPECT_EQ(entered_future.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + constexpr int kQueuedTasks = 32; + for (int i = 0; i < kQueuedTasks; ++i) { + EXPECT_TRUE(executor->Execute([completed] { ++*completed; })); } - EXPECT_TRUE(ran.load()); + EXPECT_EQ(executor->TaskNum(), kQueuedTasks); + + AsyncStop stop(executor); + EXPECT_FALSE(stop.IsReady(std::chrono::milliseconds(20))); + EXPECT_EQ(completed->load(), 0); + release.set_value(); + EXPECT_TRUE(stop.Finish()); + EXPECT_EQ(completed->load(), kQueuedTasks); +} - executor.Stop(); +TEST(ExecutorImplTest, DueTasksWaitForCpuWorker) { + auto executor = std::make_shared("unit_test_exec", 1); + ASSERT_TRUE(executor->Start()); + + auto entered = std::make_shared>(); + auto entered_future = entered->get_future(); + std::promise release; + auto released = release.get_future().share(); + EXPECT_TRUE(executor->Execute([entered, released] { + entered->set_value(std::this_thread::get_id()); + released.wait(); + })); + const bool worker_entered = + entered_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready; + EXPECT_TRUE(worker_entered); + const auto worker_id = + worker_entered ? entered_future.get() : std::thread::id{}; + + std::vector> callbacks; + for (int delay_ms : {-1, 0, 10}) { + auto completed = std::make_shared>(); + callbacks.push_back(completed->get_future()); + EXPECT_TRUE(executor->Schedule( + [completed] { completed->set_value(std::this_thread::get_id()); }, + delay_ms)); + } + // Wait for actual dispatch, not an arbitrary sleep. TaskNum excludes the + // blocked running task and includes only ready work, not pending timers. + const auto deadline = + std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (executor->TaskNum() < 3 && + std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + EXPECT_EQ(executor->TaskNum(), 3); + for (auto& callback : callbacks) { + EXPECT_EQ(callback.wait_for(std::chrono::milliseconds(0)), + std::future_status::timeout); + } + release.set_value(); + for (auto& callback : callbacks) { + const bool ready = + callback.wait_for(std::chrono::seconds(5)) == std::future_status::ready; + EXPECT_TRUE(ready); + if (ready && worker_entered) { + EXPECT_EQ(callback.get(), worker_id); + } + } + EXPECT_TRUE(executor->Stop()); } -TEST(ExecutorImplTest, ScheduleRunsTaskAfterDelay) { - ExecutorImpl executor("unit_test_exec", 2); +TEST(ExecutorImplTest, PositiveDelayNeverRunsEarly) { + ExecutorImpl executor("unit_test_exec", 1); ASSERT_TRUE(executor.Start()); - std::atomic ran(false); - EXPECT_TRUE(executor.Schedule([&] { ran.store(true); }, 10)); - - for (int i = 0; i < 200 && !ran.load(); ++i) { - std::this_thread::sleep_for(std::chrono::milliseconds(10)); + using Clock = std::chrono::steady_clock; + auto completed = std::make_shared>(); + auto completion = completed->get_future(); + const auto before_schedule = Clock::now(); + constexpr int kDelayMs = 75; + EXPECT_TRUE(executor.Schedule( + [completed] { completed->set_value(Clock::now()); }, kDelayMs)); + const bool ready = + completion.wait_for(std::chrono::seconds(5)) == std::future_status::ready; + EXPECT_TRUE(ready); + if (ready) { + EXPECT_GE(completion.get() - before_schedule, + std::chrono::milliseconds(kDelayMs)); } - EXPECT_TRUE(ran.load()); - - executor.Stop(); + EXPECT_TRUE(executor.Stop()); } TEST(ExecutorImplTest, StopDestroysPendingScheduledTask) { ExecutorImpl executor("unit_test_exec", 1); ASSERT_TRUE(executor.Start()); - std::atomic ran{false}; - auto lifetime = std::make_shared(1); + auto ran = std::make_shared>(false); + auto destroyed_on = std::make_shared(); + auto lifetime = std::shared_ptr(new int(1), [destroyed_on](int* value) { + *destroyed_on = std::this_thread::get_id(); + delete value; + }); std::weak_ptr weak_lifetime = lifetime; ASSERT_TRUE(executor.Schedule( - [lifetime, &ran] { ran.store(true, std::memory_order_release); }, + [lifetime = std::move(lifetime), ran] { ran->store(true); }, 60 * 60 * 1000)); - lifetime.reset(); - ASSERT_FALSE(weak_lifetime.expired()); - ASSERT_TRUE(executor.Stop()); - EXPECT_FALSE(ran.load(std::memory_order_acquire)); + EXPECT_FALSE(weak_lifetime.expired()); + EXPECT_EQ(executor.TaskNum(), 0); + EXPECT_TRUE(executor.Stop()); + EXPECT_FALSE(ran->load()); EXPECT_TRUE(weak_lifetime.expired()); + // With the three-layer architecture, TimerImpl::Stop releases captures on + // the EventBase timer thread (which is joined before Stop returns), not on + // the Stop caller thread. The contract is that destruction happens before + // Stop returns and the task never runs. + EXPECT_NE(*destroyed_on, std::thread::id{}); +} + +TEST(ExecutorImplTest, ScheduleRacingStopReleasesAcceptedAndRejectedCaptures) { + auto executor = std::make_shared("unit_test_exec", 1); + ASSERT_TRUE(executor->Start()); + + auto ran = std::make_shared>(false); + struct RaceState { + std::vector> lifetimes; + int accepted = 0; + int rejected = 0; + int retained_rejections = 0; + }; + auto state = std::make_shared(); + auto schedule = [executor, state, ran] { + auto lifetime = std::make_shared(1); + std::weak_ptr weak_lifetime = lifetime; + const bool ok = executor->Schedule( + [lifetime = std::move(lifetime), ran] { ran->store(true); }, + 60 * 60 * 1000); + state->lifetimes.push_back(weak_lifetime); + if (ok) { + ++state->accepted; + } else { + ++state->rejected; + if (!weak_lifetime.expired()) { + ++state->retained_rejections; + } + } + }; + + // Guarantee both admission outcomes independently of race scheduling. + schedule(); + std::promise begin; + auto begin_future = begin.get_future().share(); + auto producer_done = std::make_shared>(); + auto producer_future = producer_done->get_future(); + std::thread producer([schedule, begin_future, producer_done] { + begin_future.wait(); + for (int i = 0; i < 512; ++i) { + schedule(); + } + producer_done->set_value(); + }); + AsyncStop stop(executor, begin_future); + begin.set_value(); + const bool producer_finished = + producer_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready; + if (producer_finished) { + producer.join(); + } else { + producer.detach(); + } + ASSERT_TRUE(producer_finished) << "Schedule deadlocked while racing Stop"; + const bool stopped = stop.Finish(); + EXPECT_TRUE(stopped); + if (stopped) { + schedule(); + } + EXPECT_GT(state->accepted, 0); + EXPECT_GT(state->rejected, 0); + EXPECT_EQ(state->retained_rejections, 0); + EXPECT_FALSE(ran->load()); + for (const auto& lifetime : state->lifetimes) { + EXPECT_TRUE(lifetime.expired()); + } } -TEST(ExecutorImplTest, ScheduleAfterStopRejectsAndDestroysTask) { +TEST(ExecutorImplTest, RestartDoesNotRunCancelledCallbacks) { ExecutorImpl executor("unit_test_exec", 1); + auto old_ran = std::make_shared>(false); + auto old_lifetime = std::make_shared(1); + std::weak_ptr weak_old_lifetime = old_lifetime; ASSERT_TRUE(executor.Start()); + ASSERT_TRUE(executor.Schedule([old_lifetime = std::move(old_lifetime), + old_ran] { old_ran->store(true); }, + 60 * 60 * 1000)); ASSERT_TRUE(executor.Stop()); + EXPECT_TRUE(weak_old_lifetime.expired()); - std::atomic ran{false}; - auto lifetime = std::make_shared(1); - std::weak_ptr weak_lifetime = lifetime; - std::function task = [lifetime, &ran] { - ran.store(true, std::memory_order_release); - }; - lifetime.reset(); + ASSERT_TRUE(executor.Start()); + auto completed = std::make_shared>(); + auto completion = completed->get_future(); + EXPECT_TRUE(executor.Schedule([completed] { completed->set_value(); }, 10)); + EXPECT_EQ(completion.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_FALSE(old_ran->load()); + EXPECT_TRUE(executor.Stop()); +} - EXPECT_FALSE(executor.Schedule(std::move(task), 1)); - EXPECT_FALSE(ran.load(std::memory_order_acquire)); +TEST(ExecutorImplTest, CancelledCaptureDestructorCanReenterExecutor) { + auto executor = std::make_shared("unit_test_exec", 1); + ASSERT_TRUE(executor->Start()); + + struct Result { + std::atomic destroyed{false}; + std::atomic rejected{false}; + std::atomic rejected_capture_released{false}; + std::atomic queued{-1}; + }; + auto result = std::make_shared(); + std::weak_ptr weak_executor = executor; + auto lifetime = + std::shared_ptr(new int(1), [weak_executor, result](int* value) { + delete value; + auto executor = weak_executor.lock(); + auto nested = std::make_shared(1); + std::weak_ptr weak_nested = nested; + result->rejected = !executor->Schedule([nested = std::move(nested)] {}, + 60 * 60 * 1000); + result->rejected_capture_released = weak_nested.expired(); + result->queued = executor->TaskNum(); + result->destroyed = true; + }); + std::weak_ptr weak_lifetime = lifetime; + EXPECT_TRUE( + executor->Schedule([lifetime = std::move(lifetime)] {}, 60 * 60 * 1000)); + AsyncStop stop(executor); + ASSERT_TRUE(stop.Finish()) << "Stop deadlocked during capture destruction"; EXPECT_TRUE(weak_lifetime.expired()); + EXPECT_TRUE(result->destroyed.load()); + EXPECT_TRUE(result->rejected.load()); + EXPECT_TRUE(result->rejected_capture_released.load()); + EXPECT_EQ(result->queued.load(), 0); } } // namespace unit_test From 59da0ce5134294243fff3f3fecdbed55f777a559 Mon Sep 17 00:00:00 2001 From: chuandew Date: Tue, 15 Sep 2026 16:05:11 +0800 Subject: [PATCH 2/7] [perf][client] Avoid FsInfo copies on hot paths; narrow the lifecycle state type. --- src/client/vfs/client_session.h | 2 +- src/client/vfs/data/reader/chunk_read_op.cc | 5 ++--- src/client/vfs/data/reader/file_reader.cc | 4 ++-- src/client/vfs/data/writer/chunk_writer.cc | 4 ++-- src/client/vfs/data/writer/file_writer.cc | 4 +--- src/client/vfs/hub/vfs_hub.h | 21 +++++++++++++++++++ test/unit/client/vfs/data/test_file_reader.cc | 11 ++++++++++ .../unit/client/vfs/data/test_writer_table.cc | 3 ++- test/unit/client/vfs/mock/mock_vfs_hub.h | 3 +++ test/unit/client/vfs/test_base.h | 9 ++++++++ 10 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/client/vfs/client_session.h b/src/client/vfs/client_session.h index a6f4498f0..c4eefd57b 100644 --- a/src/client/vfs/client_session.h +++ b/src/client/vfs/client_session.h @@ -181,7 +181,7 @@ class ClientSession { private: friend class ClientSessionLifecycleTest; - enum class LifecycleState { + enum class LifecycleState : uint8_t { kCreated, kStarting, kRunning, diff --git a/src/client/vfs/data/reader/chunk_read_op.cc b/src/client/vfs/data/reader/chunk_read_op.cc index f7f39c6fa..d93b73753 100644 --- a/src/client/vfs/data/reader/chunk_read_op.cc +++ b/src/client/vfs/data/reader/chunk_read_op.cc @@ -39,9 +39,8 @@ namespace { bvar::Adder vfs_block_rreq_inflighting("vfs_block_rreq_inflighting"); Chunk MakeChunk(VFSHub* hub, const ChunkReq& req) { - const FsInfo fs_info = hub->GetFsInfo(); - return Chunk(fs_info.id, req.ino, req.index, fs_info.chunk_size, - fs_info.block_size); + return Chunk(hub->GetFsId(), req.ino, req.index, hub->GetChunkSize(), + hub->GetBlockSize()); } } // namespace diff --git a/src/client/vfs/data/reader/file_reader.cc b/src/client/vfs/data/reader/file_reader.cc index 33a911644..dba338c03 100644 --- a/src/client/vfs/data/reader/file_reader.cc +++ b/src/client/vfs/data/reader/file_reader.cc @@ -88,8 +88,8 @@ FileReader::FileReader(VFSHub* hub, uint64_t fh, uint64_t ino) fh_(fh), ino_(ino), uuid_(fmt::format("file_reader-{}-{}", ino, fh)), - chunk_size_(hub->GetFsInfo().chunk_size), - block_size_(hub->GetFsInfo().block_size), + chunk_size_(hub->GetChunkSize()), + block_size_(hub->GetBlockSize()), policy_(new ReadaheadPoclicy(fh)) {} // when file reader destructor called, diff --git a/src/client/vfs/data/writer/chunk_writer.cc b/src/client/vfs/data/writer/chunk_writer.cc index 33707e2cc..9c97610d6 100644 --- a/src/client/vfs/data/writer/chunk_writer.cc +++ b/src/client/vfs/data/writer/chunk_writer.cc @@ -44,8 +44,8 @@ namespace vfs { // protected by mutex_ ChunkWriter::ChunkWriter(VFSHub* hub, uint64_t ino, uint64_t index) : hub_(hub), - chunk_(hub->GetFsInfo().id, ino, index, hub->GetFsInfo().chunk_size, - hub->GetFsInfo().block_size), + chunk_(hub->GetFsId(), ino, index, hub->GetChunkSize(), + hub->GetBlockSize()), page_size_(hub->GetWriteMemPool()->GetPageSize()) {} ChunkWriter::~ChunkWriter() { diff --git a/src/client/vfs/data/writer/file_writer.cc b/src/client/vfs/data/writer/file_writer.cc index 7fa8b340a..c5f354445 100644 --- a/src/client/vfs/data/writer/file_writer.cc +++ b/src/client/vfs/data/writer/file_writer.cc @@ -213,9 +213,7 @@ Status FileWriter::Write(ContextSPtr ctx, const char* buf, uint64_t size, return s; } -int32_t FileWriter::GetChunkSize() const { - return vfs_hub_->GetFsInfo().chunk_size; -} +int32_t FileWriter::GetChunkSize() const { return vfs_hub_->GetChunkSize(); } ChunkWriter* FileWriter::GetOrCreateChunkWriter(int64_t chunk_index) { std::lock_guard lock(mutex_); diff --git a/src/client/vfs/hub/vfs_hub.h b/src/client/vfs/hub/vfs_hub.h index 308c8c963..abcf478ca 100644 --- a/src/client/vfs/hub/vfs_hub.h +++ b/src/client/vfs/hub/vfs_hub.h @@ -102,6 +102,12 @@ class VFSHub { virtual FsInfo GetFsInfo() = 0; + // Hot-path accessors that avoid copying the full FsInfo (which contains + // multiple std::string members). Safe to call concurrently after Start(). + virtual int32_t GetChunkSize() = 0; + virtual int32_t GetBlockSize() = 0; + virtual uint32_t GetFsId() = 0; + virtual blockaccess::BlockAccessOptions GetBlockAccesserOptions() = 0; virtual UidGidMapper* GetUidGidMapper() = 0; @@ -212,6 +218,21 @@ class VFSHubImpl : public VFSHub { return fs_info_; } + int32_t GetChunkSize() override { + CHECK(started_.load(std::memory_order_relaxed)) << "not started"; + return fs_info_.chunk_size; + } + + int32_t GetBlockSize() override { + CHECK(started_.load(std::memory_order_relaxed)) << "not started"; + return fs_info_.block_size; + } + + uint32_t GetFsId() override { + CHECK(started_.load(std::memory_order_relaxed)) << "not started"; + return fs_info_.id; + } + TraceManager* GetTraceManager() override { return &trace_manager_; } blockaccess::BlockAccessOptions GetBlockAccesserOptions() override { diff --git a/test/unit/client/vfs/data/test_file_reader.cc b/test/unit/client/vfs/data/test_file_reader.cc index 92c15b4a3..60df80330 100644 --- a/test/unit/client/vfs/data/test_file_reader.cc +++ b/test/unit/client/vfs/data/test_file_reader.cc @@ -788,6 +788,9 @@ TEST_F(FileReaderTest, Invalidate_ReadyRequestWithReader_Rereads) { InstallFullSlice(mock_meta_system_); ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 4096))); + ON_CALL(*mock_hub_, GetChunkSize()) + .WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault(Return(4096)); auto first_gate = std::make_shared(); auto second_block_calls = std::make_shared>(0); @@ -855,6 +858,10 @@ TEST_F(FileReaderTest, ConcurrentReadInvalidateClose_Chaos) { InstallFullSlice(mock_meta_system_); ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 64 * 1024))); + ON_CALL(*mock_hub_, GetChunkSize()) + .WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetBlockSize()) + .WillByDefault(Return(64 * 1024)); ON_CALL(*mock_block_store_, RangeAsync) .WillByDefault([](ContextSPtr, RangeReq req, StatusCallback cb) { if (req.dst.base != nullptr && req.length > 0) { @@ -950,6 +957,10 @@ TEST_F(FileReaderTest, ManySmallReads_EvictionBounded) { // always cover offset 0, so nothing could ever be evicted. ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 64 * 1024))); + ON_CALL(*mock_hub_, GetChunkSize()) + .WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetBlockSize()) + .WillByDefault(Return(64 * 1024)); // Count fetches of the file-offset-0 request only: slice 1 block 0 at // in-block offset 0 (strided reads below alias in-block offset 0 in OTHER diff --git a/test/unit/client/vfs/data/test_writer_table.cc b/test/unit/client/vfs/data/test_writer_table.cc index 50c89ca8a..511ed1c01 100644 --- a/test/unit/client/vfs/data/test_writer_table.cc +++ b/test/unit/client/vfs/data/test_writer_table.cc @@ -35,7 +35,6 @@ namespace client { namespace vfs { using dingofs::client::vfs::test::VFSTestBase; -using ::testing::AnyNumber; using ::testing::Return; class WriterTableTest : public VFSTestBase { @@ -218,6 +217,8 @@ TEST_F(WriterTableTest, PressureFlushSeesPartialChunkBeforeNextAdmission) { constexpr uint64_t kChunk = 2 * kPage; ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(kChunk, kChunk))); + ON_CALL(*mock_hub_, GetChunkSize()).WillByDefault(Return(kChunk)); + ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault(Return(kChunk)); WriteMemPool tiny_pool(kChunk, kPage); ON_CALL(*mock_hub_, GetWriteMemPool()).WillByDefault(Return(&tiny_pool)); diff --git a/test/unit/client/vfs/mock/mock_vfs_hub.h b/test/unit/client/vfs/mock/mock_vfs_hub.h index 279296c13..3f69cd034 100644 --- a/test/unit/client/vfs/mock/mock_vfs_hub.h +++ b/test/unit/client/vfs/mock/mock_vfs_hub.h @@ -51,6 +51,9 @@ class MockVFSHub : public VFSHub { MOCK_METHOD(Compactor*, GetCompactor, (), (override)); MOCK_METHOD(TraceManager*, GetTraceManager, (), (override)); MOCK_METHOD(FsInfo, GetFsInfo, (), (override)); + MOCK_METHOD(int32_t, GetChunkSize, (), (override)); + MOCK_METHOD(int32_t, GetBlockSize, (), (override)); + MOCK_METHOD(uint32_t, GetFsId, (), (override)); MOCK_METHOD(UidGidMapper*, GetUidGidMapper, (), (override)); MOCK_METHOD(blockaccess::BlockAccessOptions, GetBlockAccesserOptions, (), (override)); diff --git a/test/unit/client/vfs/test_base.h b/test/unit/client/vfs/test_base.h index 22c435cfb..249a11fbf 100644 --- a/test/unit/client/vfs/test_base.h +++ b/test/unit/client/vfs/test_base.h @@ -140,6 +140,12 @@ class VFSTestBase : public ::testing::Test { ON_CALL(*mock_hub_, GetCBExecutor()) .WillByDefault(Return(cb_executor_.get())); ON_CALL(*mock_hub_, GetFsInfo()).WillByDefault(Return(MakeTestFsInfo())); + ON_CALL(*mock_hub_, GetChunkSize()) + .WillByDefault([&]() { return MakeTestFsInfo().chunk_size; }); + ON_CALL(*mock_hub_, GetBlockSize()) + .WillByDefault([&]() { return MakeTestFsInfo().block_size; }); + ON_CALL(*mock_hub_, GetFsId()) + .WillByDefault([&]() { return MakeTestFsInfo().id; }); // Null mapper => uid/gid translation passthrough. Tests that exercise the // enabled-mapper path override this with their own real mapper. ON_CALL(*mock_hub_, GetUidGidMapper()).WillByDefault(Return(nullptr)); @@ -161,6 +167,9 @@ class VFSTestBase : public ::testing::Test { EXPECT_CALL(*mock_hub_, GetCBExecutor()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetFsInfo()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetUidGidMapper()).Times(AnyNumber()); + EXPECT_CALL(*mock_hub_, GetChunkSize()).Times(AnyNumber()); + EXPECT_CALL(*mock_hub_, GetBlockSize()).Times(AnyNumber()); + EXPECT_CALL(*mock_hub_, GetFsId()).Times(AnyNumber()); // --- 8. MockBlockStore: synchronous success by default --- // Callbacks invoked inline (no async) to eliminate timing non-determinism. From ea252fd2f069ccd05dad2cbe2a2d98f99cae8c24 Mon Sep 17 00:00:00 2001 From: chuandew Date: Tue, 15 Sep 2026 16:41:36 +0800 Subject: [PATCH 3/7] [fix][utils] Wake timers for earlier deadlines; add a deterministic regression test. --- src/utils/executor/timer/timer_impl.cc | 8 ++-- test/unit/utils/executor/test_timer_impl.cc | 46 +++++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/src/utils/executor/timer/timer_impl.cc b/src/utils/executor/timer/timer_impl.cc index 65efbb4c3..b66a7688e 100644 --- a/src/utils/executor/timer/timer_impl.cc +++ b/src/utils/executor/timer/timer_impl.cc @@ -21,6 +21,8 @@ #include #include +#include "common/sync_point.h" + namespace dingofs { using namespace std::chrono; @@ -81,12 +83,11 @@ bool TimerImpl::Add(std::function func, int delay_ms) { return false; } - heap_.push(std::move(fn_info)); - // Run only needs to reconsider its wait when the earliest deadline changes. // This avoids waking the timer thread (and contending its cache lines) for // tasks that do not affect the current minimum. - const bool wake = heap_.size() == 1 || next < heap_.top().next_run_time_us; + const bool wake = heap_.empty() || next < heap_.top().next_run_time_us; + heap_.push(std::move(fn_info)); if (wake) { cv_.notify_one(); } @@ -115,6 +116,7 @@ void TimerImpl::Run() { thread_pool_->Execute(std::move(fn)); lk.lock(); } else { + TEST_SYNC_POINT_CALLBACK("TimerImpl::Run:before_timed_wait", this); cv_.wait_for(lk, microseconds(cur_fn.next_run_time_us - now)); } } diff --git a/test/unit/utils/executor/test_timer_impl.cc b/test/unit/utils/executor/test_timer_impl.cc index 6a211d818..e8f3e6a1c 100644 --- a/test/unit/utils/executor/test_timer_impl.cc +++ b/test/unit/utils/executor/test_timer_impl.cc @@ -17,14 +17,17 @@ #include #include // NOLINT #include // NOLINT +#include #include #include // NOLINT #include // NOLINT +#include "common/sync_point.h" #include "glog/logging.h" #include "gtest/gtest.h" #include "utils/executor/thread/thread_pool_impl.h" #include "utils/executor/timer/timer_impl.h" +#include "utils/scoped_cleanup.h" namespace dingofs { namespace utils { @@ -86,6 +89,49 @@ TEST_F(TimerImplTest, Add) { timer->Stop(); } +TEST_F(TimerImplTest, EarlierDeadlineInterruptsTimedWait) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic timer waiting requires TEST_SYNC_POINT; " + "run this regression in a Debug build."; +#else + std::promise waiting; + auto waiting_future = waiting.get_future(); + std::once_flag waiting_once; + std::promise earlier_ran; + auto earlier_future = earlier_ran.get_future(); + std::atomic later_ran{false}; + auto timer = std::make_unique(pool.get()); + + auto cleanup = MakeScopedCleanup([&] { + timer->Stop(); + pool->Stop(); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + SyncPoint::GetInstance()->SetCallBack( + "TimerImpl::Run:before_timed_wait", [&](void* arg) { + if (arg == timer.get()) { + std::call_once(waiting_once, [&] { waiting.set_value(); }); + } + }); + SyncPoint::GetInstance()->EnableProcessing(); + + ASSERT_TRUE(timer->Start()); + ASSERT_TRUE(timer->Add([&] { later_ran.store(true); }, 60 * 60 * 1000)); + ASSERT_EQ(waiting_future.wait_for(std::chrono::seconds(5)), + std::future_status::ready) + << "Timer did not reach the wait for the distant deadline"; + + // The sync point runs with the timer mutex held. Add cannot acquire it until + // wait_for atomically registers its waiter and releases that mutex. + ASSERT_TRUE(timer->Add([&] { earlier_ran.set_value(); }, 10)); + EXPECT_EQ(earlier_future.wait_for(std::chrono::seconds(5)), + std::future_status::ready) + << "The earlier deadline did not interrupt the existing timed wait"; + EXPECT_FALSE(later_ran.load()); +#endif +} + TEST_F(TimerImplTest, StopDestroysPendingFunctionsOutsideMutex) { auto timer = std::make_unique(pool.get()); ASSERT_TRUE(timer->Start()); From af79b2e2fa60954c9dc5f32784d920e326c21d30 Mon Sep 17 00:00:00 2001 From: chuandew Date: Tue, 15 Sep 2026 18:36:21 +0800 Subject: [PATCH 4/7] [fix][test] Delegate FsInfo accessor mocks to GetFsInfo so geometry overrides stay consistent. --- test/unit/client/vfs/test_base.h | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/unit/client/vfs/test_base.h b/test/unit/client/vfs/test_base.h index 249a11fbf..16e270308 100644 --- a/test/unit/client/vfs/test_base.h +++ b/test/unit/client/vfs/test_base.h @@ -140,12 +140,15 @@ class VFSTestBase : public ::testing::Test { ON_CALL(*mock_hub_, GetCBExecutor()) .WillByDefault(Return(cb_executor_.get())); ON_CALL(*mock_hub_, GetFsInfo()).WillByDefault(Return(MakeTestFsInfo())); + // Delegate to GetFsInfo so a test that overrides the geometry with a + // custom MakeTestFsInfo(chunk, block) keeps all three accessors + // consistent without overriding each one separately. ON_CALL(*mock_hub_, GetChunkSize()) - .WillByDefault([&]() { return MakeTestFsInfo().chunk_size; }); + .WillByDefault([this]() { return mock_hub_->GetFsInfo().chunk_size; }); ON_CALL(*mock_hub_, GetBlockSize()) - .WillByDefault([&]() { return MakeTestFsInfo().block_size; }); + .WillByDefault([this]() { return mock_hub_->GetFsInfo().block_size; }); ON_CALL(*mock_hub_, GetFsId()) - .WillByDefault([&]() { return MakeTestFsInfo().id; }); + .WillByDefault([this]() { return mock_hub_->GetFsInfo().id; }); // Null mapper => uid/gid translation passthrough. Tests that exercise the // enabled-mapper path override this with their own real mapper. ON_CALL(*mock_hub_, GetUidGidMapper()).WillByDefault(Return(nullptr)); From 6e62e1f098851e8c4e323b953102b292476b69a8 Mon Sep 17 00:00:00 2001 From: chuandew Date: Tue, 15 Sep 2026 20:40:30 +0800 Subject: [PATCH 5/7] [perf][client] Shard VFS indexes; preserve reference and shutdown contracts. --- src/client/vfs/data/reader/file_reader.cc | 2 + src/client/vfs/data/reader/reader_registry.cc | 79 +++-- src/client/vfs/data/reader/reader_registry.h | 35 +- src/client/vfs/data/writer_table.cc | 152 ++++---- src/client/vfs/data/writer_table.h | 58 +++- src/client/vfs/handle/handle_manager.cc | 326 +++++++++++------- src/client/vfs/handle/handle_manager.h | 92 ++++- test/unit/client/vfs/data/test_file_reader.cc | 127 ++++++- .../unit/client/vfs/data/test_writer_table.cc | 39 +++ .../client/vfs/handle/test_handle_manager.cc | 94 +++++ 10 files changed, 736 insertions(+), 268 deletions(-) diff --git a/src/client/vfs/data/reader/file_reader.cc b/src/client/vfs/data/reader/file_reader.cc index dba338c03..66f55cfe6 100644 --- a/src/client/vfs/data/reader/file_reader.cc +++ b/src/client/vfs/data/reader/file_reader.cc @@ -45,6 +45,7 @@ #include "client/vfs/hub/vfs_hub.h" #include "client/vfs/vfs_meta.h" #include "common/status.h" +#include "common/sync_point.h" #include "common/trace/context.h" #include "read_request.h" #include "utils/scoped_cleanup.h" @@ -97,6 +98,7 @@ FileReader::FileReader(VFSHub* hub, uint64_t fh, uint64_t ino) FileReader::~FileReader() { CHECK(closing_.load(std::memory_order_acquire)) << uuid_ << " FileReader destructor called without Close"; + TEST_SYNC_POINT_CALLBACK("FileReader::~FileReader", this); { std::vector to_delete; diff --git a/src/client/vfs/data/reader/reader_registry.cc b/src/client/vfs/data/reader/reader_registry.cc index 0c326b1ad..3395eda65 100644 --- a/src/client/vfs/data/reader/reader_registry.cc +++ b/src/client/vfs/data/reader/reader_registry.cc @@ -18,63 +18,82 @@ #include -#include - +#include "absl/hash/hash.h" #include "client/vfs/data/reader/file_reader.h" +#include "common/sync_point.h" namespace dingofs { namespace client { namespace vfs { +ReaderRegistryShard& ReaderRegistry::GetShard(Ino ino) { + return shards_[absl::HashOf(ino) & (kShardCount - 1)]; +} + void ReaderRegistry::Register(FileReader* reader) { CHECK_NOTNULL(reader); + GetShard(reader->GetIno()).Register(reader); +} + +void ReaderRegistry::Unregister(FileReader* reader) { + CHECK_NOTNULL(reader); + GetShard(reader->GetIno()).Unregister(reader); +} + +void ReaderRegistry::InvalidateByIno(Ino ino, int64_t offset, int64_t size) { + auto readers = GetShard(ino).Snapshot(ino); + TEST_SYNC_POINT_CALLBACK("ReaderRegistry::InvalidateByIno:after_snapshot", + this); + + for (auto* reader : readers) { + reader->Invalidate(offset, size); + reader->ReleaseRef(); + } +} + +size_t ReaderRegistry::Size() const { + size_t size = 0; + for (const auto& shard : shards_) { + size += shard.Size(); + } + return size; +} + +void ReaderRegistryShard::Register(FileReader* reader) { const Ino ino = reader->GetIno(); std::lock_guard lock(mutex_); CHECK(readers_[ino].insert(reader).second) << "FileReader registered more than once, ino: " << ino; + ++reader_count_; } -void ReaderRegistry::Unregister(FileReader* reader) { - CHECK_NOTNULL(reader); +void ReaderRegistryShard::Unregister(FileReader* reader) { const Ino ino = reader->GetIno(); std::lock_guard lock(mutex_); auto it = readers_.find(ino); CHECK(it != readers_.end()) << "FileReader inode is not registered: " << ino; CHECK_EQ(it->second.erase(reader), 1) << "FileReader is not registered for inode: " << ino; - if (it->second.empty()) { - readers_.erase(it); - } + --reader_count_; + if (it->second.empty()) readers_.erase(it); } -void ReaderRegistry::InvalidateByIno(Ino ino, int64_t offset, int64_t size) { +std::vector ReaderRegistryShard::Snapshot(Ino ino) { std::vector readers; - { - std::lock_guard lock(mutex_); - auto it = readers_.find(ino); - if (it == readers_.end()) { - return; - } - readers.reserve(it->second.size()); - for (auto* reader : it->second) { - reader->AcquireRef(); - readers.push_back(reader); - } - } - - for (auto* reader : readers) { - reader->Invalidate(offset, size); - reader->ReleaseRef(); + std::lock_guard lock(mutex_); + auto it = readers_.find(ino); + if (it == readers_.end()) return readers; + readers.reserve(it->second.size()); + for (auto* reader : it->second) { + reader->AcquireRef(); + readers.push_back(reader); } + return readers; } -size_t ReaderRegistry::Size() const { +size_t ReaderRegistryShard::Size() const { std::lock_guard lock(mutex_); - size_t size = 0; - for (const auto& entry : readers_) { - size += entry.second.size(); - } - return size; + return reader_count_; } } // namespace vfs diff --git a/src/client/vfs/data/reader/reader_registry.h b/src/client/vfs/data/reader/reader_registry.h index 79c7aedf7..6e320d18a 100644 --- a/src/client/vfs/data/reader/reader_registry.h +++ b/src/client/vfs/data/reader/reader_registry.h @@ -17,11 +17,12 @@ #ifndef DINGOFS_CLIENT_VFS_DATA_READER_READER_REGISTRY_H_ #define DINGOFS_CLIENT_VFS_DATA_READER_READER_REGISTRY_H_ +#include #include -#include #include #include #include +#include #include "client/vfs/vfs_meta.h" @@ -30,10 +31,30 @@ namespace client { namespace vfs { class FileReader; +class ReaderRegistryShard; // Non-owning per-inode index of open FileReaders. HandleResources remains the -// owner of each reader. Registry snapshots pin readers with their intrusive -// refcount and never call FileReader methods while holding mutex_. +// owner of each reader. Snapshots pin readers under their inode's shard lock; +// Invalidate and pin release run outside that lock. The owner drains callers +// before destroying the registry. +// One inode partition: a non-owning reader index with its own lock. The +// registry owns routing; the reader's owner keeps it alive from before +// Register until after Unregister, while the shard owns snapshot pins. +class alignas(64) ReaderRegistryShard { + public: + void Register(FileReader* reader); + void Unregister(FileReader* reader); + // Every returned reader owns one pin. Invalidate and ReleaseRef happen + // after this function releases the lock, including after removal. + std::vector Snapshot(Ino ino); + size_t Size() const; + + private: + mutable std::mutex mutex_; + size_t reader_count_{0}; + std::unordered_map> readers_; +}; + class ReaderRegistry { public: void Register(FileReader* reader); @@ -41,11 +62,15 @@ class ReaderRegistry { void InvalidateByIno(Ino ino, int64_t offset, int64_t size); + // Best-effort number of registered readers, not inode entries. size_t Size() const; private: - mutable std::mutex mutex_; - std::unordered_map> readers_; + static constexpr size_t kShardCount = 64; + + ReaderRegistryShard& GetShard(Ino ino); + + std::array shards_; }; } // namespace vfs diff --git a/src/client/vfs/data/writer_table.cc b/src/client/vfs/data/writer_table.cc index 88af312e9..00a5bae95 100644 --- a/src/client/vfs/data/writer_table.cc +++ b/src/client/vfs/data/writer_table.cc @@ -23,6 +23,7 @@ #include #include +#include "absl/hash/hash.h" #include "client/vfs/data/writer/file_writer.h" namespace dingofs { @@ -36,8 +37,34 @@ WriterTable::~WriterTable() { Stop(); } +WriterTableShard& WriterTable::GetShard(uint64_t ino) { + return shards_[absl::HashOf(ino) & (kShardCount - 1)]; +} + +WriterTable::ShardLocks WriterTable::LockShards() { + ShardLocks locks; + for (size_t i = 0; i < shards_.size(); ++i) { + locks[i] = shards_[i].LockForSnapshot(); + } + return locks; +} + +std::vector WriterTable::Snapshot() { + auto locks = LockShards(); + size_t count = 0; + for (const auto& shard : shards_) { + count += shard.SizeLocked(); + } + std::vector snapshot; + snapshot.reserve(count); + for (auto& shard : shards_) { + shard.AppendPinnedLocked(snapshot); + } + return snapshot; +} + Status WriterTable::Start() { - std::lock_guard lg(mutex_); + std::lock_guard lg(lifecycle_mutex_); if (stopped_) { return Status::Internal("WriterTable already stopped"); } @@ -46,8 +73,14 @@ Status WriterTable::Start() { } void WriterTable::Stop() { - std::lock_guard lg(mutex_); + std::lock_guard lg(lifecycle_mutex_); if (stopped_) return; + { + auto locks = LockShards(); + for (auto& shard : shards_) { + shard.StopLocked(); + } + } stopped_ = true; LOG(INFO) << "WriterTable stopped"; } @@ -56,16 +89,7 @@ Status WriterTable::FlushAll() { // Snapshot live writers and pin each entry with a transient holder. A plain // FileWriter ref would prevent UAF but would still allow the last external // holder to erase the entry and Close() the writer before Flush() starts. - std::vector snap; - { - std::lock_guard lg(mutex_); - snap.reserve(writers_.size()); - for (auto& [ino, e] : writers_) { - e.writer->AcquireRef(); - ++e.holders; - snap.push_back(e.writer); - } - } + auto snap = Snapshot(); Status final_status; for (auto* w : snap) { @@ -83,16 +107,7 @@ Status WriterTable::FlushAll() { } void WriterTable::FlushDirtyAsync(StatusCallback cb) { - std::vector snap; - { - std::lock_guard lg(mutex_); - snap.reserve(writers_.size()); - for (auto& [ino, entry] : writers_) { - entry.writer->AcquireRef(); - ++entry.holders; - snap.push_back(entry.writer); - } - } + auto snap = Snapshot(); if (snap.empty()) { cb(Status::OK()); @@ -134,82 +149,93 @@ void WriterTable::FlushDirtyAsync(StatusCallback cb) { } size_t WriterTable::Size() const { - std::lock_guard lg(mutex_); - return writers_.size(); + size_t count = 0; + for (const auto& shard : shards_) { + count += shard.Size(); + } + return count; } FileWriter* WriterTable::AcquireWriter(uint64_t ino) { - std::lock_guard lg(mutex_); - if (stopped_) { - LOG(WARNING) << "AcquireWriter on stopped WriterTable, ino=" << ino; - return nullptr; + return GetShard(ino).Acquire(ino, vfs_hub_); +} + +FileWriter* WriterTable::PeekWriter(uint64_t ino) { + return GetShard(ino).Peek(ino); +} + +void WriterTable::ReleaseWriter(FileWriter* writer) { + if (writer != nullptr) { + GetShard(writer->Ino()).Release(writer); } +} +FileWriter* WriterTableShard::Acquire(uint64_t ino, VFSHub* hub) { + std::lock_guard lock(mutex_); + if (stopped_) return nullptr; auto it = writers_.find(ino); if (it != writers_.end()) { it->second.writer->AcquireRef(); - it->second.holders++; + ++it->second.holders; return it->second.writer; } - // First-time create. - auto* w = new FileWriter(vfs_hub_, ino); - w->AcquireRef(); // ref balance for the AcquireWriter caller - Status s = w->Open(); - if (!s.ok()) { - LOG(ERROR) << fmt::format("AcquireWriter Open failed, ino={}, status={}", - ino, s.ToString()); - // refs=1; ReleaseRef triggers delete-this. - w->ReleaseRef(); + auto* writer = new FileWriter(hub, ino); + writer->AcquireRef(); + Status status = writer->Open(); + if (!status.ok()) { + LOG(ERROR) << "AcquireWriter Open failed, ino=" << ino + << ", status=" << status.ToString(); + writer->ReleaseRef(); return nullptr; } - writers_.emplace(ino, Entry{w, /*holders*/ 1}); - return w; + writers_.emplace(ino, Entry{writer, 1}); + return writer; } -FileWriter* WriterTable::PeekWriter(uint64_t ino) { - std::lock_guard lg(mutex_); +FileWriter* WriterTableShard::Peek(uint64_t ino) { + std::lock_guard lock(mutex_); if (stopped_) return nullptr; auto it = writers_.find(ino); if (it == writers_.end()) return nullptr; it->second.writer->AcquireRef(); - it->second.holders++; + ++it->second.holders; return it->second.writer; } -void WriterTable::ReleaseWriter(FileWriter* writer) { - if (writer == nullptr) return; - - uint64_t ino = writer->Ino(); - bool need_close = false; +void WriterTableShard::Release(FileWriter* writer) { + const uint64_t ino = writer->Ino(); + bool close = false; { - std::lock_guard lg(mutex_); + std::lock_guard lock(mutex_); auto it = writers_.find(ino); if (it == writers_.end()) { - // Defensive: shouldn't happen if Acquire/Release are balanced. Still - // call ReleaseRef below so the writer doesn't leak. LOG(WARNING) << "ReleaseWriter: ino " << ino << " not in table"; } else { CHECK_EQ(it->second.writer, writer) << "ReleaseWriter: pointer mismatch for ino=" << ino; - it->second.holders--; - CHECK_GE(it->second.holders, 0); - if (it->second.holders == 0) { + CHECK_GT(it->second.holders, 0); + if (--it->second.holders == 0) { writers_.erase(it); - need_close = true; + close = true; } } } + if (close) writer->Close(); + writer->ReleaseRef(); +} + +size_t WriterTableShard::Size() const { + std::lock_guard lock(mutex_); + return writers_.size(); +} - // Close + ReleaseRef are intentionally outside the table mutex: - // - Close drains any inflight flush task; if held under mutex_ it - // would block all other Acquire/Release. - // - ReleaseRef may transitively call delete-this; doing it outside - // also keeps the table mutex's critical section short. - if (need_close) { - writer->Close(); // flips closed_ so SchedulePeriodicFlush stops arming +void WriterTableShard::AppendPinnedLocked(std::vector& out) { + for (auto& [ino, entry] : writers_) { + entry.writer->AcquireRef(); + ++entry.holders; + out.push_back(entry.writer); } - writer->ReleaseRef(); // may delete-this once refs_ reaches 0 } } // namespace vfs diff --git a/src/client/vfs/data/writer_table.h b/src/client/vfs/data/writer_table.h index bcee48078..4eb154995 100644 --- a/src/client/vfs/data/writer_table.h +++ b/src/client/vfs/data/writer_table.h @@ -17,9 +17,12 @@ #ifndef DINGOFS_CLIENT_VFS_DATA_WRITER_TABLE_H_ #define DINGOFS_CLIENT_VFS_DATA_WRITER_TABLE_H_ +#include +#include #include #include #include +#include #include "common/callback.h" #include "common/status.h" @@ -30,6 +33,7 @@ namespace vfs { class VFSHub; class FileWriter; +class WriterTableShard; // WriterTable shares a single FileWriter per inode across all writable fhs. // @@ -61,6 +65,38 @@ class FileWriter; // snapshotted writer before that writer's Flush completes. // - Stop() marks stopped_ to refuse new acquires. It does NOT flush. // Callers that need both should call FlushAll() first. +// One inode partition: its own writer index and lock. The table owns +// routing, snapshots, and lifecycle; holders protect against Close while +// FileWriter refs protect against deletion. +class alignas(64) WriterTableShard { + public: + using Lock = std::unique_lock; + + // Acquire/Peek return one holder; balance every success with Release. + FileWriter* Acquire(uint64_t ino, VFSHub* hub); + FileWriter* Peek(uint64_t ino); + // Erase under the local lock, then Close/ReleaseRef outside it. + void Release(FileWriter* writer); + size_t Size() const; + + // Locked operations require LockForSnapshot(). Multi-shard callers + // acquire locks in ascending order; pinned holders must outlive flushes. + Lock LockForSnapshot() { return Lock(mutex_); } + size_t SizeLocked() const { return writers_.size(); } + void StopLocked() { stopped_ = true; } + void AppendPinnedLocked(std::vector& out); + + private: + struct Entry { + FileWriter* writer; + int64_t holders; + }; + + mutable std::mutex mutex_; + bool stopped_{false}; + std::unordered_map writers_; +}; + class WriterTable { public: explicit WriterTable(VFSHub* hub); @@ -69,15 +105,16 @@ class WriterTable { WriterTable(const WriterTable&) = delete; WriterTable& operator=(const WriterTable&) = delete; - // Lifecycle. + // Lifecycle. The owner must drain users/callbacks before destruction. Status Start(); void Stop(); // refuse new acquires; does not flush - // Synchronously flush all live writers; idempotent. + // Pin a consistent table-membership snapshot, then flush outside shard locks. Status FlushAll(); // Fan-outs all currently dirty writers and invokes cb exactly once after // every participant callback and transient holder release completes. + // The caller keeps the table and writer dependencies alive until cb finishes. void FlushDirtyAsync(StatusCallback cb); // Get-or-create the FileWriter for ino. Returned pointer has a holder @@ -98,16 +135,19 @@ class WriterTable { size_t Size() const; private: - struct Entry { - FileWriter* writer{nullptr}; - int64_t holders{0}; // external holders + transient WriterTable pins - }; + static constexpr size_t kShardCount = 64; + + using ShardLocks = std::array; + + WriterTableShard& GetShard(uint64_t ino); + ShardLocks LockShards(); + std::vector Snapshot(); VFSHub* vfs_hub_{nullptr}; - mutable std::mutex mutex_; - bool stopped_{false}; - std::unordered_map writers_; + std::array shards_; + std::mutex lifecycle_mutex_; + bool stopped_{false}; // Protected by lifecycle_mutex_. }; } // namespace vfs diff --git a/src/client/vfs/handle/handle_manager.cc b/src/client/vfs/handle/handle_manager.cc index 1cf33d30b..fd465960a 100644 --- a/src/client/vfs/handle/handle_manager.cc +++ b/src/client/vfs/handle/handle_manager.cc @@ -24,6 +24,7 @@ #include #include +#include "absl/hash/hash.h" #include "client/vfs/data/reader/file_reader.h" #include "client/vfs/data/reader/reader_registry.h" #include "client/vfs/data/writer/file_writer.h" @@ -31,6 +32,7 @@ #include "client/vfs/hub/vfs_hub.h" #include "client/vfs/vfs_fh.h" #include "common/const.h" +#include "common/sync_point.h" #include "fmt/format.h" namespace dingofs { @@ -45,24 +47,36 @@ std::string Handle::ToString() const { return oss.str(); } +HandleManagerShard& HandleManager::GetShard(uint64_t fh) { + return shards_[absl::HashOf(fh) & (kShardCount - 1)]; +} + +HandleManager::ShardLocks HandleManager::LockShards() { + ShardLocks locks; + for (size_t i = 0; i < shards_.size(); ++i) { + locks[i] = shards_[i].LockForSnapshot(); + } + return locks; +} + +size_t HandleManager::Size() const { + size_t count = 0; + for (const auto& shard : shards_) { + count += shard.Size(); + } + return count; +} + HandleManager::~HandleManager() { Status s = Stop(); if (!s.ok()) { LOG(ERROR) << fmt::format("HandleManager destructor flush failed: {}", s.ToString()); } - std::vector handles; - { - std::unique_lock lock(mutex_); - handles.reserve(handles_.size()); - for (auto& [fh, handle] : handles_) { - handles.push_back(handle); + for (auto& shard : shards_) { + for (auto& [fh, handle] : shard.ExtractAll()) { + ReleaseRefHandle(handle, shard); } - handles_.clear(); - } - - for (auto* handle : handles) { - ReleaseRefHandle(handle); } } @@ -89,36 +103,24 @@ void HandleGuard::Reset() { } } -void HandleManager::AcquireRefHandle(Handle* h) { - int64_t orgin = h->refs.fetch_add(1); - VLOG(12) << fmt::format("handle-{} AcquireRef origin refs: {}", h->fh, orgin); - CHECK_GE(orgin, 0); -} - -void HandleManager::ReleaseRefHandle(Handle* h) { +void HandleManager::ReleaseRefHandle(Handle* h, HandleManagerShard& shard) { + const uint64_t fh = h->fh; int64_t orgin = h->refs.fetch_sub(1); - VLOG(12) << fmt::format("handle-{} ReleaseRef origin refs: {}", h->fh, orgin); + VLOG(12) << fmt::format("handle-{} ReleaseRef origin refs: {}", fh, orgin); CHECK_GT(orgin, 0); if (orgin == 1) { - DestroyHandle(h); + DestroyHandle(h, shard); } - cv_.notify_all(); + shard.NotifyRefReleased(); } -void HandleManager::DestroyHandle(Handle* h) { - HandleResources resources; - { - std::lock_guard lock(mutex_); - resources = DetachHandleResourcesLocked(h); - } - ReleaseHandleResources(resources); +void HandleManager::DestroyHandle(Handle* h, HandleManagerShard& shard) { + ReleaseHandleResources(shard.Detach(h)); delete h; } -void HandleManager::ReleaseGuard(Handle* h) { ReleaseRefHandle(h); } - -HandleResources HandleManager::DetachHandleResourcesLocked(Handle* h) { - return std::exchange(h->resources, {}); +void HandleManager::ReleaseGuard(Handle* h) { + ReleaseRefHandle(h, GetShard(h->fh)); } void HandleManager::ReleaseHandleResources(HandleResources resources) { @@ -136,58 +138,38 @@ void HandleManager::ReleaseHandleResources(HandleResources resources) { Status HandleManager::Start() { return Status::OK(); } Status HandleManager::Stop() { - std::vector resources_to_release; - std::vector stop_refs; + std::lock_guard stop_lock(stop_mutex_); + if (stopped_) { + return Status::OK(); + } + stopped_ = true; + + size_t count = 0; { - std::unique_lock lock(mutex_); - if (stopped_) { - LOG(INFO) << "HandleManager already stopped"; - return Status::OK(); + auto locks = LockShards(); + for (auto& shard : shards_) { + shard.CloseAdmissionLocked(); + count += shard.SizeLocked(); } + } - stopped_ = true; - - cv_.wait(lock, [&]() { - for (auto& [fh, handle] : handles_) { - if (handle->refs.load(std::memory_order_acquire) != 1) { - return false; - } - } - return true; - }); - - for (auto& [fh, handle] : handles_) { - if (handle->ino == kStatsIno) { - continue; - } - AcquireRefHandle(handle); - stop_refs.push_back(handle); - - auto resources = DetachHandleResourcesLocked(handle); - if (resources.reader != nullptr || resources.writer != nullptr) { - resources_to_release.push_back(resources); - } - } + std::vector resources_to_release; + std::vector stop_refs; + resources_to_release.reserve(count); + stop_refs.reserve(count); + for (auto& shard : shards_) { + shard.Drain(resources_to_release, stop_refs); } - // resources_to_release owns the detached writer holders, so every writer is - // still present in WriterTable here. Flush while metadata, BlockStore, and - // writer executors are alive, before the loop below can drop the last holder - // and close the writer. FlushAll deliberately visits every writer and returns - // the first error without stopping early. + // Detached holders pin every writer until the final flush has completed. + // Never release an earlier shard's resources before flushing all writers. Status flush_status = vfs_hub_->GetWriterTable()->FlushAll(); - - // Release resources outside mutex_: dropping the last writer holder may - // block while FileWriter::Close() waits for already in-flight tasks and then - // destroys nested writer resources. for (auto& resources : resources_to_release) { ReleaseHandleResources(resources); } - for (auto* handle : stop_refs) { - ReleaseRefHandle(handle); + ReleaseGuard(handle); } - return flush_status; } @@ -221,69 +203,38 @@ Handle* HandleManager::NewHandle(uint64_t fh, Ino ino, int flags) { vfs_hub_->GetReaderRegistry()->Register(handle->resources.reader); if (!AddHandle(handle)) { - DestroyHandle(handle); + DestroyHandle(handle, GetShard(fh)); return nullptr; } return handle; } bool HandleManager::AddHandle(Handle* handle) { - std::lock_guard lock(mutex_); - if (stopped_) { + if (!GetShard(handle->fh).Add(handle)) { LOG(WARNING) << "AddHandle rejected because HandleManager is stopped, fh: " << handle->fh; return false; } - - AcquireRefHandle(handle); - handles_[handle->fh] = handle; - total_count_ << 1; return true; } void HandleManager::ReleaseHandler(uint64_t fh) { - Handle* h = nullptr; - { - std::lock_guard lock(mutex_); - auto iter = handles_.find(fh); - if (iter == handles_.end()) { - VLOG(1) << "ReleaseHandler ignored, fh not found: " << fh; - return; - } - h = iter->second; - handles_.erase(iter); + auto& shard = GetShard(fh); + auto* handle = shard.Remove(fh); + if (handle != nullptr) { + ReleaseRefHandle(handle, shard); } - // Drop the AddHandle-time ref. DestroyHandle (called when refs→0) does - // reader Close + writer return + delete; running it outside the table - // mutex avoids deadlocks against WriterTable / FileWriter cleanup. - ReleaseRefHandle(h); } HandleGuard HandleManager::FindHandlerGuard(uint64_t fh) { - std::lock_guard lock(mutex_); - if (stopped_) { - return {}; - } - - auto it = handles_.find(fh); - if (it == handles_.end()) { - return {}; - } - - AcquireRefHandle(it->second); - return HandleGuard(this, it->second); + auto* handle = GetShard(fh).Find(fh, /*for_release=*/false); + return handle == nullptr ? HandleGuard{} : HandleGuard(this, handle); } HandleGuard HandleManager::FindHandlerForRelease(uint64_t fh) { - std::lock_guard lock(mutex_); - auto it = handles_.find(fh); - if (it == handles_.end()) { - return {}; - } - - AcquireRefHandle(it->second); - return HandleGuard(this, it->second); + auto* handle = GetShard(fh).Find(fh, /*for_release=*/true); + return handle == nullptr ? HandleGuard{} : HandleGuard(this, handle); } Status HandleManager::FlushByIno(Ino ino) { @@ -302,31 +253,35 @@ Status HandleManager::FlushByIno(Ino ino) { } void HandleManager::Summary(Json::Value& value) { - std::lock_guard lock(mutex_); - value["name"] = "handler"; - value["count"] = handles_.size(); + value["count"] = Size(); value["total_count"] = total_count_.get_value(); } bool HandleManager::Dump(Json::Value& value) { - std::lock_guard lock(mutex_); - Json::Value handlers = Json::arrayValue; - - for (const auto& handle : handles_) { - auto* fileHandle = handle.second; + std::vector snapshot; + { + auto locks = LockShards(); + size_t count = 0; + for (const auto& shard : shards_) { + count += shard.SizeLocked(); + } + snapshot.reserve(count); + for (const auto& shard : shards_) { + shard.AppendIdentitiesLocked(snapshot); + } + } + Json::Value handlers = Json::arrayValue; + for (const auto& handle : snapshot) { Json::Value item; - item["ino"] = fileHandle->ino; - item["fh"] = fileHandle->fh; - item["flags"] = fileHandle->flags; - + item["ino"] = handle.ino; + item["fh"] = handle.fh; + item["flags"] = handle.flags; handlers.append(item); } - value["handlers"] = handlers; - - LOG(INFO) << "successfuly dump " << handles_.size() << " handlers"; - + value["handlers"] = std::move(handlers); + LOG(INFO) << "successfuly dump " << snapshot.size() << " handlers"; return true; } @@ -356,14 +311,119 @@ bool HandleManager::Load(const Json::Value& value) { max_fh = std::max(max_fh, fh); } - vfs::FhGenerator::UpdateNextFh(max_fh + 1); + FhGenerator::UpdateNextFh(max_fh + 1); - LOG(INFO) << "successfuly load " << handles_.size() - << " handlers, next fh is:" << vfs::FhGenerator::GetNextFh(); + LOG(INFO) << "successfuly load " << Size() + << " handlers, next fh is:" << FhGenerator::GetNextFh(); return true; } +void HandleManagerShard::Ref(Handle* handle) { + const int64_t old_refs = handle->refs.fetch_add(1); + VLOG(12) << fmt::format("handle-{} AcquireRef origin refs: {}", handle->fh, + old_refs); + CHECK_GE(old_refs, 0); +} + +bool HandleManagerShard::Add(Handle* handle) { + std::lock_guard lock(mutex_); + if (drain_.closing.load(std::memory_order_seq_cst)) { + return false; + } + CHECK(handles_.emplace(handle->fh, handle).second) + << "Duplicate fh: " << handle->fh; + Ref(handle); + return true; +} + +Handle* HandleManagerShard::Find(uint64_t fh, bool for_release) { + std::lock_guard lock(mutex_); + if (!for_release && drain_.closing.load(std::memory_order_seq_cst)) { + return nullptr; + } + auto it = handles_.find(fh); + if (it == handles_.end()) return nullptr; + Ref(it->second); + return it->second; +} + +Handle* HandleManagerShard::Remove(uint64_t fh) { + std::lock_guard lock(mutex_); + auto it = handles_.find(fh); + if (it == handles_.end()) return nullptr; + auto* handle = it->second; + handles_.erase(it); + return handle; +} + +size_t HandleManagerShard::Size() const { + std::lock_guard lock(mutex_); + return handles_.size(); +} + +HandleManagerShard::HandleMap HandleManagerShard::ExtractAll() { + HandleMap handles; + { + std::lock_guard lock(mutex_); + handles.swap(handles_); + } + return handles; +} + +void HandleManagerShard::CloseAdmissionLocked() { + drain_.closing.store(true, std::memory_order_seq_cst); +} + +void HandleManagerShard::AppendIdentitiesLocked( + std::vector& out) const { + for (const auto& [fh, handle] : handles_) { + out.push_back({handle->ino, fh, handle->flags}); + } +} + +HandleResources HandleManagerShard::DetachLocked(Handle* handle) { + return std::exchange(handle->resources, {}); +} + +HandleResources HandleManagerShard::Detach(Handle* handle) { + std::lock_guard lock(mutex_); + return DetachLocked(handle); +} + +void HandleManagerShard::Drain(std::vector& resources, + std::vector& stop_refs) { + std::unique_lock lock(mutex_); + drain_.cv.wait(lock, [&] { + for (auto& [fh, handle] : handles_) { + if (handle->refs.load(std::memory_order_seq_cst) != 1) { + TEST_SYNC_POINT_CALLBACK("HandleManagerShard::Drain:waiting_for_guard", + handle); + return false; + } + } + return true; + }); + for (auto& [fh, handle] : handles_) { + if (handle->ino == kStatsIno) continue; + Ref(handle); + stop_refs.push_back(handle); + auto detached = DetachLocked(handle); + if (detached.reader != nullptr || detached.writer != nullptr) { + resources.push_back(detached); + } + } +} + +void HandleManagerShard::NotifyRefReleased() { + // SC ordering pairs the preceding ref decrement with closing/predicate. + // Taking mutex_ excludes notification between the predicate and wait. + if (drain_.closing.load(std::memory_order_seq_cst)) { + std::lock_guard lock(mutex_); + drain_.cv.notify_all(); + } +} + } // namespace vfs } // namespace client } // namespace dingofs diff --git a/src/client/vfs/handle/handle_manager.h b/src/client/vfs/handle/handle_manager.h index c3da8d2ec..2416176f9 100644 --- a/src/client/vfs/handle/handle_manager.h +++ b/src/client/vfs/handle/handle_manager.h @@ -17,12 +17,16 @@ #ifndef DINGOFS_CLIENT_VFS_HANDLE_MANAGER_H #define DINGOFS_CLIENT_VFS_HANDLE_MANAGER_H +#include #include #include +#include #include #include #include +#include #include +#include #include "bvar/reducer.h" #include "client/vfs/vfs_meta.h" @@ -37,6 +41,7 @@ class VFSHub; class FileReader; class FileWriter; class HandleManager; +class HandleManagerShard; // temporary store .stats file data struct FileBuffer { @@ -46,7 +51,7 @@ struct FileBuffer { struct HandleResources { FileReader* reader{nullptr}; - FileWriter* writer{nullptr}; // nullptr for O_RDONLY + FileWriter* writer{nullptr}; }; // Handle is a pure-data per-fh value object: identity (fh/ino/flags), the @@ -95,6 +100,54 @@ class HandleGuard { Handle* handle_{nullptr}; }; +// One fh partition: its own index and lock. Handles are added and looked up +// here; HandleManager owns routing, drain, and resource return. +class alignas(64) HandleManagerShard { + public: + using Lock = std::unique_lock; + using HandleMap = std::unordered_map; + struct Identity { + Ino ino; + uint64_t fh; + int32_t flags; + }; + + bool Add(Handle* handle); + Handle* Find(uint64_t fh, bool for_release); + Handle* Remove(uint64_t fh); + size_t Size() const; + HandleMap ExtractAll(); + + // Locked operations require LockForSnapshot(). Multi-shard callers acquire + // locks in ascending shard order and release them before any callback or + // I/O. + Lock LockForSnapshot() { return Lock(mutex_); } + size_t SizeLocked() const { return handles_.size(); } + void CloseAdmissionLocked(); + void AppendIdentitiesLocked(std::vector& out) const; + + // After admission closes, wait for table-resident guards, then pin and + // detach. Callers retain every returned holder until the final flush. + void Drain(std::vector& resources, + std::vector& stop_refs); + HandleResources Detach(Handle* handle); + void NotifyRefReleased(); + + private: + static void Ref(Handle* handle); + static HandleResources DetachLocked(Handle* handle); + + // Guard release reads only this cold line during normal operation. + struct alignas(64) DrainState { + std::atomic closing{false}; + std::condition_variable cv; + }; + + mutable std::mutex mutex_; + HandleMap handles_; + DrainState drain_; +}; + class HandleManager { public: HandleManager(VFSHub* hub) : vfs_hub_(hub){}; @@ -103,6 +156,9 @@ class HandleManager { Status Start(); + // The owner drains public operations before Stop and keeps this manager alive + // through every guard release. Stop drains table-resident guards, flushes + // writers, and detaches resources; erased handles remain the owner's concern. Status Stop(); // Build a new Handle for (fh, ino, flags). Allocates FileReader @@ -110,15 +166,15 @@ class HandleManager { // writable open mode. Returns nullptr on failure. Handle* NewHandle(uint64_t fh, Ino ino, int flags); - // Used by NewHandle and the .stats path. Returns false after Stop() starts; - // ownership stays with the caller in that case. + // Used by NewHandle and the .stats path. fh must be unique. Returns false + // after stop admission closes; ownership stays with the caller on rejection. bool AddHandle(Handle* handle); - // Data-path lookup: returns empty after Stop() starts. + // Data-path lookup: returns empty after stop admission closes. HandleGuard FindHandlerGuard(uint64_t fh); - // Release-path lookup: FUSE_RELEASE must be allowed after Stop() starts so - // the fh identity can be removed from handles_. + // Release-path lookup remains available during/after Stop so a late release + // can remove the fh identity. HandleGuard FindHandlerForRelease(uint64_t fh); void ReleaseHandler(uint64_t fh); @@ -127,39 +183,39 @@ class HandleManager { // is O(1): a single PeekWriter lookup + Flush. Status FlushByIno(Ino ino); + // Best-effort count; Dump captures a consistent identity snapshot. void Summary(Json::Value& value); bool Dump(Json::Value& value); bool Load(const Json::Value& value); private: friend class HandleGuard; + static constexpr size_t kShardCount = 64; + + using ShardLocks = std::array; - // Atomic refs ops on Handle::refs. Caller must use these instead of - // touching Handle::refs directly. - void AcquireRefHandle(Handle* h); + HandleManagerShard& GetShard(uint64_t fh); + ShardLocks LockShards(); + size_t Size() const; // Drop one ref on `h`; if it was the last ref, close the reader, return // the writer to WriterTable, and delete the handle. - void ReleaseRefHandle(Handle* h); + void ReleaseRefHandle(Handle* h, HandleManagerShard& shard); // Internal cleanup: closes reader, returns writer, deletes the handle. // Called from ReleaseRefHandle when refs hit 0. - void DestroyHandle(Handle* h); + void DestroyHandle(Handle* h, HandleManagerShard& shard); void ReleaseGuard(Handle* h); - // Detach resources from the handle identity. Caller must hold mutex_. - HandleResources DetachHandleResourcesLocked(Handle* h); - - // Release detached resources without holding mutex_. + // No index lock may be held while releasing resources. void ReleaseHandleResources(HandleResources resources); VFSHub* vfs_hub_{nullptr}; - std::mutex mutex_; - std::condition_variable cv_; + std::array shards_; + std::mutex stop_mutex_; // Lifecycle only; never on the lookup/release path. bool stopped_{false}; - std::unordered_map handles_; // metrics bvar::Adder total_count_{"vfs_handle_total_count"}; diff --git a/test/unit/client/vfs/data/test_file_reader.cc b/test/unit/client/vfs/data/test_file_reader.cc index 60df80330..a493d2841 100644 --- a/test/unit/client/vfs/data/test_file_reader.cc +++ b/test/unit/client/vfs/data/test_file_reader.cc @@ -19,6 +19,7 @@ #include #include +#include #include #include #include @@ -28,11 +29,13 @@ #include #include #include +#include #include #include "client/vfs/data/reader/file_reader.h" #include "client/vfs/data_buffer.h" #include "common/options/client.h" +#include "common/sync_point.h" #include "common/trace/trace_manager.h" #include "test/unit/client/vfs/test_base.h" #include "utils/scoped_cleanup.h" @@ -598,6 +601,115 @@ struct RangeGate { } // namespace +TEST_F(FileReaderTest, RegistryInvalidatesAllFhsWithoutCrossingInodes) { + gflags::FlagSaver flags; + FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; + InstallFullSlice(mock_meta_system_); + constexpr Ino kOtherIno = kIno + 1; + ON_CALL(*mock_meta_system_, GetAttr(_, kOtherIno, _)) + .WillByDefault(DoAll(SetArgPointee<2>(MakeAttr(kOtherIno, file_length_)), + Return(Status::OK()))); + EXPECT_CALL(*mock_meta_system_, GetAttr(_, kOtherIno, _)).Times(AnyNumber()); + auto payload = std::make_shared>('a'); + ON_CALL(*mock_block_store_, RangeAsync) + .WillByDefault([payload](ContextSPtr, RangeReq req, StatusCallback cb) { + std::memset(req.dst.data(), payload->load(), req.length); + cb(Status::OK()); + }); + + std::array readers{MakeOpenReader(kIno, kFh), + MakeOpenReader(kIno, kFh + 1), + MakeOpenReader(kOtherIno, kFh + 2)}; + for (auto* reader : readers) reader_registry_->Register(reader); + auto cleanup = MakeScopedCleanup([&] { + for (auto* reader : readers) { + reader_registry_->Unregister(reader); + CloseAndRelease(reader); + } + }); + auto read_bytes = [&](FileReader* reader) { + DataBuffer buffer; + uint64_t size = 0; + EXPECT_TRUE(reader->Read(ctx_, &buffer, 4096, 0, &size).ok()); + EXPECT_EQ(size, 4096u); + std::string bytes; + for (const auto& iov : buffer.GatherIOVecs()) { + bytes.append(static_cast(iov.iov_base), iov.iov_len); + } + return bytes; + }; + EXPECT_EQ(reader_registry_->Size(), 3u); + for (auto* reader : readers) + EXPECT_EQ(read_bytes(reader), std::string(4096, 'a')); + payload->store('b'); + reader_registry_->InvalidateByIno(kIno, 0, 4096); + EXPECT_EQ(read_bytes(readers[0]), std::string(4096, 'b')); + EXPECT_EQ(read_bytes(readers[1]), std::string(4096, 'b')); + EXPECT_EQ(read_bytes(readers[2]), std::string(4096, 'a')); +} + +TEST_F(FileReaderTest, RegistrySnapshotPinsReaderAcrossUnregisterAndClose) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic snapshot staging requires TEST_SYNC_POINT."; +#else + gflags::FlagSaver flags; + FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; + auto* reader = MakeOpenReader(); + // Remove the periodic task's self-reference so only the owner and snapshot + // can keep this reader alive. + ASSERT_TRUE(read_cleanup_executor_->Stop()); + reader_registry_->Register(reader); + auto gate = std::make_shared(); + std::atomic destroyed{false}; + std::promise retired; + auto retired_future = retired.get_future(); + std::thread invalidator; + std::thread retirer; + bool retirement_started = false; + auto cleanup = MakeScopedCleanup([&] { + gate->Release(); + if (retirer.joinable()) retirer.join(); + if (invalidator.joinable()) invalidator.join(); + if (!retirement_started) { + reader_registry_->Unregister(reader); + CloseAndRelease(reader); + } + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + SyncPoint::GetInstance()->SetCallBack( + "ReaderRegistry::InvalidateByIno:after_snapshot", [&](void* arg) { + if (arg == reader_registry_.get()) gate->EnterAndWait(); + }); + SyncPoint::GetInstance()->SetCallBack("FileReader::~FileReader", + [&](void* arg) { + if (arg == reader) + destroyed.store(true); + }); + SyncPoint::GetInstance()->EnableProcessing(); + invalidator = + std::thread([&] { reader_registry_->InvalidateByIno(kIno, 0, 4096); }); + ASSERT_TRUE(gate->WaitEntered()); + retirer = std::thread([&] { + reader_registry_->Unregister(reader); + CloseAndRelease(reader); + retired.set_value(); + }); + retirement_started = true; + const bool retired_without_waiting = + retired_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready; + EXPECT_TRUE(retired_without_waiting) + << "Unregister must not wait for an already-pinned invalidation"; + if (retired_without_waiting) EXPECT_FALSE(destroyed.load()); + gate->Release(); + retirer.join(); + invalidator.join(); + EXPECT_TRUE(destroyed.load()); + EXPECT_EQ(reader_registry_->Size(), 0u); +#endif +} + // 15. Regression for the request birth-window race: a request must not become // runnable before its creator registered a reference. With an inline-error // BlockStore the completion path fires as early as possible; pre-fix this @@ -788,8 +900,7 @@ TEST_F(FileReaderTest, Invalidate_ReadyRequestWithReader_Rereads) { InstallFullSlice(mock_meta_system_); ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 4096))); - ON_CALL(*mock_hub_, GetChunkSize()) - .WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetChunkSize()).WillByDefault(Return(4 * 1024 * 1024)); ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault(Return(4096)); auto first_gate = std::make_shared(); @@ -858,10 +969,8 @@ TEST_F(FileReaderTest, ConcurrentReadInvalidateClose_Chaos) { InstallFullSlice(mock_meta_system_); ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 64 * 1024))); - ON_CALL(*mock_hub_, GetChunkSize()) - .WillByDefault(Return(4 * 1024 * 1024)); - ON_CALL(*mock_hub_, GetBlockSize()) - .WillByDefault(Return(64 * 1024)); + ON_CALL(*mock_hub_, GetChunkSize()).WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault(Return(64 * 1024)); ON_CALL(*mock_block_store_, RangeAsync) .WillByDefault([](ContextSPtr, RangeReq req, StatusCallback cb) { if (req.dst.base != nullptr && req.length > 0) { @@ -957,10 +1066,8 @@ TEST_F(FileReaderTest, ManySmallReads_EvictionBounded) { // always cover offset 0, so nothing could ever be evicted. ON_CALL(*mock_hub_, GetFsInfo()) .WillByDefault(Return(test::MakeTestFsInfo(4 * 1024 * 1024, 64 * 1024))); - ON_CALL(*mock_hub_, GetChunkSize()) - .WillByDefault(Return(4 * 1024 * 1024)); - ON_CALL(*mock_hub_, GetBlockSize()) - .WillByDefault(Return(64 * 1024)); + ON_CALL(*mock_hub_, GetChunkSize()).WillByDefault(Return(4 * 1024 * 1024)); + ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault(Return(64 * 1024)); // Count fetches of the file-offset-0 request only: slice 1 block 0 at // in-block offset 0 (strided reads below alias in-block offset 0 in OTHER diff --git a/test/unit/client/vfs/data/test_writer_table.cc b/test/unit/client/vfs/data/test_writer_table.cc index 511ed1c01..13b0bcc0c 100644 --- a/test/unit/client/vfs/data/test_writer_table.cc +++ b/test/unit/client/vfs/data/test_writer_table.cc @@ -17,10 +17,13 @@ #include #include +#include +#include #include #include #include #include +#include #include #include @@ -29,6 +32,7 @@ #include "client/vfs/data/writer_table.h" #include "test/unit/client/vfs/test_base.h" #include "utils/executor/thread/executor_impl.h" +#include "utils/scoped_cleanup.h" namespace dingofs { namespace client { @@ -75,6 +79,41 @@ TEST_F(WriterTableTest, AcquireDedupSameIno) { EXPECT_EQ(table_->Size(), 0u) << "evicted after last holder release"; } +TEST_F(WriterTableTest, ConcurrentAcquiresShareWriterAndReleaseAfterStop) { + constexpr uint64_t kIno = 150; + constexpr size_t kThreads = 8; + std::array writers{}; + std::array threads; + std::promise begin; + auto beginning = begin.get_future().share(); + auto cleanup = MakeScopedCleanup([&] { + for (auto& thread : threads) { + if (thread.joinable()) thread.join(); + } + for (auto* writer : writers) table_->ReleaseWriter(writer); + }); + for (size_t i = 0; i < kThreads; ++i) { + threads[i] = std::thread([&, i] { + beginning.wait(); + writers[i] = table_->AcquireWriter(kIno); + }); + } + begin.set_value(); + for (auto& thread : threads) thread.join(); + ASSERT_NE(writers[0], nullptr); + for (auto* writer : writers) EXPECT_EQ(writer, writers[0]); + EXPECT_EQ(table_->Size(), 1u); + + table_->Stop(); + EXPECT_EQ(table_->AcquireWriter(kIno), nullptr); + EXPECT_EQ(table_->PeekWriter(kIno), nullptr); + for (auto& writer : writers) { + table_->ReleaseWriter(writer); + writer = nullptr; + } + EXPECT_EQ(table_->Size(), 0u); +} + // Different inos get different FileWriters. TEST_F(WriterTableTest, AcquireDistinctIno) { auto* w_a = table_->AcquireWriter(100); diff --git a/test/unit/client/vfs/handle/test_handle_manager.cc b/test/unit/client/vfs/handle/test_handle_manager.cc index e726c2a0b..e782ac0aa 100644 --- a/test/unit/client/vfs/handle/test_handle_manager.cc +++ b/test/unit/client/vfs/handle/test_handle_manager.cc @@ -20,12 +20,17 @@ #include #include +#include +#include #include +#include #include "client/vfs/data/writer/file_writer.h" #include "client/vfs/data/writer_table.h" #include "client/vfs/handle/handle_manager.h" +#include "common/sync_point.h" #include "test/unit/client/vfs/test_base.h" +#include "utils/scoped_cleanup.h" namespace dingofs { namespace client { @@ -359,6 +364,95 @@ TEST_F(HandleManagerTest, ReleaseHandler_AfterStop_Idempotent) { << "identity removed after late release"; } +TEST_F(HandleManagerTest, ConcurrentGuardsSurviveTableRemoval) { + constexpr uint64_t kFh = 600; + constexpr Ino kIno = 1600; + constexpr int kThreads = 8; + ASSERT_NE(handle_manager_->NewHandle(kFh, kIno, O_RDONLY), nullptr); + + std::promise ready; + auto ready_future = ready.get_future(); + std::promise release; + auto released = release.get_future().share(); + std::atomic acquired{0}; + std::atomic errors{0}; + std::vector threads; + bool released_threads = false; + auto cleanup = MakeScopedCleanup([&] { + if (!released_threads) release.set_value(); + for (auto& thread : threads) { + if (thread.joinable()) thread.join(); + } + handle_manager_->ReleaseHandler(kFh); + }); + for (int i = 0; i < kThreads; ++i) { + threads.emplace_back([&] { + auto guard = handle_manager_->FindHandlerGuard(kFh); + if (acquired.fetch_add(1) + 1 == kThreads) ready.set_value(); + released.wait(); + if (!guard || guard->fh != kFh || guard->ino != kIno || + guard->resources.reader == nullptr) { + ++errors; + } + }); + } + ASSERT_EQ(ready_future.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + handle_manager_->ReleaseHandler(kFh); + EXPECT_FALSE(handle_manager_->FindHandlerGuard(kFh)); + EXPECT_EQ(reader_registry_->Size(), 1u); + release.set_value(); + released_threads = true; + for (auto& thread : threads) thread.join(); + EXPECT_EQ(errors.load(), 0); + EXPECT_EQ(reader_registry_->Size(), 0u); +} + +TEST_F(HandleManagerTest, StopWakesWhenTableReferenceIsReleased) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic Stop staging requires TEST_SYNC_POINT."; +#else + constexpr uint64_t kFh = 601; + ASSERT_NE(handle_manager_->NewHandle(kFh, 1601, O_WRONLY), nullptr); + auto guard = handle_manager_->FindHandlerGuard(kFh); + ASSERT_TRUE(guard); + auto* waiting_handle = guard.get(); + std::promise waiting; + auto waiting_future = waiting.get_future(); + std::once_flag waiting_once; + std::promise stopped; + auto stopped_future = stopped.get_future(); + std::thread stopper; + auto cleanup = MakeScopedCleanup([&] { + guard = HandleGuard{}; + if (stopper.joinable()) stopper.join(); + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + SyncPoint::GetInstance()->SetCallBack( + "HandleManagerShard::Drain:waiting_for_guard", [&](void* arg) { + if (arg == waiting_handle) { + std::call_once(waiting_once, [&] { waiting.set_value(); }); + } + }); + SyncPoint::GetInstance()->EnableProcessing(); + stopper = std::thread([&] { stopped.set_value(handle_manager_->Stop()); }); + ASSERT_EQ(waiting_future.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + + // Erasing the entry changes Stop's predicate even while this guard stays + // alive. The map-reference release, not only guard destruction, must notify. + handle_manager_->ReleaseHandler(kFh); + const bool finished = stopped_future.wait_for(std::chrono::seconds(5)) == + std::future_status::ready; + EXPECT_TRUE(finished); + if (finished) EXPECT_TRUE(stopped_future.get().ok()); + guard = HandleGuard{}; + stopper.join(); + EXPECT_EQ(writer_table_->Size(), 0u); +#endif +} + // Core of the N1 fix: Stop() must wait for an outstanding HandleGuard (an // in-flight request) to be released before tearing down resources. TEST_F(HandleManagerTest, Stop_WaitsForOutstandingGuard) { From 0d8e93600eab8e88feb38d12988f6e5a51ea5c87 Mon Sep 17 00:00:00 2001 From: chuandew Date: Thu, 17 Sep 2026 15:10:36 +0800 Subject: [PATCH 6/7] [refactor][client] Unify VFS maintenance; isolate writer cleanup. --- src/client/vfs/components/CMakeLists.txt | 1 + .../vfs/components/maintenance_manager.cc | 253 +++++ .../vfs/components/maintenance_manager.h | 91 ++ src/client/vfs/data/CMakeLists.txt | 2 + src/client/vfs/data/reader/file_reader.cc | 31 +- src/client/vfs/data/reader/file_reader.h | 14 +- src/client/vfs/data/reader/reader_registry.cc | 18 + src/client/vfs/data/reader/reader_registry.h | 17 +- .../vfs/data/reader/reader_registry_task.cc | 62 ++ .../vfs/data/reader/reader_registry_task.h | 44 + .../vfs/data/write_pressure_controller.h | 9 +- src/client/vfs/data/writer/file_writer.cc | 47 +- src/client/vfs/data/writer/file_writer.h | 14 +- src/client/vfs/data/writer_table.cc | 79 +- src/client/vfs/data/writer_table.h | 50 +- src/client/vfs/data/writer_table_task.cc | 128 +++ src/client/vfs/data/writer_table_task.h | 66 ++ src/client/vfs/handle/handle_manager.cc | 5 +- src/client/vfs/hub/vfs_hub.cc | 64 +- src/client/vfs/hub/vfs_hub.h | 44 +- src/common/options/client.cc | 17 + src/common/options/client.h | 1 + .../components/test_maintenance_manager.cc | 339 ++++++ test/unit/client/vfs/data/test_file_reader.cc | 26 +- test/unit/client/vfs/data/test_file_writer.cc | 130 ++- .../vfs/data/test_maintenance_manager.cc | 996 ++++++++++++++++++ .../unit/client/vfs/data/test_writer_table.cc | 294 +++++- test/unit/client/vfs/mock/mock_vfs_hub.h | 1 + test/unit/client/vfs/test_base.h | 31 +- test/unit/client/vfs/test_vfs_impl.cc | 12 +- 30 files changed, 2649 insertions(+), 237 deletions(-) create mode 100644 src/client/vfs/components/maintenance_manager.cc create mode 100644 src/client/vfs/components/maintenance_manager.h create mode 100644 src/client/vfs/data/reader/reader_registry_task.cc create mode 100644 src/client/vfs/data/reader/reader_registry_task.h create mode 100644 src/client/vfs/data/writer_table_task.cc create mode 100644 src/client/vfs/data/writer_table_task.h create mode 100644 test/unit/client/vfs/components/test_maintenance_manager.cc create mode 100644 test/unit/client/vfs/data/test_maintenance_manager.cc diff --git a/src/client/vfs/components/CMakeLists.txt b/src/client/vfs/components/CMakeLists.txt index cfdd8f930..bd6347233 100644 --- a/src/client/vfs/components/CMakeLists.txt +++ b/src/client/vfs/components/CMakeLists.txt @@ -14,6 +14,7 @@ add_library(vfs_components file_suffix_watcher.cc + maintenance_manager.cc prefetch_manager.cc warmup_manager.cc uid_gid_mapper.cc diff --git a/src/client/vfs/components/maintenance_manager.cc b/src/client/vfs/components/maintenance_manager.cc new file mode 100644 index 000000000..3a46f4ca5 --- /dev/null +++ b/src/client/vfs/components/maintenance_manager.cc @@ -0,0 +1,253 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#include "client/vfs/components/maintenance_manager.h" + +#include + +#include +#include +#include +#include +#include +#include + +#include "common/sync_point.h" +#include "utils/executor/executor.h" +#include "utils/scoped_cleanup.h" + +namespace dingofs { +namespace client { +namespace vfs { + +constexpr size_t kRunOnceBudget = 64; + +struct MaintenanceManager::Control { + enum class State : uint8_t { kCreated, kRunning, kStopping, kStopped }; + std::mutex mutex; + std::condition_variable cv; + State state{State::kCreated}; + size_t active_tasks{0}; // queued/running batches and unregister cleanup + std::vector> tasks; // guarded by mutex +}; + +// Published metadata is immutable; scheduling flags share the owning Control +// lock. +struct MaintenanceManager::Registration { + std::string name; + std::shared_ptr task; + Executor* executor; + int interval_ms{0}; + bool scheduled{false}; + bool running{false}; + bool stopping{false}; +}; + +MaintenanceManager::MaintenanceManager() + : control_(std::make_shared()) {} + +MaintenanceManager::~MaintenanceManager() { StopAndDrain(); } + +Status MaintenanceManager::RegisterTask(std::string name, + std::shared_ptr task, + Executor* executor, int interval_ms) { + if (name.empty() || !task || executor == nullptr || interval_ms <= 0) { + return Status::InvalidParam( + "maintenance registration requires name, task, executor and positive " + "interval"); + } + + std::lock_guard lock(control_->mutex); + if (control_->state != Control::State::kCreated) { + return Status::InvalidParam("register maintenance tasks before Start"); + } + + for (const auto& entry : control_->tasks) { + if (entry->name == name || entry->task == task) { + return Status::InvalidParam("maintenance task already registered"); + } + } + auto entry = std::make_shared(); + entry->name = std::move(name); + entry->task = std::move(task); + entry->executor = executor; + entry->interval_ms = interval_ms; + control_->tasks.push_back(std::move(entry)); + return Status::OK(); +} + +Status MaintenanceManager::Start() { + bool accepted = true; + { + std::lock_guard lock(control_->mutex); + if (control_->state != Control::State::kCreated) { + return Status::Internal("maintenance already started or stopped"); + } + control_->state = Control::State::kRunning; + for (const auto& entry : control_->tasks) { + if (!ScheduleNextTick(control_, entry)) { + accepted = false; + break; + } + } + } + + if (!accepted) { + StopAndDrain(); + return Status::Internal("initial maintenance wakeup rejected"); + } + return Status::OK(); +} + +// Caller holds Control::mutex. Timers capture no manager/Hub pointer and only +// weakly reference the registration, so removed tasks do not wait for expiry. +bool MaintenanceManager::ScheduleNextTick( + const std::shared_ptr& control, + const std::shared_ptr& entry) { + std::weak_ptr weak = entry; + return entry->executor->Schedule( + [control, weak] { + if (auto entry = weak.lock()) OnTick(control, entry); + }, + entry->interval_ms); +} + +void MaintenanceManager::OnTick(const std::shared_ptr& control, + const std::shared_ptr& entry) { + bool submit = false; + { + std::lock_guard lock(control->mutex); + if (control->state != Control::State::kRunning || entry->stopping) return; + CHECK(ScheduleNextTick(control, entry)) << "maintenance tick rejected"; + if (!entry->scheduled && !entry->running) { + entry->scheduled = true; + ++control->active_tasks; + submit = true; + } + } + if (submit) QueueRun(control, entry); +} + +void MaintenanceManager::QueueRun(const std::shared_ptr& control, + const std::shared_ptr& entry) { + CHECK(entry->executor->Execute([control, entry] { RunTask(control, entry); })) + << "executor rejected an admitted maintenance task"; +} + +void MaintenanceManager::RunTask(const std::shared_ptr& control, + const std::shared_ptr& entry) { + bool admitted = false; + { + std::lock_guard lock(control->mutex); + entry->scheduled = false; + entry->running = true; + admitted = control->state == Control::State::kRunning && !entry->stopping; + } + + bool again = false; + auto finish = MakeScopedCleanup([&] { FinishRun(control, entry, again); }); + + if (admitted) again = entry->task->RunOnce(kRunOnceBudget); +} + +void MaintenanceManager::FinishRun(const std::shared_ptr& control, + const std::shared_ptr& entry, + bool again) { + bool submit = false; + { + std::lock_guard lock(control->mutex); + entry->running = false; + if (again && control->state == Control::State::kRunning && + !entry->stopping) { + entry->scheduled = true; + submit = true; // transfer the counted unit to the continuation + } else { + CHECK_GT(control->active_tasks, 0u); + --control->active_tasks; + } + control->cv.notify_all(); + } + + if (submit) QueueRun(control, entry); +} + +Status MaintenanceManager::UnregisterTask(const std::string& name) { + auto control = control_; + std::shared_ptr entry; + { + std::unique_lock lock(control->mutex); + auto it = boost::range::find_if( + control->tasks, [&](const auto& task) { return task->name == name; }); + if (it == control->tasks.end()) { + return Status::NotFound("maintenance task not registered"); + } + if (control->state == Control::State::kStopping) { + control->cv.wait( + lock, [&] { return control->state == Control::State::kStopped; }); + return Status::OK(); + } + entry = *it; + entry->stopping = true; + control->tasks.erase(it); + ++control->active_tasks; // Global Stop must include this local OnStop. + TEST_SYNC_POINT("MaintenanceManager:unregister:cancelled"); + control->cv.wait(lock, + [&] { return !entry->scheduled && !entry->running; }); + } + + auto complete = MakeScopedCleanup([control] { + std::lock_guard lock(control->mutex); + --control->active_tasks; + control->cv.notify_all(); + }); + + entry->task->OnStop(); + entry.reset(); + return Status::OK(); +} + +void MaintenanceManager::StopAndDrain() { + std::vector> stopping; + { + std::unique_lock lock(control_->mutex); + if (control_->state == Control::State::kStopped) return; + if (control_->state == Control::State::kStopping) { + control_->cv.wait( + lock, [this] { return control_->state == Control::State::kStopped; }); + return; + } + control_->state = Control::State::kStopping; + for (const auto& entry : control_->tasks) entry->stopping = true; + control_->cv.wait(lock, [&] { return control_->active_tasks == 0; }); + stopping = control_->tasks; + } + + // OnStop may block on task-local cleanup. Never hold Control's mutex here. + for (const auto& entry : stopping) entry->task->OnStop(); + + stopping.clear(); // Make swap below leave the registry empty. + { + std::lock_guard lock(control_->mutex); + control_->tasks.swap(stopping); + } + stopping.clear(); // Destroy task objects outside the scheduler lock. + + { + std::lock_guard lock(control_->mutex); + control_->state = Control::State::kStopped; + control_->cv.notify_all(); + } +} + +} // namespace vfs +} // namespace client +} // namespace dingofs diff --git a/src/client/vfs/components/maintenance_manager.h b/src/client/vfs/components/maintenance_manager.h new file mode 100644 index 000000000..0c0095619 --- /dev/null +++ b/src/client/vfs/components/maintenance_manager.h @@ -0,0 +1,91 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +#ifndef DINGOFS_CLIENT_VFS_COMPONENTS_MAINTENANCE_MANAGER_H_ +#define DINGOFS_CLIENT_VFS_COMPONENTS_MAINTENANCE_MANAGER_H_ + +#include +#include +#include + +#include "common/status.h" + +namespace dingofs { +class Executor; +namespace client { +namespace vfs { + +class MaintenanceTask { + public: + virtual ~MaintenanceTask() = default; + // Process at most budget objects. True requests the next batch immediately; + // false waits for the next tick. Async I/O is task-owned, not waited here. + virtual bool RunOnce(size_t budget) = 0; + // Called on the lifecycle thread after RunOnce calls drain. Release residual + // snapshots and wait for this task's async work and actual holder cleanup. + // Task methods must not re-enter their manager's lifecycle APIs. + virtual void OnStop() = 0; +}; + +// Public scheduling only; components implement and register MaintenanceTask. +class MaintenanceManager final { + public: + MaintenanceManager(); + ~MaintenanceManager(); + MaintenanceManager(const MaintenanceManager&) = delete; + MaintenanceManager& operator=(const MaintenanceManager&) = delete; + + // Register before Start with a positive task-specific interval. Names and + // instances must be unique; executors enqueue, never inline. Dependencies + // outlive OnStop. Executors must accept work until drain completes; rejection + // after Start succeeds is a fatal contract violation. Stopped instances + // cannot be reused; Control's mutex owns all lifecycle transitions, including + // concurrent stop/unregister draining. + Status RegisterTask(std::string name, std::shared_ptr task, + Executor* executor, int interval_ms); + + // Return NotFound if unregistered. Otherwise drain queued/running RunOnce + // calls, invoke OnStop on this thread and wait for its cleanup. If global + // Stop is already draining this task, wait for it instead. Never call from + // a worker needed by the drain. + Status UnregisterTask(const std::string& name); + + // Arm all tasks once. Return Internal if already started/stopped, or if an + // initial wakeup is rejected; the latter fully drains to stopped first. + Status Start(); + + // Close admission, wait for queued/running RunOnce calls, then invoke each + // task's OnStop outside the scheduler lock. Never call from a worker needed + // by the drain. Idempotent; no restart and no wait for passive timer expiry. + void StopAndDrain(); + + private: + struct Control; + struct Registration; + static bool ScheduleNextTick(const std::shared_ptr& control, + const std::shared_ptr& entry); + static void OnTick(const std::shared_ptr& control, + const std::shared_ptr& entry); + static void QueueRun(const std::shared_ptr& control, + const std::shared_ptr& entry); + static void RunTask(const std::shared_ptr& control, + const std::shared_ptr& entry); + static void FinishRun(const std::shared_ptr& control, + const std::shared_ptr& entry, bool again); + + std::shared_ptr control_; +}; + +} // namespace vfs +} // namespace client +} // namespace dingofs +#endif diff --git a/src/client/vfs/data/CMakeLists.txt b/src/client/vfs/data/CMakeLists.txt index 26207ca78..590597a15 100644 --- a/src/client/vfs/data/CMakeLists.txt +++ b/src/client/vfs/data/CMakeLists.txt @@ -29,6 +29,8 @@ add_library(vfs_data writer/file_writer.cc writer/chunk_writer.cc writer_table.cc + reader/reader_registry_task.cc + writer_table_task.cc write_pressure_controller.cc writer/task/file_flush_task.cc writer/task/chunk_flush_task.cc diff --git a/src/client/vfs/data/reader/file_reader.cc b/src/client/vfs/data/reader/file_reader.cc index 66f55cfe6..43df956d5 100644 --- a/src/client/vfs/data/reader/file_reader.cc +++ b/src/client/vfs/data/reader/file_reader.cc @@ -127,12 +127,6 @@ FileReader::~FileReader() { } } -Status FileReader::Open() { - VLOG(9) << fmt::format("{} FileReader opened", uuid_); - SchedulePeriodicShrink(); - return Status::OK(); -} - void FileReader::Close() { if (closing_.load(std::memory_order_acquire)) { return; @@ -236,29 +230,13 @@ void FileReader::ShrinkMem() { } } -void FileReader::SchedulePeriodicShrink() { - if (closing_.load(std::memory_order_acquire)) { - VLOG(8) << fmt::format("{} SchedulePeriodicShrink skipped because closed", - uuid_); - return; - } - - boost::intrusive_ptr self(this); - vfs_hub_->GetReadCleanupExecutor()->Schedule( - [self = std::move(self)] { self->RunPeriodicShrink(); }, - FLAGS_vfs_periodic_flush_interval_ms); -} - -void FileReader::RunPeriodicShrink() { +void FileReader::ShrinkIfOpen() { if (closing_.load(std::memory_order_acquire)) { - VLOG(8) << fmt::format("{} RunPeriodicShrink skipped because closed", - uuid_); + VLOG(8) << fmt::format("{} ShrinkIfOpen skipped because closed", uuid_); return; } ShrinkMem(); - - SchedulePeriodicShrink(); } void FileReader::Invalidate(int64_t offset, int64_t size) { @@ -895,8 +873,9 @@ Status FileReader::Read(ContextSPtr ctx, DataBuffer* data_buffer, int64_t size, // Foreground backpressure: above the backpressure watermark, wait a bounded // window for in-flight reads to release their slots (and for the periodic - // RunPeriodicShrink to reclaim idle readahead) before proceeding. Reclaim is - // intentionally NOT driven from the read path -- it runs on its own timer. + // ShrinkIfOpen maintenance to reclaim idle readahead) before proceeding. + // Reclaim is intentionally NOT driven from the read path -- the hub-level + // registered ReaderRegistryTask owns it. // The per-request pool Allocate still hard-fails -> ENOMEM if truly exhausted // (pool-only, no malloc). Step-3 follow-up may turn this into a bounded // cv-wait. diff --git a/src/client/vfs/data/reader/file_reader.h b/src/client/vfs/data/reader/file_reader.h index 6bee45b65..09ba3f284 100644 --- a/src/client/vfs/data/reader/file_reader.h +++ b/src/client/vfs/data/reader/file_reader.h @@ -44,13 +44,18 @@ class FileReader { ~FileReader(); - Status Open(); - void Close(); Status Read(ContextSPtr ctx, DataBuffer* data_buffer, int64_t size, int64_t offset, uint64_t* out_rsize); + // Single-shot maintenance entry for the hub-level periodic scanner: runs + // the idle readahead reclaim (ShrinkMem) once if the reader is still open. + // Unlike the removed per-object periodic task, it never re-arms itself. + // Safe to call concurrently with Close/Read; snapshot holders keep the + // object alive, and the closing flag makes this a no-op after Close. + void ShrinkIfOpen(); + // NOTE: if we manage filehandle by ino, // then write/commit_slice/fallocate/truncate/copyfile_range should call this void Invalidate(int64_t offset, int64_t size); @@ -64,9 +69,12 @@ class FileReader { private: friend class FileReaderTestPeer; + friend class ReaderRegistryTaskTestPeer; + friend void intrusive_ptr_add_ref(FileReader* reader) { reader->AcquireRef(); } + friend void intrusive_ptr_release(FileReader* reader) { reader->ReleaseRef(); } @@ -77,8 +85,6 @@ class FileReader { const FileRange& frange); void ShrinkMem(); - void SchedulePeriodicShrink(); - void RunPeriodicShrink(); int64_t TotalMem() const; int64_t UsedMem() const; diff --git a/src/client/vfs/data/reader/reader_registry.cc b/src/client/vfs/data/reader/reader_registry.cc index 3395eda65..3542998f4 100644 --- a/src/client/vfs/data/reader/reader_registry.cc +++ b/src/client/vfs/data/reader/reader_registry.cc @@ -51,6 +51,11 @@ void ReaderRegistry::InvalidateByIno(Ino ino, int64_t offset, int64_t size) { } } +std::vector ReaderRegistry::SnapshotShard(size_t shard_index) { + CHECK_LT(shard_index, kShardCount); + return shards_[shard_index].SnapshotAll(); +} + size_t ReaderRegistry::Size() const { size_t size = 0; for (const auto& shard : shards_) { @@ -91,6 +96,19 @@ std::vector ReaderRegistryShard::Snapshot(Ino ino) { return readers; } +std::vector ReaderRegistryShard::SnapshotAll() { + std::vector readers; + std::lock_guard lock(mutex_); + readers.reserve(reader_count_); + for (const auto& [ino, reader_set] : readers_) { + for (FileReader* reader : reader_set) { + reader->AcquireRef(); + readers.push_back(reader); + } + } + return readers; +} + size_t ReaderRegistryShard::Size() const { std::lock_guard lock(mutex_); return reader_count_; diff --git a/src/client/vfs/data/reader/reader_registry.h b/src/client/vfs/data/reader/reader_registry.h index 6e320d18a..6da4b351c 100644 --- a/src/client/vfs/data/reader/reader_registry.h +++ b/src/client/vfs/data/reader/reader_registry.h @@ -47,6 +47,11 @@ class alignas(64) ReaderRegistryShard { // Every returned reader owns one pin. Invalidate and ReleaseRef happen // after this function releases the lock, including after removal. std::vector Snapshot(Ino ino); + // Background single-shard snapshot across all inodes of this shard: every + // returned reader owns one reference pin taken under the shard lock. The + // caller releases refs outside any registry lock. Closed readers may + // appear; their maintenance entry re-checks the closing flag. + std::vector SnapshotAll(); size_t Size() const; private: @@ -57,17 +62,25 @@ class alignas(64) ReaderRegistryShard { class ReaderRegistry { public: + // Shard count for background single-shard scans (see SnapshotShard). + static constexpr size_t kShardCount = 64; + void Register(FileReader* reader); void Unregister(FileReader* reader); void InvalidateByIno(Ino ino, int64_t offset, int64_t size); + // Background single-shard snapshot (shard_index < kShardCount): pins every + // reader of that shard with a reference under the shard's lock only. The + // caller owns releasing the refs outside any registry lock. A round is not + // a consistent whole-table snapshot; readers registered after the shard was + // visited are seen on the next round. + std::vector SnapshotShard(size_t shard_index); + // Best-effort number of registered readers, not inode entries. size_t Size() const; private: - static constexpr size_t kShardCount = 64; - ReaderRegistryShard& GetShard(Ino ino); std::array shards_; diff --git a/src/client/vfs/data/reader/reader_registry_task.cc b/src/client/vfs/data/reader/reader_registry_task.cc new file mode 100644 index 000000000..23a1cf2ed --- /dev/null +++ b/src/client/vfs/data/reader/reader_registry_task.cc @@ -0,0 +1,62 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "client/vfs/data/reader/reader_registry_task.h" + +#include + +#include +#include + +#include "client/vfs/data/reader/file_reader.h" +#include "client/vfs/data/reader/reader_registry.h" +#include "common/sync_point.h" +namespace dingofs { +namespace client { +namespace vfs { +ReaderRegistryTask::ReaderRegistryTask(ReaderRegistry* registry) + : registry_(CHECK_NOTNULL(registry)) {} + +bool ReaderRegistryTask::RunOnce(size_t budget) { + CHECK_GT(budget, 0u); + while (next_ == snapshot_.size()) { + snapshot_.clear(); + next_ = 0; + if (shard_ == ReaderRegistry::kShardCount) { + shard_ = 0; + return false; + } + snapshot_ = registry_->SnapshotShard(shard_++); + } + const size_t end = next_ + std::min(budget, snapshot_.size() - next_); + while (next_ < end) { + auto* reader = snapshot_[next_++]; + reader->ShrinkIfOpen(); + reader->ReleaseRef(); + } + TEST_SYNC_POINT("ReaderRegistryTask:after_batch"); + return true; +} + +void ReaderRegistryTask::OnStop() { + // Scheduler guarantees the RunOnce stack has returned before OnStop. + while (next_ < snapshot_.size()) snapshot_[next_++]->ReleaseRef(); + snapshot_.clear(); + next_ = 0; +} +} // namespace vfs +} // namespace client +} // namespace dingofs diff --git a/src/client/vfs/data/reader/reader_registry_task.h b/src/client/vfs/data/reader/reader_registry_task.h new file mode 100644 index 000000000..29d3bdce5 --- /dev/null +++ b/src/client/vfs/data/reader/reader_registry_task.h @@ -0,0 +1,44 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef DINGOFS_CLIENT_VFS_DATA_READER_READER_REGISTRY_TASK_H_ +#define DINGOFS_CLIENT_VFS_DATA_READER_READER_REGISTRY_TASK_H_ +#include + +#include "client/vfs/components/maintenance_manager.h" +namespace dingofs { +namespace client { +namespace vfs { +class FileReader; +class ReaderRegistry; + +// Registered reader shrink operation. Registry outlives OnStop completion. +class ReaderRegistryTask final : public MaintenanceTask { + public: + explicit ReaderRegistryTask(ReaderRegistry* registry); + bool RunOnce(size_t budget) override; + void OnStop() override; + + private: + ReaderRegistry* registry_; + size_t shard_{0}; + size_t next_{0}; + std::vector snapshot_; +}; +} // namespace vfs +} // namespace client +} // namespace dingofs +#endif diff --git a/src/client/vfs/data/write_pressure_controller.h b/src/client/vfs/data/write_pressure_controller.h index ef6b91d30..1d97b91dc 100644 --- a/src/client/vfs/data/write_pressure_controller.h +++ b/src/client/vfs/data/write_pressure_controller.h @@ -39,6 +39,8 @@ class WriterTable; // to fan out dirty writers. It does not rate-limit backend PUT traffic. class WritePressureController final : public WritePressureObserver { public: + // The table owns flush/cleanup coordination. Both dependencies and the + // table's cleanup executor must remain alive until StopAndDrain returns. WritePressureController(WriterTable* writer_table, Executor* executor); ~WritePressureController() override; @@ -49,9 +51,9 @@ class WritePressureController final : public WritePressureObserver { // only mutates the small controller state and submits work to executor. void OnWritePressure() override; - // Rejects new events and waits for the active round, its writer callbacks, - // and any bounded submit retry to finish. The executor and WriterTable must - // remain alive until this returns. + // Rejects new events and waits for the active round, including the table's + // holder cleanup and any bounded submit retry. All dependencies remain + // alive until this returns; do not call from a worker needed by the round. void StopAndDrain(); private: @@ -68,6 +70,7 @@ class WritePressureController final : public WritePressureObserver { std::mutex mutex_; std::condition_variable cv_; + bool running_{false}; bool pending_{false}; bool stopped_{false}; diff --git a/src/client/vfs/data/writer/file_writer.cc b/src/client/vfs/data/writer/file_writer.cc index c5f354445..b35723373 100644 --- a/src/client/vfs/data/writer/file_writer.cc +++ b/src/client/vfs/data/writer/file_writer.cc @@ -22,7 +22,6 @@ #include #include -#include #include #include #include @@ -55,16 +54,9 @@ size_t PageNeed(uint64_t offset, uint64_t size, uint64_t page_size) { FileWriter::~FileWriter() { Close(); } -Status FileWriter::Open() { - VLOG(9) << fmt::format("{} FileWriter opened", uuid_); - SchedulePeriodicFlush(); - return Status::OK(); -} - void FileWriter::Close() { std::unique_lock lg(mutex_); if (closed_) { - LOG(INFO) << fmt::format("{} FileWriter already closed", uuid_); return; } @@ -323,14 +315,17 @@ void FileWriter::AsyncFlush(StatusCallback cb) { ReleaseRef(); }); } + void FileWriter::FlushDirtyAsync(StatusCallback cb) { bool dirty = false; bool closed = false; + { std::lock_guard lock(mutex_); closed = closed_; dirty = write_generation_ > flushed_generation_; } + if (closed) { cb(Status::BadFd("file already closed")); } else if (!dirty) { @@ -361,42 +356,6 @@ void FileWriter::SetStatusIfBroken(const Status& s) { } } -void FileWriter::SchedulePeriodicFlush() { - { - std::lock_guard lock(mutex_); - if (closed_) { - LOG(INFO) << fmt::format("{} ScheduleFlush skipped because closed", - uuid_); - return; - } - } - - boost::intrusive_ptr self(this); - vfs_hub_->GetWriteBackgroundExecutor()->Schedule( - [self = std::move(self)] { self->RunPeriodicFlush(); }, - FLAGS_vfs_periodic_flush_interval_ms); -} - -void FileWriter::RunPeriodicFlush() { - { - std::lock_guard lock(mutex_); - if (closed_) { - LOG(INFO) << fmt::format("{} RunPeriodicFlush skipped because closed", - uuid_); - return; - } - } - - AsyncFlush([this](Status s) { - if (!s.ok()) { - LOG(ERROR) << fmt::format("{} RunPeriodicFlush failed, status: {}", uuid_, - s.ToString()); - } - }); - - SchedulePeriodicFlush(); -} - } // namespace vfs } // namespace client } // namespace dingofs diff --git a/src/client/vfs/data/writer/file_writer.h b/src/client/vfs/data/writer/file_writer.h index 8b8e0a34e..a371ca627 100644 --- a/src/client/vfs/data/writer/file_writer.h +++ b/src/client/vfs/data/writer/file_writer.h @@ -41,8 +41,6 @@ class FileWriter { ~FileWriter(); - Status Open(); - void Close(); Status Write(ContextSPtr ctx, const char* buf, uint64_t size, uint64_t offset, @@ -50,8 +48,13 @@ class FileWriter { Status Flush(); - // Starts a flush only when published writes are not fully flushed. The - // callback is invoked exactly once, possibly inline for a clean writer. + // Dirty-only flush for pressure and periodic maintenance, not a final Flush. + // Completes exactly once: clean/empty writers inline with OK, closed writers + // inline with BadFd. Concurrent writes may require a later round. + // Callbacks may run inline or on a flush completion thread: transfer holder + // release to the independent cleanup executor, never synchronously Close or + // drain the callback's executor. Keep the writer and its dependencies alive + // through callback completion. void FlushDirtyAsync(StatusCallback cb); void AcquireRef(); @@ -79,9 +82,6 @@ class FileWriter { void AsyncFlush(StatusCallback cb); - void SchedulePeriodicFlush(); - void RunPeriodicFlush(); - ChunkWriter* GetOrCreateChunkWriter(int64_t chunk_index); void FileFlushTaskDone(uint64_t file_flush_id, uint64_t target_generation, diff --git a/src/client/vfs/data/writer_table.cc b/src/client/vfs/data/writer_table.cc index 00a5bae95..1d33401ee 100644 --- a/src/client/vfs/data/writer_table.cc +++ b/src/client/vfs/data/writer_table.cc @@ -25,6 +25,8 @@ #include "absl/hash/hash.h" #include "client/vfs/data/writer/file_writer.h" +#include "client/vfs/hub/vfs_hub.h" +#include "utils/executor/executor.h" namespace dingofs { namespace client { @@ -114,40 +116,70 @@ void WriterTable::FlushDirtyAsync(StatusCallback cb) { return; } + Executor* cleanup_executor = CHECK_NOTNULL(vfs_hub_->GetCleanupExecutor()); + struct FlushGroup { std::mutex mutex; size_t remaining{0}; Status status; StatusCallback done; - }; - auto group = std::make_shared(); - group->remaining = snap.size(); - group->done = std::move(cb); - for (FileWriter* writer : snap) { - writer->FlushDirtyAsync([this, writer, group](Status status) { - // Holder release is part of round completion: it may be the last holder - // and synchronously close the writer. - ReleaseWriter(writer); - - StatusCallback done; + // Called only after the member's holder has actually been released. + void CompleteMember(const Status& member_status) { + StatusCallback callback; Status final_status; { - std::lock_guard lock(group->mutex); - if (!status.ok() && group->status.ok()) { - group->status = status; + std::lock_guard lock(mutex); + if (!member_status.ok() && status.ok()) { + status = member_status; } - CHECK_GT(group->remaining, 0); - if (--group->remaining == 0) { - final_status = group->status; - done = std::move(group->done); + CHECK_GT(remaining, 0); + if (--remaining == 0) { + final_status = status; + callback = std::move(done); } } - if (done) done(std::move(final_status)); - }); + if (callback) callback(std::move(final_status)); + } + }; + + auto group = std::make_shared(); + group->remaining = snap.size(); + group->done = std::move(cb); + + for (FileWriter* writer : snap) { + writer->FlushDirtyAsync( + [this, cleanup_executor, writer, group](Status status) { + // Member completion only transfers responsibility: the holder release + // below may be the last holder and synchronously Close the writer, + // which can block on flush/CB/storage progress. Running that on this + // completion thread (possibly a CBExecutor worker) self-deadlocks, so + // it must happen on the independent cleanup executor. + if (!cleanup_executor->Execute( + [this, writer, group, status = std::move(status)] { + ReleaseWriter(writer); + group->CompleteMember(status); + })) { + // A live ExecutorImpl never rejects; rejection means the "cleanup + // executor outlives every producer" lifecycle invariant is broken. + // Leak-free fallbacks (inline Close, drop the holder, fake success) + // are all forbidden by the interface contract. + LOG(FATAL) << "WriterTable::FlushDirtyAsync: cleanup executor " + "rejected the member cleanup task"; + } + }); } } +std::vector WriterTable::SnapshotShard(size_t shard_index) { + CHECK_LT(shard_index, kShardCount); + std::vector out; + auto& shard = shards_[shard_index]; + auto lock = shard.LockForSnapshot(); + shard.AppendPinnedLocked(out); + return out; +} + size_t WriterTable::Size() const { size_t count = 0; for (const auto& shard : shards_) { @@ -182,13 +214,6 @@ FileWriter* WriterTableShard::Acquire(uint64_t ino, VFSHub* hub) { auto* writer = new FileWriter(hub, ino); writer->AcquireRef(); - Status status = writer->Open(); - if (!status.ok()) { - LOG(ERROR) << "AcquireWriter Open failed, ino=" << ino - << ", status=" << status.ToString(); - writer->ReleaseRef(); - return nullptr; - } writers_.emplace(ino, Entry{writer, 1}); return writer; } diff --git a/src/client/vfs/data/writer_table.h b/src/client/vfs/data/writer_table.h index 4eb154995..453e9d4bc 100644 --- a/src/client/vfs/data/writer_table.h +++ b/src/client/vfs/data/writer_table.h @@ -28,36 +28,34 @@ #include "common/status.h" namespace dingofs { + namespace client { namespace vfs { class VFSHub; class FileWriter; -class WriterTableShard; // WriterTable shares a single FileWriter per inode across all writable fhs. // -// Responsibility scope: index + lifetime + share. Periodic flush is left to -// each FileWriter's own self-managed scheduler (SchedulePeriodicFlush / -// RunPeriodicFlush in file_writer.cc). The table only orchestrates Close() -// on eviction so the writer's self-loop can terminate. +// Responsibility scope: index + lifetime + share. Periodic flush is driven by +// a WriterTableTask registered with MaintenanceManager, not by self-arming +// tasks. The table only orchestrates Close() on eviction. // // Lifetime contract — two-layer ref counting: // - FileWriter::refs_ remains the single source of truth for "is the // object alive". `FileWriter::ReleaseRef()` is the sole place that // calls `delete this` (when refs_ hits 0). // - WriterTable maintains a *separate* `holders` counter per entry. It counts -// outstanding AcquireWriter / PeekWriter callers and short-lived FlushAll -// pins. It does NOT count internal FileWriter lambdas (e.g. async flush -// callbacks that AcquireRef/ReleaseRef themselves). +// outstanding AcquireWriter / PeekWriter callers and short-lived +// FlushAll/background snapshot pins. It does NOT count internal +// FileWriter lambdas (e.g. async flush callbacks that +// AcquireRef/ReleaseRef themselves). // - When `holders` drops to 0, the entry is removed from the map BEFORE // the matching `Close()` and `ReleaseRef()` calls. Any lingering // internal lambdas will eventually drop refs_ to 0 and delete-this; // by then the map no longer has a dangling pointer to chase. -// - Close() on the evicted writer flips its `closed_` flag so its own -// periodic flush loop stops re-arming itself, allowing the self-ref -// to drain on the next interval (≤ FLAGS_vfs_periodic_flush_interval_ms -// latency until refs_ actually hits 0 and the writer is destroyed). +// - Close() on the evicted writer flips its `closed_` flag so late flush +// attempts fail fast instead of writing behind the metadata lifecycle. // // FlushAll vs Stop: // - FlushAll() synchronously flushes every live writer; its transient holder @@ -99,6 +97,9 @@ class alignas(64) WriterTableShard { class WriterTable { public: + // Shard count for background single-shard scans (see SnapshotShard). + static constexpr size_t kShardCount = 64; + explicit WriterTable(VFSHub* hub); ~WriterTable(); @@ -112,11 +113,21 @@ class WriterTable { // Pin a consistent table-membership snapshot, then flush outside shard locks. Status FlushAll(); - // Fan-outs all currently dirty writers and invokes cb exactly once after - // every participant callback and transient holder release completes. - // The caller keeps the table and writer dependencies alive until cb finishes. + // Flush dirty snapshot members, then release each holder on the hub's + // independent cleanup executor before completing cb exactly once. + // The hub, table, cleanup executor and writer dependencies must remain + // alive until cb finishes. Cleanup must enqueue, never run inline or reject + // accepted-lifecycle work. Empty snapshots complete inline; otherwise cb + // runs on cleanup. A callback must not synchronously drain its own executor. void FlushDirtyAsync(StatusCallback cb); + // Background single-shard snapshot: pins every writer of shard + // `shard_index` (< kShardCount) with a FileWriter ref plus a transient + // holder, under that shard's lock only. The caller owns the release + // responsibility (ReleaseWriter per entry, typically via a cleanup + // executor); never hold shard locks while processing or releasing. + std::vector SnapshotShard(size_t shard_index); + // Get-or-create the FileWriter for ino. Returned pointer has a holder // outstanding; caller MUST call ReleaseWriter exactly once. FileWriter* AcquireWriter(uint64_t ino); @@ -126,17 +137,16 @@ class WriterTable { FileWriter* PeekWriter(uint64_t ino); // Drop one holder. If it was the last holder, the entry is erased from - // the map, Close() is called on the writer (so its self-managed - // periodic flush stops re-arming), and finally ReleaseRef() is invoked - // (which may delete-this once refs_ reaches 0). + // the map and Close() is called on the writer (which may synchronously + // flush and block on storage/CB dependencies), and finally ReleaseRef() + // is invoked (which may delete-this once refs_ reaches 0). Do not call + // this while holding any table/maintenance lock. void ReleaseWriter(FileWriter* writer); // Number of live entries (best-effort, for metrics / tests). size_t Size() const; private: - static constexpr size_t kShardCount = 64; - using ShardLocks = std::array; WriterTableShard& GetShard(uint64_t ino); diff --git a/src/client/vfs/data/writer_table_task.cc b/src/client/vfs/data/writer_table_task.cc new file mode 100644 index 000000000..4ed97ea13 --- /dev/null +++ b/src/client/vfs/data/writer_table_task.cc @@ -0,0 +1,128 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law + * or agreed to in writing, software distributed under the License is + * distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ +#include "client/vfs/data/writer_table_task.h" + +#include + +#include +#include +#include + +#include "client/vfs/data/writer/file_writer.h" +#include "client/vfs/data/writer_table.h" +#include "common/sync_point.h" +#include "utils/executor/executor.h" + +namespace dingofs { +namespace client { +namespace vfs { +WriterTableTask::WriterTableTask(WriterTable* table, Executor* cleanup_executor) + : table_(CHECK_NOTNULL(table)), + cleanup_executor_(CHECK_NOTNULL(cleanup_executor)) {} + +bool WriterTableTask::RunOnce(size_t budget) { + CHECK_GT(budget, 0u); + while (next_ == snapshot_.size()) { + snapshot_.clear(); + next_ = 0; + if (shard_ == WriterTable::kShardCount) { + shard_ = 0; + return false; + } + snapshot_ = table_->SnapshotShard(shard_++); + } + + std::array batch{}; + size_t count = 0; + { + std::lock_guard lock(mutex_); + if (stopping_) return false; + while (next_ < snapshot_.size() && count < std::min(budget, kMaxInFlight) && + in_flight_ < kMaxInFlight) { + auto* writer = snapshot_[next_++]; + batch[count++] = {writer, flushing_.insert(writer).second}; + ++in_flight_; + } + } + if (count == 0) { + TEST_SYNC_POINT("WriterTableTask:paused"); + return false; // Keep the cursor; the next periodic tick retries it. + } + for (size_t i = 0; i < count; ++i) StartFlush(batch[i]); + return true; +} + +void WriterTableTask::StartFlush(Member member) { + if (!member.owns_record) { + QueueCleanup(member); // Even a duplicate snapshot owns an extra holder. + return; + } + TEST_SYNC_POINT_CALLBACK("WriterTableTask:before_flush", member.writer); + member.writer->FlushDirtyAsync( + [self = shared_from_this(), member](Status status) { + if (!status.ok()) + LOG(WARNING) << "Periodic flush failed: " << status.ToString(); + self->QueueCleanup(member); + }); +} + +void WriterTableTask::QueueCleanup(Member member) { + CHECK(cleanup_executor_->Execute([self = shared_from_this(), member] { + self->Cleanup(member); + })) << "cleanup executor rejected maintenance holder"; +} + +void WriterTableTask::Cleanup(Member member) { + { + std::lock_guard lock(mutex_); + if (member.owns_record) flushing_.erase(member.writer); + } + table_->ReleaseWriter( + member.writer); // May synchronously Close; no task lock. + { + std::lock_guard lock(mutex_); + CHECK_GT(in_flight_, 0u); + --in_flight_; // Only completed resource release returns capacity. + drained_.notify_all(); + } +} + +void WriterTableTask::OnStop() { + std::unique_lock lock(mutex_); + stopping_ = true; + drained_.wait(lock, [this] { return in_flight_ == 0; }); + DCHECK(flushing_.empty()); + + snapshot_.erase(snapshot_.begin(), snapshot_.begin() + next_); + auto tail = std::exchange(snapshot_, {}); + next_ = 0; + if (tail.empty()) return; + + // All active members drained. Release the unprocessed tail as one cleanup + // operation; shutdown does not need a scan slot or a retained resume token. + in_flight_ = 1; + lock.unlock(); + + CHECK(cleanup_executor_->Execute([self = shared_from_this(), + tail = std::move(tail)] { + for (auto* writer : tail) self->table_->ReleaseWriter(writer); + std::lock_guard lock(self->mutex_); + --self->in_flight_; + self->drained_.notify_all(); + })) << "cleanup executor rejected residual snapshot"; + + lock.lock(); + drained_.wait(lock, [this] { return in_flight_ == 0; }); +} +} // namespace vfs +} // namespace client +} // namespace dingofs diff --git a/src/client/vfs/data/writer_table_task.h b/src/client/vfs/data/writer_table_task.h new file mode 100644 index 000000000..c9d169ad9 --- /dev/null +++ b/src/client/vfs/data/writer_table_task.h @@ -0,0 +1,66 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law + * or agreed to in writing, software distributed under the License is + * distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ +#ifndef DINGOFS_CLIENT_VFS_DATA_WRITER_TABLE_TASK_H_ +#define DINGOFS_CLIENT_VFS_DATA_WRITER_TABLE_TASK_H_ +#include +#include +#include +#include +#include + +#include "client/vfs/components/maintenance_manager.h" +namespace dingofs { +class Executor; +namespace client { +namespace vfs { +class FileWriter; +class WriterTable; + +// No global manager pointer. The table and cleanup executor outlive OnStop. +// A full slot budget returns to the scheduler; the next tick resumes next_. +class WriterTableTask final + : public MaintenanceTask, + public std::enable_shared_from_this { + public: + WriterTableTask(WriterTable* table, Executor* cleanup_executor); + bool RunOnce(size_t budget) override; + void OnStop() override; + + private: + static constexpr size_t kMaxInFlight = 64; + struct Member { + FileWriter* writer; + bool owns_record; + }; + void StartFlush(Member member); + void QueueCleanup(Member member); + void Cleanup(Member member); + + WriterTable* table_; + Executor* cleanup_executor_; + + // Scan state: RunOnce and OnStop are manager-serialized, not mutex-guarded. + size_t shard_{0}; + size_t next_{0}; + std::vector snapshot_; + std::mutex mutex_; + std::condition_variable drained_; + // Async state: guarded by mutex_, including access from cleanup callbacks. + size_t in_flight_{0}; // includes actual holder release + bool stopping_{false}; + std::unordered_set + flushing_; // avoid duplicate cross-round work +}; +} // namespace vfs +} // namespace client +} // namespace dingofs +#endif diff --git a/src/client/vfs/handle/handle_manager.cc b/src/client/vfs/handle/handle_manager.cc index fd465960a..6d72bbc0b 100644 --- a/src/client/vfs/handle/handle_manager.cc +++ b/src/client/vfs/handle/handle_manager.cc @@ -179,11 +179,10 @@ Handle* HandleManager::NewHandle(uint64_t fh, Ino ino, int flags) { handle->ino = ino; handle->flags = flags; - // Reader is always per-fh. + // Reader is always per-fh. Creation registers nothing: periodic + // maintenance discovers readers through ReaderRegistry snapshots. handle->resources.reader = new FileReader(vfs_hub_, fh, ino); handle->resources.reader->AcquireRef(); - CHECK(handle->resources.reader->Open().ok()) - << "FileReader::Open is currently infallible"; // Writer only for writable opens. Borrowed from WriterTable. if ((flags & O_ACCMODE) != O_RDONLY) { handle->resources.writer = vfs_hub_->GetWriterTable()->AcquireWriter(ino); diff --git a/src/client/vfs/hub/vfs_hub.cc b/src/client/vfs/hub/vfs_hub.cc index 310dbac4f..f1a7be1a7 100644 --- a/src/client/vfs/hub/vfs_hub.cc +++ b/src/client/vfs/hub/vfs_hub.cc @@ -27,11 +27,13 @@ #include "client/vfs/blockstore/fake_block_store.h" #include "client/vfs/common/helper.h" #include "client/vfs/compaction/compactor_impl.h" +#include "client/vfs/components/maintenance_manager.h" #include "client/vfs/components/prefetch_manager.h" -#include "client/vfs/components/warmup_manager.h" #include "client/vfs/data/reader/reader_registry.h" +#include "client/vfs/data/reader/reader_registry_task.h" #include "client/vfs/data/write_pressure_controller.h" #include "client/vfs/data/writer_table.h" +#include "client/vfs/data/writer_table_task.h" #include "client/vfs/metasystem/local/metasystem.h" #include "client/vfs/metasystem/mds/metasystem.h" #include "client/vfs/metasystem/memory/metasystem.h" @@ -58,7 +60,7 @@ static const std::string kReadCleanupExecutorName = "vfs_read_cleanup"; static const std::string kWriteBackgroundExecutorName = "vfs_write_bg"; static const std::string kCBExecutorName = "vfs_callback"; static const std::string kWritePressureExecutorName = "vfs_write_pressure"; - +static const std::string kCleanupExecutorName = "vfs_cleanup"; static MetaSystemUPtr BuildMetaSystem(const VFSConfig& vfs_conf, const ClientId& client_id, TraceManager& trace_manager, @@ -105,6 +107,10 @@ VFSHubImpl::~VFSHubImpl() { // override it. Stop(/*skip_unmount=*/false); + if (maintenance_manager_ != nullptr) { + maintenance_manager_.reset(); + } + if (handle_manager_ != nullptr) { handle_manager_.reset(); } @@ -137,6 +143,10 @@ VFSHubImpl::~VFSHubImpl() { write_pressure_executor_.reset(); } + if (cleanup_executor_ != nullptr) { + cleanup_executor_.reset(); + } + if (warmup_manager_ != nullptr) { warmup_manager_.reset(); } @@ -366,6 +376,18 @@ Status VFSHubImpl::Start(bool skip_mount) { } } + { + if (FLAGS_vfs_cleanup_executor_thread <= 0) { + return Status::InvalidParam( + "vfs_cleanup_executor_thread must be positive"); + } + cleanup_executor_ = std::make_unique( + kCleanupExecutorName, FLAGS_vfs_cleanup_executor_thread); + if (!cleanup_executor_->Start()) { + return Status::Internal("cleanup executor start fail"); + } + } + write_buffer_manager_ = std::make_unique( write_buffer_total_bytes, FLAGS_vfs_write_buffer_page_size); write_pressure_controller_ = std::make_unique( @@ -457,6 +479,27 @@ Status VFSHubImpl::Start(bool skip_mount) { started_.store(true, std::memory_order_relaxed); + // Arm the unified periodic maintenance last. Its first tick is at least + // one positive interval away, so by the time any maintenance callback + // runs, every started_-gated accessor is available. A failure here fails + // Start; the armed teardown gate rolls everything back via Stop(). + { + maintenance_manager_ = std::make_unique(); + DINGOFS_RETURN_NOT_OK(maintenance_manager_->RegisterTask( + "reader-shrink", + std::make_shared(reader_registry_.get()), + read_cleanup_executor_.get(), FLAGS_vfs_periodic_flush_interval_ms)); + DINGOFS_RETURN_NOT_OK(maintenance_manager_->RegisterTask( + "writer-flush", + std::make_shared(writer_table_.get(), + cleanup_executor_.get()), + write_background_executor_.get(), + FLAGS_vfs_periodic_flush_interval_ms)); + // Keep accessor readiness until Stop drains all previously started users, + // including when maintenance startup fails. + DINGOFS_RETURN_NOT_OK(maintenance_manager_->Start()); + } + return Status::OK(); } @@ -493,9 +536,17 @@ Status VFSHubImpl::Stop(bool skip_unmount) { compactor_->Stop(); } + // Drain the unified periodic maintenance (both streams and every cleanup + // it published) before the pressure round. CleanupExecutor, flush/CB/ + // storage dependencies all stay alive throughout this drain. + if (maintenance_manager_ != nullptr) { + maintenance_manager_->StopAndDrain(); + } + // Drain the event-driven flush round before HandleManager performs the final // synchronous writer flush. This prevents overlapping pressure and shutdown - // flush ownership. + // flush ownership. Completion includes every member holder release on the + // cleanup executor. if (write_pressure_controller_ != nullptr) { write_pressure_controller_->StopAndDrain(); } @@ -503,6 +554,13 @@ Status VFSHubImpl::Stop(bool skip_unmount) { write_pressure_executor_->Stop(); } + // Both cleanup producers (periodic maintenance and pressure rounds) have + // drained; join the cleanup executor before HandleManager's final + // synchronous flush, which no longer publishes cleanup tasks. + if (cleanup_executor_ != nullptr) { + cleanup_executor_->Stop(); + } + Status handle_stop_status; if (handle_manager_ != nullptr) { handle_stop_status = handle_manager_->Stop(); diff --git a/src/client/vfs/hub/vfs_hub.h b/src/client/vfs/hub/vfs_hub.h index abcf478ca..627b4d743 100644 --- a/src/client/vfs/hub/vfs_hub.h +++ b/src/client/vfs/hub/vfs_hub.h @@ -48,6 +48,7 @@ namespace vfs { class WriterTable; // forward decl; full include lives in vfs_hub.cc class WritePressureController; +class MaintenanceManager; class ReaderRegistry; class VFSHub { @@ -84,6 +85,12 @@ class VFSHub { virtual Executor* GetCBExecutor() = 0; + // Independent cleanup executor ("vfs_cleanup"): runs possibly-blocking + // background writer holder releases (pressure rounds and periodic + // maintenance) off callback/scan threads. Producers must drain before it + // is stopped; see the CleanupExecutor design contract in writer_table.h. + virtual Executor* GetCleanupExecutor() = 0; + virtual WriteMemPool* GetWriteMemPool() = 0; virtual ReadMemPool* GetReadMemPool() = 0; @@ -173,6 +180,11 @@ class VFSHubImpl : public VFSHub { return flush_executor_.get(); } + Executor* GetCleanupExecutor() override { + CHECK_NOTNULL(cleanup_executor_); + return cleanup_executor_.get(); + } + Executor* GetCBExecutor() override { CHECK_NOTNULL(cb_executor_); return cb_executor_.get(); @@ -286,23 +298,39 @@ class VFSHubImpl : public VFSHub { std::unique_ptr block_store_; std::unique_ptr read_executor_; - // Reader-local cleanup only: periodic shrink and read-request cleanup. - // It must not issue block_store I/O. Keep it alive until after - // block_store_->Shutdown(), because cache bthread read completions can still - // schedule cleanup work while block_store is draining. + // Reader-local cleanup only: read-request cleanup and the reader leg of + // periodic maintenance. It must not issue block_store I/O. Keep it alive + // until after block_store_->Shutdown(), because cache bthread read + // completions can still schedule cleanup work while block_store is + // draining. std::unique_ptr read_cleanup_executor_; - // Writer-side background work: periodic flush scheduling and slice-id - // pre-allocation. These tasks may touch flush_executor, block_store, and - // meta_system, so Stop() must drain this executor before those dependencies - // are torn down. + // Writer-side background work: periodic maintenance scans and slice-id + // pre-allocation. These tasks may touch flush_executor, block_store, and + // meta_system, so Stop() must drain this executor before those + // dependencies are torn down. std::unique_ptr write_background_executor_; std::unique_ptr flush_executor_; std::unique_ptr cb_executor_; std::unique_ptr write_pressure_executor_; + + // "vfs_cleanup": independent executor for possibly-blocking background + // writer holder releases (pressure rounds and periodic maintenance). + // Started after every producer dependency; stopped only after + // MaintenanceManager and WritePressureController have drained, so their + // cleanups can run until then, and before HandleManager's final + // synchronous flush so that flush produces no new cleanup tasks. + std::unique_ptr cleanup_executor_; + std::unique_ptr write_buffer_manager_; std::unique_ptr write_pressure_controller_; + + // Unified periodic maintenance: scans ReaderRegistry/WriterTable and + // publishes writer holder cleanup to cleanup_executor_. Armed last in + // Start (after started_ is set), drained first in Stop. + std::unique_ptr maintenance_manager_; + std::unique_ptr read_mem_pool_; std::unique_ptr read_mem_pool_vars_; // after pool: dtor first diff --git a/src/common/options/client.cc b/src/common/options/client.cc index 707bc8ca4..874b3c18a 100644 --- a/src/common/options/client.cc +++ b/src/common/options/client.cc @@ -138,6 +138,23 @@ DEFINE_int32(vfs_periodic_flush_interval_ms, 5000, "periodic flush interval in milliseconds"); DEFINE_validator(vfs_periodic_flush_interval_ms, brpc::PassValidate); +// vfs_cleanup runs the possibly-blocking holder cleanup for background +// writer flushes (pressure rounds and periodic maintenance): ReleaseWriter +// may synchronously Close a FileWriter and wait on flush/CB/storage +// dependencies. It must never run that on callback or scan threads, and it +// is not shared with read-cleanup / write-background / write-pressure. +DEFINE_int32(vfs_cleanup_executor_thread, 4, + "number of vfs cleanup executor threads; must be positive"); +DEFINE_validator(vfs_cleanup_executor_thread, + [](const char* /*flag_name*/, int32_t value) -> bool { + if (value <= 0) { + LOG(ERROR) + << "vfs_cleanup_executor_thread must be positive."; + return false; + } + return true; + }); + DEFINE_int32(vfs_periodic_trim_mem_ms, 3000, "periodic trim mem in milliseconds"); DEFINE_validator(vfs_periodic_trim_mem_ms, brpc::PassValidate); diff --git a/src/common/options/client.h b/src/common/options/client.h index 4f09659af..3930cee46 100644 --- a/src/common/options/client.h +++ b/src/common/options/client.h @@ -115,6 +115,7 @@ DECLARE_bool(vfs_meta_warmup_small_file_enable); DECLARE_int32(vfs_read_cleanup_executor_thread); DECLARE_int32(vfs_write_background_executor_thread); DECLARE_int32(vfs_periodic_flush_interval_ms); +DECLARE_int32(vfs_cleanup_executor_thread); DECLARE_int32(vfs_periodic_trim_mem_ms); // vfs meta diff --git a/test/unit/client/vfs/components/test_maintenance_manager.cc b/test/unit/client/vfs/components/test_maintenance_manager.cc new file mode 100644 index 000000000..29f1cb3b4 --- /dev/null +++ b/test/unit/client/vfs/components/test_maintenance_manager.cc @@ -0,0 +1,339 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 Unless required by applicable law + * or agreed to in writing, software distributed under the License is + * distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the specific language + * governing permissions and limitations under the License. + */ +#include +#include +#include + +#include +#include +#include +#include +#include +#include + +#include "client/vfs/components/maintenance_manager.h" +#include "common/options/client.h" +#include "common/sync_point.h" +#include "utils/executor/executor.h" +#include "utils/scoped_cleanup.h" + +namespace dingofs { +namespace client { +namespace vfs { +namespace { +class ManualMaintenanceExecutor final : public Executor { + struct Timer { + int64_t deadline; + std::function run; + }; + + public: + bool Start() override { return true; } + bool Stop() override { return true; } + bool Execute(std::function task) override { + std::lock_guard lock(mutex_); + tasks_.push_back(std::move(task)); + return true; + } + bool Schedule(std::function task, int delay_ms) override { + std::lock_guard lock(mutex_); + timers_.push_back({now_ms_ + delay_ms, std::move(task)}); + return true; + } + int ThreadNum() const override { return 1; } + int TaskNum() const override { + std::lock_guard lock(mutex_); + return tasks_.size() + timers_.size(); + } + std::string Name() const override { return "maintenance-test"; } + void RunOne() { + std::function task; + { + std::lock_guard lock(mutex_); + if (!tasks_.empty()) { + task = std::move(tasks_.front()); + tasks_.pop_front(); + } else { + CHECK(!timers_.empty()); + auto timer = EarliestTimerLocked(); + now_ms_ = timer->deadline; + task = std::move(timer->run); + timers_.erase(timer); + } + } + task(); + } + void Advance(int milliseconds) { + int64_t target; + { + std::lock_guard lock(mutex_); + target = now_ms_ + milliseconds; + } + for (;;) { + std::function task; + { + std::lock_guard lock(mutex_); + if (!tasks_.empty()) { + task = std::move(tasks_.front()); + tasks_.pop_front(); + } else { + auto timer = EarliestTimerLocked(); + if (timer == timers_.end() || timer->deadline > target) { + now_ms_ = target; + return; + } + now_ms_ = timer->deadline; + task = std::move(timer->run); + timers_.erase(timer); + } + } + task(); + } + } + + private: + std::deque::iterator EarliestTimerLocked() { + auto first = timers_.end(); + for (auto it = timers_.begin(); it != timers_.end(); ++it) { + if (first == timers_.end() || it->deadline < first->deadline) first = it; + } + return first; + } + mutable std::mutex mutex_; + std::deque> tasks_; + std::deque timers_; + int64_t now_ms_{0}; +}; + +class ProbeMaintenanceTask final : public MaintenanceTask { + public: + ProbeMaintenanceTask(bool block_run = false, bool block_stop = false, + int batches = 1) + : allow_run_(!block_run), allow_stop_(!block_stop), batches_(batches) {} + bool RunOnce(size_t budget) override { + EXPECT_GT(budget, 0u); + std::unique_lock lock(mutex_); + ++runs; + cv_.notify_all(); + cv_.wait(lock, [&] { return allow_run_; }); + return runs < batches_; + } + void OnStop() override { + std::unique_lock lock(mutex_); + ++stops; + cv_.notify_all(); + cv_.wait(lock, [&] { return allow_stop_; }); + } + bool WaitForRun() { + std::unique_lock lock(mutex_); + return cv_.wait_for(lock, std::chrono::seconds(5), + [&] { return runs != 0; }); + } + bool WaitForStop() { + std::unique_lock lock(mutex_); + return cv_.wait_for(lock, std::chrono::seconds(5), + [&] { return stops != 0; }); + } + void ReleaseRun() { + std::lock_guard lock(mutex_); + allow_run_ = true; + cv_.notify_all(); + } + void ReleaseStop() { + std::lock_guard lock(mutex_); + allow_stop_ = true; + cv_.notify_all(); + } + std::atomic runs{0}; + std::atomic stops{0}; + + private: + std::mutex mutex_; + std::condition_variable cv_; + bool allow_run_; + bool allow_stop_; + const int batches_; +}; +} // namespace + +TEST(MaintenanceRegistrationTest, + RegistersAnUnrelatedTaskAndRejectsDuplicates) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("other", task, &executor, 1000).ok()); + EXPECT_FALSE(manager + .RegisterTask("other", + std::make_shared(), + &executor, 1000) + .ok()); + EXPECT_FALSE(manager.RegisterTask("alias", task, &executor, 1000).ok()); + ASSERT_TRUE(manager.Start().ok()); + EXPECT_FALSE(manager + .RegisterTask("late", + std::make_shared(), + &executor, 1000) + .ok()); + executor.RunOne(); + executor.RunOne(); + EXPECT_EQ(task->runs, 1); + ASSERT_TRUE(manager.UnregisterTask("other").ok()); + EXPECT_EQ(task->stops, 1); + executor.RunOne(); + EXPECT_EQ(task->runs, 1); + EXPECT_EQ(executor.TaskNum(), 0); +} + +TEST(MaintenanceRegistrationTest, UnregisterDrainsAnAlreadyQueuedStep) { +#ifdef NDEBUG + GTEST_SKIP() << "Queued cancellation boundary requires SyncPoint."; +#else + ManualMaintenanceExecutor executor; + auto task = std::make_shared(); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("queued", task, &executor, 1000).ok()); + ASSERT_TRUE(manager.Start().ok()); + executor.RunOne(); + std::promise cancelled; + SyncPoint::GetInstance()->SetCallBack( + "MaintenanceManager:unregister:cancelled", + [&](void*) { cancelled.set_value(); }); + SyncPoint::GetInstance()->EnableProcessing(); + auto reset = MakeScopedCleanup([] { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + auto result = std::async(std::launch::async, + [&] { return manager.UnregisterTask("queued"); }); + ASSERT_EQ(cancelled.get_future().wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_EQ(result.wait_for(std::chrono::milliseconds(0)), + std::future_status::timeout); + executor.RunOne(); + EXPECT_TRUE(result.get().ok()); + EXPECT_EQ(task->runs, 0); + EXPECT_EQ(task->stops, 1); +#endif +} + +TEST(MaintenanceRegistrationTest, UnregisterWaitsForTaskLocalCleanup) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(false, true); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("cleanup", task, &executor, 1000).ok()); + ASSERT_TRUE(manager.Start().ok()); + executor.RunOne(); + executor.RunOne(); + auto result = std::async(std::launch::async, + [&] { return manager.UnregisterTask("cleanup"); }); + ASSERT_TRUE(task->WaitForStop()); + EXPECT_EQ(result.wait_for(std::chrono::milliseconds(0)), + std::future_status::timeout); + task->ReleaseStop(); + EXPECT_TRUE(result.get().ok()); + executor.RunOne(); + EXPECT_EQ(task->runs, 1); +} + +TEST(MaintenanceRegistrationTest, StopWaitsForRunningStepBeforeOnStop) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(true, false); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("running", task, &executor, 1000).ok()); + ASSERT_TRUE(manager.Start().ok()); + executor.RunOne(); + auto worker = std::async(std::launch::async, [&] { executor.RunOne(); }); + ASSERT_TRUE(task->WaitForRun()); + auto stopped = + std::async(std::launch::async, [&] { manager.StopAndDrain(); }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(20)), + std::future_status::timeout); + EXPECT_EQ(task->stops, 0); + task->ReleaseRun(); + worker.get(); + stopped.get(); + EXPECT_EQ(task->stops, 1); +} + +TEST(MaintenanceRegistrationTest, + ContinuesBatchesWithoutWaitingForAnotherTick) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(false, false, 3); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("batches", task, &executor, 1000).ok()); + ASSERT_TRUE(manager.Start().ok()); + executor.RunOne(); + executor.RunOne(); + executor.RunOne(); + executor.RunOne(); + EXPECT_EQ(task->runs, 3); + manager.StopAndDrain(); + EXPECT_EQ(task->stops, 1); +} + +TEST(MaintenanceRegistrationTest, EachTaskRunsAtItsRegisteredInterval) { + ManualMaintenanceExecutor executor; + auto fast = std::make_shared(); + auto slow = std::make_shared(); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("fast", fast, &executor, 500).ok()); + ASSERT_TRUE(manager.RegisterTask("slow", slow, &executor, 2000).ok()); + ASSERT_TRUE(manager.Start().ok()); + executor.Advance(499); + EXPECT_EQ(fast->runs, 0); + EXPECT_EQ(slow->runs, 0); + executor.Advance(1); + EXPECT_EQ(fast->runs, 1); + EXPECT_EQ(slow->runs, 0); + executor.Advance(1500); + EXPECT_EQ(fast->runs, 4); + EXPECT_EQ(slow->runs, 1); +} + +TEST(MaintenanceRegistrationTest, + GlobalStopWaitsForConcurrentUnregisterCleanup) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(false, true); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("cleanup", task, &executor, 1000).ok()); + auto unregister = std::async( + std::launch::async, [&] { return manager.UnregisterTask("cleanup"); }); + ASSERT_TRUE(task->WaitForStop()); + auto stopped = + std::async(std::launch::async, [&] { manager.StopAndDrain(); }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(20)), + std::future_status::timeout); + task->ReleaseStop(); + EXPECT_TRUE(unregister.get().ok()); + stopped.get(); + EXPECT_EQ(task->stops, 1); +} + +TEST(MaintenanceRegistrationTest, ConcurrentStopWaitsForTheFirstDrain) { + ManualMaintenanceExecutor executor; + auto task = std::make_shared(false, true); + MaintenanceManager manager; + ASSERT_TRUE(manager.RegisterTask("cleanup", task, &executor, 1000).ok()); + auto first = std::async(std::launch::async, [&] { manager.StopAndDrain(); }); + ASSERT_TRUE(task->WaitForStop()); + auto second = std::async(std::launch::async, [&] { manager.StopAndDrain(); }); + EXPECT_EQ(second.wait_for(std::chrono::milliseconds(20)), + std::future_status::timeout); + task->ReleaseStop(); + first.get(); + second.get(); + EXPECT_EQ(task->stops, 1); +} + +} // namespace vfs +} // namespace client +} // namespace dingofs diff --git a/test/unit/client/vfs/data/test_file_reader.cc b/test/unit/client/vfs/data/test_file_reader.cc index a493d2841..303939e4e 100644 --- a/test/unit/client/vfs/data/test_file_reader.cc +++ b/test/unit/client/vfs/data/test_file_reader.cc @@ -78,11 +78,10 @@ class FileReaderTest : public test::VFSTestBase { EXPECT_CALL(*mock_meta_system_, GetAttr(_, kIno, _)).Times(AnyNumber()); } - // Creates, acquires ref, and opens a FileReader. + // Creates and acquires a ref on a FileReader. FileReader* MakeOpenReader(uint64_t ino = kIno, uint64_t fh = kFh) { auto* r = new FileReader(mock_hub_, fh, ino); r->AcquireRef(); - CHECK(r->Open().ok()); return r; } @@ -129,20 +128,6 @@ class FileReaderTestPeer { } }; -TEST_F(FileReaderTest, StopReleasesPendingPeriodicTaskRef) { - gflags::FlagSaver flag_saver; - FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; - - auto* reader = MakeOpenReader(); - ASSERT_EQ(FileReaderTestPeer::RefCount(reader), 2); - - reader->Close(); - ASSERT_TRUE(read_cleanup_executor_->Stop()); - EXPECT_EQ(FileReaderTestPeer::RefCount(reader), 1); - - reader->ReleaseRef(); -} - // 1. Read() of a zero-length range returns 0 bytes. TEST_F(FileReaderTest, Read_ZeroSize_ReturnsZero) { auto* r = MakeOpenReader(); @@ -602,8 +587,6 @@ struct RangeGate { } // namespace TEST_F(FileReaderTest, RegistryInvalidatesAllFhsWithoutCrossingInodes) { - gflags::FlagSaver flags; - FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; InstallFullSlice(mock_meta_system_); constexpr Ino kOtherIno = kIno + 1; ON_CALL(*mock_meta_system_, GetAttr(_, kOtherIno, _)) @@ -652,12 +635,9 @@ TEST_F(FileReaderTest, RegistrySnapshotPinsReaderAcrossUnregisterAndClose) { #ifdef NDEBUG GTEST_SKIP() << "Deterministic snapshot staging requires TEST_SYNC_POINT."; #else - gflags::FlagSaver flags; - FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; auto* reader = MakeOpenReader(); - // Remove the periodic task's self-reference so only the owner and snapshot - // can keep this reader alive. - ASSERT_TRUE(read_cleanup_executor_->Stop()); + // No per-object periodic task holds a reference anymore: only the owner + // and a registry snapshot can keep this reader alive. reader_registry_->Register(reader); auto gate = std::make_shared(); std::atomic destroyed{false}; diff --git a/test/unit/client/vfs/data/test_file_writer.cc b/test/unit/client/vfs/data/test_file_writer.cc index d00814466..0a1ca39fc 100644 --- a/test/unit/client/vfs/data/test_file_writer.cc +++ b/test/unit/client/vfs/data/test_file_writer.cc @@ -14,20 +14,19 @@ * limitations under the License. */ -#include #include #include #include #include #include +#include #include #include #include #include #include "client/vfs/data/writer/file_writer.h" -#include "common/options/client.h" #include "common/trace/trace_manager.h" #include "common/writemempool/write_mem_pool.h" #include "test/unit/client/vfs/test_base.h" @@ -60,14 +59,12 @@ class FileWriterTest : public VFSTestBase { std::unique_ptr trace_manager_; - // Creates, acquires a ref on, and opens a FileWriter. - // The caller owns the writer; ReleaseRef() destroys it. - // (`fh` arg kept for legacy test call sites; ignored under the per-inode - // shared writer model.) + // Creates and acquires a ref on a FileWriter. The caller owns the writer; + // ReleaseRef() destroys it. (`fh` arg kept for legacy test call sites; + // ignored under the per-inode shared writer model.) FileWriter* MakeOpenWriter(uint64_t ino = 200, uint64_t /*fh*/ = 2) { auto* w = new FileWriter(mock_hub_, ino); w->AcquireRef(); - CHECK(w->Open().ok()); return w; } @@ -78,20 +75,6 @@ class FileWriterTest : public VFSTestBase { } }; -TEST_F(FileWriterTest, StopReleasesPendingPeriodicTaskRef) { - gflags::FlagSaver flag_saver; - FLAGS_vfs_periodic_flush_interval_ms = 60 * 60 * 1000; - - auto* writer = MakeOpenWriter(); - ASSERT_EQ(FileWriterTestPeer::RefCount(writer), 2); - - writer->Close(); - ASSERT_TRUE(write_background_executor_->Stop()); - EXPECT_EQ(FileWriterTestPeer::RefCount(writer), 1); - - writer->ReleaseRef(); -} - // 1. Write() for a simple in-chunk write succeeds and returns the correct // written size. TEST_F(FileWriterTest, Write_SingleChunk_CorrectSize) { @@ -417,20 +400,12 @@ TEST_F(FileWriterTest, Flush_AfterClose_ReturnsBadFd) { w->ReleaseRef(); } -TEST_F(FileWriterTest, PeriodicFlushErrorBecomesStickyAndBlocksWrites) { - gflags::FlagSaver flag_saver; - FLAGS_vfs_periodic_flush_interval_ms = 1; - - std::mutex mutex; - std::condition_variable cv; - bool write_slice_called = false; +// Background dirty flush reports failures and preserves sticky-error semantics. +TEST_F(FileWriterTest, FlushDirtyAsyncReportsErrorAndSticks) { + int write_slice_calls = 0; ON_CALL(*mock_meta_system_, WriteSlice) .WillByDefault([&](auto, auto, auto, auto, auto) { - { - std::lock_guard lock(mutex); - write_slice_called = true; - } - cv.notify_all(); + ++write_slice_calls; return Status::Internal("periodic flush failed"); }); @@ -439,18 +414,24 @@ TEST_F(FileWriterTest, PeriodicFlushErrorBecomesStickyAndBlocksWrites) { uint64_t wsize = 0; ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + std::mutex mutex; + std::condition_variable cv; + Status reported; + bool done = false; + w->FlushDirtyAsync([&](Status s) { + std::lock_guard lock(mutex); + reported = s; + done = true; + cv.notify_all(); + }); { std::unique_lock lock(mutex); - ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), - [&] { return write_slice_called; })); - } - - const auto deadline = - std::chrono::steady_clock::now() + std::chrono::seconds(5); - while (w->GetStatus().ok() && std::chrono::steady_clock::now() < deadline) { - std::this_thread::sleep_for(std::chrono::milliseconds(1)); + ASSERT_TRUE( + cv.wait_for(lock, std::chrono::seconds(5), [&] { return done; })); } - ASSERT_FALSE(w->GetStatus().ok()); + EXPECT_FALSE(reported.ok()); + EXPECT_EQ(write_slice_calls, 1); + ASSERT_FALSE(w->GetStatus().ok()) << "failure must be sticky"; wsize = 12345; Status s = w->Write(ctx_, buf, sizeof(buf), 4096, &wsize); @@ -461,6 +442,71 @@ TEST_F(FileWriterTest, PeriodicFlushErrorBecomesStickyAndBlocksWrites) { w->ReleaseRef(); } +// Empty and closed writers complete inline, exactly once. +TEST_F(FileWriterTest, FlushDirtyAsyncExactlyOnce_EmptyAndClosed) { + auto* empty_writer = MakeOpenWriter(211); + int calls = 0; + empty_writer->FlushDirtyAsync([&](Status s) { + ++calls; + EXPECT_TRUE(s.ok()) << "empty writer has nothing to flush"; + }); + EXPECT_EQ(calls, 1) << "empty writer completes inline"; + + auto* closed_writer = MakeOpenWriter(212); + closed_writer->Close(); + int closed_calls = 0; + closed_writer->FlushDirtyAsync([&](Status s) { + ++closed_calls; + EXPECT_TRUE(s.IsBadFd()) << s.ToString(); + }); + EXPECT_EQ(closed_calls, 1) << "closed writer reports BadFd exactly once"; + + empty_writer->ReleaseRef(); + closed_writer->ReleaseRef(); +} + +TEST_F(FileWriterTest, FlushDirtyAsyncSkipsCleanAndFlushesNewWrites) { + int write_slice_calls = 0; + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto, auto, auto, auto) { + ++write_slice_calls; + return Status::OK(); + }); + + auto* w = MakeOpenWriter(); + const auto caller = std::this_thread::get_id(); + const char buf[] = "dirty again"; + int expected_commits = 0; + // Exercise dirty -> clean -> dirty on the same writer with retained chunks. + for (uint64_t offset : {0u, 4096u}) { + uint64_t wsize = 0; + ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), offset, &wsize).ok()); + ASSERT_EQ(wsize, sizeof(buf)); + + auto flushed = std::make_shared>(); + auto flush_result = flushed->get_future(); + w->FlushDirtyAsync( + [flushed](Status status) { flushed->set_value(std::move(status)); }); + ASSERT_EQ(flush_result.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_TRUE(flush_result.get().ok()); + EXPECT_EQ(write_slice_calls, ++expected_commits); + + auto clean = std::make_shared>(); + auto clean_result = clean->get_future(); + w->FlushDirtyAsync([clean, caller](Status status) { + EXPECT_EQ(std::this_thread::get_id(), caller) + << "a clean writer must complete inline without a flush task"; + clean->set_value(std::move(status)); + }); + ASSERT_EQ(clean_result.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_TRUE(clean_result.get().ok()); + EXPECT_EQ(write_slice_calls, expected_commits); + } + FlushCloseAndRelease(w); +} + // A successful explicit Flush owns all writeback. Close must not submit a // second WriteSlice after metadata lifecycle code is free to close the session. TEST_F(FileWriterTest, Close_AfterFlush_DoesNotFlushAgain) { diff --git a/test/unit/client/vfs/data/test_maintenance_manager.cc b/test/unit/client/vfs/data/test_maintenance_manager.cc new file mode 100644 index 000000000..a14050af7 --- /dev/null +++ b/test/unit/client/vfs/data/test_maintenance_manager.cc @@ -0,0 +1,996 @@ +/* + * Copyright (c) 2026 dingodb.com, Inc. All Rights Reserved + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "absl/hash/hash.h" +#include "client/vfs/components/maintenance_manager.h" +#include "client/vfs/data/reader/file_reader.h" +#include "client/vfs/data/reader/reader_registry.h" +#include "client/vfs/data/reader/reader_registry_task.h" +#include "client/vfs/data/writer/file_writer.h" +#include "client/vfs/data/writer_table.h" +#include "client/vfs/data/writer_table_task.h" +#include "client/vfs/data_buffer.h" +#include "common/options/client.h" +#include "common/readmempool/read_mem_pool.h" +#include "common/sync_point.h" +#include "test/unit/client/vfs/test_base.h" +#include "utils/executor/executor.h" +#include "utils/scoped_cleanup.h" + +namespace dingofs { +namespace client { +namespace vfs { + +using dingofs::client::vfs::test::VFSTestBase; +using ::testing::_; +using ::testing::DoAll; +using ::testing::Return; +using ::testing::SetArgPointee; + +// White-box peer (the friend declaration in file_reader.h names this class): +// lets tests age cached requests deterministically and observe the request +// set shrinking after a maintenance tick instead of guessing timing. +class ReaderRegistryTaskTestPeer { + public: + static size_t RequestCount(FileReader* reader) { + std::lock_guard lock(reader->mutex_); + return reader->requests_.size(); + } + + static int64_t RefCount(FileReader* reader) { + return reader->refs_.load(std::memory_order_acquire); + } + + // Pretend every cached request has been idle for an hour so ShrinkIfOpen's + // reclaim conditions are met without sleeping. + static void AgeAllRequests(FileReader* reader) { + const int64_t old = butil::monotonic_time_s() - 3600; + std::lock_guard lock(reader->mutex_); + for (auto& [id, req] : reader->requests_) { + std::lock_guard req_lock(req->mutex); + req->access_sec = old; + } + } +}; + +namespace { + +bool WaitFor(std::function pred, + std::chrono::milliseconds timeout = std::chrono::seconds(10)) { + const auto deadline = std::chrono::steady_clock::now() + timeout; + while (std::chrono::steady_clock::now() < deadline) { + if (pred()) return true; + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + return pred(); +} + +// Deterministically collect `count` inos that route to `shard` under the +// registries' 64-way sharding, starting the search at `base`. +std::vector InosInShard(size_t shard, size_t count, uint64_t base) { + std::vector inos; + for (uint64_t ino = base; inos.size() < count; ++ino) { + if ((absl::HashOf(ino) & 63u) == shard) inos.push_back(ino); + } + CHECK_EQ(inos.size(), count); + return inos; +} + +// Executor double for Start-rejection tests: Schedule either rejects or +// queues into a deque the test executes by hand, so a "late" wakeup really +// runs against the drained control state. +class ManualScheduleExecutor final : public Executor { + public: + bool Start() override { + std::lock_guard lock(mutex_); + running_ = true; + return true; + } + bool Stop() override { + std::lock_guard lock(mutex_); + running_ = false; + return true; + } + bool Execute(std::function func) override { + std::lock_guard lock(mutex_); + if (!running_) return false; + tasks_.push_back(std::move(func)); + return true; + } + bool Schedule(std::function func, int /*delay_ms*/) override { + if (reject_schedule_) return false; + return Execute(std::move(func)); + } + int ThreadNum() const override { return 1; } + int TaskNum() const override { + std::lock_guard lock(mutex_); + return static_cast(tasks_.size()); + } + std::string Name() const override { return "manual_schedule"; } + + void SetRejectSchedule(bool reject) { reject_schedule_ = reject; } + + // Actually executes one queued closure (a "late" wakeup that really runs). + void RunOne() { + std::function task; + { + std::lock_guard lock(mutex_); + CHECK(!tasks_.empty()); + task = std::move(tasks_.front()); + tasks_.pop_front(); + } + task(); + } + + private: + mutable std::mutex mutex_; + bool running_{false}; + bool reject_schedule_{false}; + std::deque> tasks_; +}; + +} // namespace + +static std::unique_ptr MakeVfsMaintenance( + ReaderRegistry* readers, WriterTable* writers, Executor* read_executor, + Executor* write_executor, Executor* cleanup_executor, int interval_ms = 1) { + auto manager = std::make_unique(); + CHECK(manager + ->RegisterTask("reader-shrink", + std::make_shared(readers), + read_executor, interval_ms) + .ok()); + CHECK(manager + ->RegisterTask( + "writer-flush", + std::make_shared(writers, cleanup_executor), + write_executor, interval_ms) + .ok()); + return manager; +} + +class MaintenanceManagerTest : public VFSTestBase { + protected: + void SetUp() override { + VFSTestBase::SetUp(); + + maintenance_ = MakeVfsMaintenance( + reader_registry_.get(), writer_table_.get(), + read_cleanup_executor_.get(), write_background_executor_.get(), + cleanup_executor_.get()); + } + + void TearDown() override { + if (maintenance_) { + maintenance_->StopAndDrain(); + maintenance_.reset(); + } + VFSTestBase::TearDown(); + } + + std::unique_ptr maintenance_; +}; + +// Registration validates each task's interval without poisoning the manager. +TEST_F(MaintenanceManagerTest, RegistrationRejectsNonPositiveInterval) { + MaintenanceManager manager; + auto task = std::make_shared(reader_registry_.get()); + EXPECT_TRUE( + manager.RegisterTask("zero", task, read_cleanup_executor_.get(), 0) + .IsInvalidParam()); + EXPECT_TRUE( + manager.RegisterTask("negative", task, read_cleanup_executor_.get(), -1) + .IsInvalidParam()); + EXPECT_TRUE( + manager.RegisterTask("valid", task, read_cleanup_executor_.get(), 1) + .ok()); +} + +// A rejected FIRST wakeup submission is a startup failure, not a process +// abort: Start returns non-OK and the component rolls back without hanging. +TEST_F(MaintenanceManagerTest, StartFailsWhenFirstWakeupRejected) { + gflags::FlagSaver flag_saver; + + ManualScheduleExecutor read_executor; + ManualScheduleExecutor write_executor; + ASSERT_TRUE(read_executor.Start()); + ASSERT_TRUE(write_executor.Start()); + read_executor.SetRejectSchedule(true); + + maintenance_ = MakeVfsMaintenance(reader_registry_.get(), writer_table_.get(), + &read_executor, &write_executor, + cleanup_executor_.get()); + + Status s = maintenance_->Start(); + ASSERT_FALSE(s.ok()); + EXPECT_TRUE(s.IsInternal()) << s.ToString(); + + // Rollback is clean: drain returns promptly, nothing was accepted. + maintenance_->StopAndDrain(); + EXPECT_EQ(read_executor.TaskNum(), 0); + EXPECT_EQ(write_executor.TaskNum(), 0); + maintenance_.reset(); +} + +// When the SECOND wakeup submission is rejected, the already-accepted first +// timer must be neutralized by the rollback: after StopAndDrain the late +// wakeup REALLY RUNS (executed by hand below) and only observes the stopped +// state without touching any dependency. +TEST_F(MaintenanceManagerTest, StartRollsBackAcceptedWakeupWhenSecondRejected) { + gflags::FlagSaver flag_saver; + + ManualScheduleExecutor read_executor; // accepts, queues for manual run + ManualScheduleExecutor write_executor; // rejects + ASSERT_TRUE(read_executor.Start()); + ASSERT_TRUE(write_executor.Start()); + write_executor.SetRejectSchedule(true); + + maintenance_ = MakeVfsMaintenance(reader_registry_.get(), writer_table_.get(), + &read_executor, &write_executor, + cleanup_executor_.get()); + + Status s = maintenance_->Start(); + ASSERT_FALSE(s.ok()); + EXPECT_TRUE(s.IsInternal()) << s.ToString(); + + // The reader wakeup was accepted before the failure; the rollback closed + // admission. Execute it for real now (the "late timer" case): it must + // observe stopped and return without touching the cleared dependencies + // and without enqueuing anything. + maintenance_.reset(); + ASSERT_EQ(read_executor.TaskNum(), 1); + read_executor.RunOne(); + EXPECT_EQ(read_executor.TaskNum(), 0) + << "the late wakeup must not re-arm or submit anything"; + + EXPECT_EQ(reader_registry_->Size(), 0u); + EXPECT_EQ(writer_table_->Size(), 0u); +} + +TEST_F(MaintenanceManagerTest, LateWakeupsDoNotResurrectDestroyedOwner) { + gflags::FlagSaver flag_saver; + ManualScheduleExecutor read_executor; + ManualScheduleExecutor write_executor; + ASSERT_TRUE(read_executor.Start()); + ASSERT_TRUE(write_executor.Start()); + maintenance_ = MakeVfsMaintenance(reader_registry_.get(), writer_table_.get(), + &read_executor, &write_executor, + cleanup_executor_.get()); + ASSERT_TRUE(maintenance_->Start().ok()); + EXPECT_FALSE(maintenance_->Start().ok()); + maintenance_->StopAndDrain(); + EXPECT_FALSE(maintenance_->Start().ok()); + maintenance_.reset(); + + ASSERT_EQ(read_executor.TaskNum(), 1); + ASSERT_EQ(write_executor.TaskNum(), 1); + read_executor.RunOne(); + write_executor.RunOne(); + EXPECT_EQ(read_executor.TaskNum(), 0); + EXPECT_EQ(write_executor.TaskNum(), 0); +} + +TEST_F(MaintenanceManagerTest, StopBeforeStartClosesAdmissionPermanently) { + maintenance_->StopAndDrain(); + maintenance_->StopAndDrain(); + EXPECT_FALSE(maintenance_->Start().ok()); +} + +// The unified maintenance tick must flush dirty writers without any explicit +// Flush and without per-object periodic tasks. +TEST_F(MaintenanceManagerTest, TickFlushesDirtyWriterWithoutExplicitFlush) { + gflags::FlagSaver flag_saver; + + std::mutex mutex; + std::condition_variable cv; + int write_slice_calls = 0; + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto, auto, auto, auto) { + { + std::lock_guard lock(mutex); + ++write_slice_calls; + } + cv.notify_all(); + return Status::OK(); + }); + + FileWriter* writer = writer_table_->AcquireWriter(700); + ASSERT_NE(writer, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(writer->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + ASSERT_TRUE(maintenance_->Start().ok()); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), + [&] { return write_slice_calls >= 1; })); + } + // The tick flushed the dirty data; the writer is clean and error-free. + EXPECT_TRUE(writer->GetStatus().ok()); + + maintenance_->StopAndDrain(); + maintenance_.reset(); + + writer_table_->ReleaseWriter(writer); + EXPECT_EQ(writer_table_->Size(), 0u); +} + +// A round must walk through empty shards and reach objects that do not live +// in shard 0. Pre-fix, an empty shard snapshot was rebuilt forever and the +// scan never advanced past it. +TEST_F(MaintenanceManagerTest, TickFlushesWriterBeyondShardZero) { + gflags::FlagSaver flag_saver; + + const uint64_t ino = InosInShard(/*shard=*/7, /*count=*/1, /*base=*/900)[0]; + + std::mutex mutex; + std::condition_variable cv; + int write_slice_calls = 0; + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto ino_called, auto, auto, auto) { + { + std::lock_guard lock(mutex); + if (ino_called == ino) ++write_slice_calls; + } + cv.notify_all(); + return Status::OK(); + }); + + FileWriter* writer = writer_table_->AcquireWriter(ino); + ASSERT_NE(writer, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(writer->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + ASSERT_TRUE(maintenance_->Start().ok()); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), [&] { + return write_slice_calls >= 1; + })) << "scan must skip the empty shards 0..6 and reach shard 7"; + } + EXPECT_TRUE(writer->GetStatus().ok()); + + maintenance_->StopAndDrain(); + maintenance_.reset(); + + writer_table_->ReleaseWriter(writer); + EXPECT_EQ(writer_table_->Size(), 0u); +} + +// The reader stream of the maintenance tick must reclaim cached read +// requests that satisfy the existing shrink conditions (aged out under +// read-pool pressure) and must skip closed readers safely. The reader lives +// beyond shard 0, covering empty-shard advance on the reader side too. +TEST_F(MaintenanceManagerTest, TickShrinksAgedReadaheadRequests) { + gflags::FlagSaver flag_saver; + // Suppress speculative readahead entirely: with the tiny pool below it + // could not allocate anyway, and the aged-request reclaim must not depend + // on it. + FLAGS_vfs_read_mempool_readahead_watermark = 0.0; + + // A non-zero shard: the scan must pass the empty shards before it. + const Ino kReaderIno = + static_cast(InosInShard(/*shard=*/9, /*count=*/1, /*base=*/950)[0]); + const Attr attr = test::MakeFileAttr(kReaderIno, 4 * 1024 * 1024); + ON_CALL(*mock_meta_system_, GetAttr(_, kReaderIno, _)) + .WillByDefault(DoAll(SetArgPointee<2>(attr), Return(Status::OK()))); + // One non-hole slice covering the whole file so reads consult the + // (zero-filling) mock block store instead of zero-filling holes. + ON_CALL(*mock_meta_system_, ReadSlice) + .WillByDefault([](ContextSPtr, Ino, uint64_t, uint64_t, + std::vector* slices, uint64_t& version) { + slices->clear(); + Slice slice; + slice.id = 1; + slice.pos = 0; + slice.size = 4 * 1024 * 1024; + slice.off = 0; + slice.len = slice.size; + slices->push_back(slice); + version = 1; + return Status::OK(); + }); + + FileReader* reader = new FileReader(mock_hub_, 11, kReaderIno); + reader->AcquireRef(); + reader_registry_->Register(reader); + auto cleanup = MakeScopedCleanup([&] { + maintenance_->StopAndDrain(); + reader_registry_->Unregister(reader); + reader->Close(); + reader->ReleaseRef(); + }); + + auto read_4k = [&](int64_t offset) { + DataBuffer buffer; + uint64_t rsize = 0; + EXPECT_TRUE(reader->Read(ctx_, &buffer, 4096, offset, &rsize).ok()); + EXPECT_EQ(rsize, 4096u); + }; + read_4k(0); + read_4k(4096); + ASSERT_EQ(ReaderRegistryTaskTestPeer::RequestCount(reader), 2u) + << "both reads must be served from cached requests"; + + // Meet the shrink conditions deterministically: pool usage is high and the + // cached requests count as idle. + ReaderRegistryTaskTestPeer::AgeAllRequests(reader); + + ASSERT_TRUE(maintenance_->Start().ok()); + EXPECT_TRUE(WaitFor( + [&] { return ReaderRegistryTaskTestPeer::RequestCount(reader) == 0; }, + std::chrono::seconds(5))) + << "maintenance tick must reclaim the aged-out cached requests"; + + // Close WITHOUT unregistering: the registered-but-closed reader is the + // concurrent-Close race the maintenance snapshot must handle. Subsequent + // ticks (and StopAndDrain's own step handoff) call ShrinkIfOpen on it; + // the closing flag makes that a safe no-op. The scoped cleanup performs + // the real Unregister afterwards. + reader->Close(); + EXPECT_EQ(reader_registry_->Size(), 1u); +} + +// A slow writer must not block periodic maintenance for the other writers, +// and the slow writer itself must never receive a second overlapping +// periodic flush while its first one is in flight. +TEST_F(MaintenanceManagerTest, + SlowWriterDoesNotBlockOthersAndIsNotResubmitted) { + gflags::FlagSaver flag_saver; + + constexpr uint64_t kSlowIno = 800; + constexpr int kWriters = 8; + + std::mutex mutex; + std::condition_variable cv; + std::condition_variable gate_cv; + bool slow_upload_entered = false; + bool allow_slow_upload = false; + std::unordered_map writeslice_per_ino; +#ifndef NDEBUG + std::unordered_map periodic_calls; +#endif + ON_CALL(*mock_block_store_, PutAsync) + .WillByDefault([&](ContextSPtr, PutReq, StatusCallback cb) { + { + std::unique_lock lock(mutex); + // Gate exactly one upload (the "slow" writer, whichever flushed + // first); every other upload completes inline. + if (!slow_upload_entered) { + slow_upload_entered = true; + gate_cv.wait(lock, [&] { return allow_slow_upload; }); + } + } + cb(Status::OK()); + }); + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto ino, auto, auto, auto) { + std::lock_guard lock(mutex); + ++writeslice_per_ino[ino]; + cv.notify_all(); + return Status::OK(); + }); + + std::vector writers; + for (int i = 0; i < kWriters; ++i) { + FileWriter* w = writer_table_->AcquireWriter(kSlowIno + i); + ASSERT_NE(w, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + writers.push_back(w); + } + auto cleanup = MakeScopedCleanup([&] { + { + std::lock_guard lock(mutex); + allow_slow_upload = true; + } + gate_cv.notify_all(); + maintenance_->StopAndDrain(); +#ifndef NDEBUG + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); +#endif + for (auto* w : writers) { + EXPECT_TRUE(w->Flush().ok()); + writer_table_->ReleaseWriter(w); + } + }); +#ifndef NDEBUG + SyncPoint::GetInstance()->SetCallBack( + "WriterTableTask:before_flush", [&](void* arg) { + auto* writer = static_cast(arg); + std::lock_guard lock(mutex); + ++periodic_calls[writer->Ino()]; + }); + SyncPoint::GetInstance()->EnableProcessing(); +#endif + + ASSERT_TRUE(maintenance_->Start().ok()); + + // While the slow upload is blocked, every other writer must still get its + // periodic flush. + EXPECT_TRUE(WaitFor( + [&] { + std::lock_guard lock(mutex); + int flushed = 0; + for (auto& [ino, n] : writeslice_per_ino) flushed += (n > 0) ? 1 : 0; + return flushed >= kWriters - 1; + }, + std::chrono::seconds(5))) + << "a slow writer must not block maintenance for the other writers"; + + { + std::lock_guard lock(mutex); + for (auto& [ino, n] : writeslice_per_ino) { + EXPECT_LE(n, 1) << "writer " << ino << " submitted " << n << " flushes"; + } + } + +#ifndef NDEBUG + // A third visit to any fast writer proves another complete scan has + // passed the still-blocked writer, not merely that its commit is pending. + EXPECT_TRUE(WaitFor([&] { + std::lock_guard lock(mutex); + for (const auto& [ino, count] : periodic_calls) { + if (count >= 3) return true; + } + return false; + })); + { + std::lock_guard lock(mutex); + for (auto* writer : writers) { + if (writeslice_per_ino.count(writer->Ino()) == 0) { + EXPECT_EQ(periodic_calls[writer->Ino()], 1); + } + } + } +#endif + + // Release the slow upload; the last writer completes too. + { + std::lock_guard lock(mutex); + allow_slow_upload = true; + } + gate_cv.notify_all(); + + EXPECT_TRUE(WaitFor( + [&] { + std::lock_guard lock(mutex); + int flushed = 0; + for (auto& [ino, n] : writeslice_per_ino) flushed += (n > 0) ? 1 : 0; + return flushed == kWriters; + }, + std::chrono::seconds(5))); + + // The original dirty slices are committed once. Later clean maintenance + // visits must not manufacture duplicate slice commits. + { + std::lock_guard lock(mutex); + EXPECT_EQ(writeslice_per_ino.size(), static_cast(kWriters)); + for (auto& [ino, n] : writeslice_per_ino) { + EXPECT_EQ(n, 1) << "writer " << ino << " must not be flushed twice"; + } + } +} + +// Stop must not wait for an armed-but-unfired wakeup: StopAndDrain returns +// immediately, and the later executor teardown destroys the pending closure +// (which holds only shared control state) without running it. +TEST_F(MaintenanceManagerTest, StopWithArmedWakeupReturnsImmediately) { + gflags::FlagSaver flag_saver; + maintenance_ = MakeVfsMaintenance(reader_registry_.get(), writer_table_.get(), + read_cleanup_executor_.get(), + write_background_executor_.get(), + cleanup_executor_.get(), 60 * 60 * 1000); + + ASSERT_TRUE(maintenance_->Start().ok()); + + const auto begin = std::chrono::steady_clock::now(); + maintenance_->StopAndDrain(); + const auto elapsed = std::chrono::steady_clock::now() - begin; + EXPECT_LT(elapsed, std::chrono::seconds(1)) + << "StopAndDrain must not wait for the pending hour-long wakeup"; + + maintenance_.reset(); + // Fixture teardown stops the executors (destroying the pending timer + // closures) and destroys the registry/table afterwards; any use-after-free + // here fails under ASAN/TSAN. +} + +// Stop during an in-flight periodic flush must wait for the flush callback +// AND the actual holder release on the cleanup executor -- not just for the +// callback to return. +TEST_F(MaintenanceManagerTest, StopWaitsForInflightFlushHolderRelease) { + gflags::FlagSaver flag_saver; + + std::mutex mutex; + std::condition_variable cv; + std::condition_variable gate_cv; + bool upload_entered = false; + bool allow_upload = false; + ON_CALL(*mock_block_store_, PutAsync) + .WillByDefault([&](ContextSPtr, PutReq, StatusCallback cb) { + { + std::unique_lock lock(mutex); + upload_entered = true; + cv.notify_all(); + gate_cv.wait(lock, [&] { return allow_upload; }); + } + cb(Status::OK()); + }); + + FileWriter* writer = writer_table_->AcquireWriter(820); + ASSERT_NE(writer, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(writer->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + ASSERT_TRUE(maintenance_->Start().ok()); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), + [&] { return upload_entered; })); + } + + // Drop the external holder: the maintenance's snapshot holder is the only + // thing keeping the entry alive now. + writer_table_->ReleaseWriter(writer); + EXPECT_EQ(writer_table_->Size(), 1u); + + auto stopped = std::async(std::launch::async, [&] { + maintenance_->StopAndDrain(); + return true; + }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(100)), + std::future_status::timeout) + << "StopAndDrain must wait for the in-flight flush and its cleanup"; + + { + std::lock_guard lock(mutex); + allow_upload = true; + } + gate_cv.notify_all(); + + ASSERT_EQ(stopped.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_TRUE(stopped.get()); + + // The cleanup executor actually released the last holder: the entry is + // gone and the writer was closed by the maintenance stream. + EXPECT_EQ(writer_table_->Size(), 0u); +} + +// When every maintenance slot is busy mid-shard, the scan pauses. The +// unconsumed tail (the 65th writer of the SAME shard snapshot) must be +// cancelled by Stop -- handed to ONE cleanup task, not flushed, not +// double-released: after the drain, every external holder still evicts its +// entry exactly once. +TEST_F(MaintenanceManagerTest, StopWhilePausedCancelsTailWithoutDoubleRelease) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic pause staging requires TEST_SYNC_POINT."; +#else + gflags::FlagSaver flag_saver; + + constexpr int kMaintenanceSlots = 64; + constexpr int kWriters = kMaintenanceSlots + 1; + + std::mutex mutex; + std::condition_variable cv; + std::condition_variable gate_cv; + bool allow_uploads = false; + int put_async_entered = 0; + int write_slice_calls = 0; + bool scan_paused = false; + ON_CALL(*mock_block_store_, PutAsync) + .WillByDefault([&](ContextSPtr, PutReq, StatusCallback cb) { + { + std::unique_lock lock(mutex); + ++put_async_entered; + cv.notify_all(); + gate_cv.wait(lock, [&] { return allow_uploads; }); + } + cb(Status::OK()); + }); + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto, auto, auto, auto) { + std::lock_guard lock(mutex); + ++write_slice_calls; + cv.notify_all(); + return Status::OK(); + }); + + SyncPoint::GetInstance()->SetCallBack("WriterTableTask:paused", [&](void*) { + std::lock_guard lock(mutex); + scan_paused = true; + cv.notify_all(); + }); + SyncPoint::GetInstance()->EnableProcessing(); + auto disable_syncpoint = MakeScopedCleanup([] { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + + // All writers in ONE shard: batch 1 consumes 64, the 65th stays in the + // same snapshot as the unconsumed tail. + const auto inos = InosInShard(/*shard=*/5, kWriters, /*base=*/1000); + std::vector writers; + for (uint64_t ino : inos) { + FileWriter* w = writer_table_->AcquireWriter(ino); + ASSERT_NE(w, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + writers.push_back(w); + } + ASSERT_TRUE(maintenance_->Start().ok()); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(10), [&] { + return scan_paused && put_async_entered >= 1; + })) << "scan must pause once all maintenance slots are busy"; + } + + auto stopped = std::async(std::launch::async, [&] { + maintenance_->StopAndDrain(); + return true; + }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(100)), + std::future_status::timeout) + << "StopAndDrain still owes the in-flight member cleanups"; + + { + std::lock_guard lock(mutex); + allow_uploads = true; + } + gate_cv.notify_all(); + + ASSERT_EQ(stopped.wait_for(std::chrono::seconds(10)), + std::future_status::ready); + EXPECT_TRUE(stopped.get()); + + { + std::lock_guard lock(mutex); + // Exactly the slot-count worth of members were submitted and flushed; + // the paused tail was cancelled, not flushed by maintenance. + EXPECT_EQ(write_slice_calls, kMaintenanceSlots); + } + + // Every pin was returned exactly once: the 64 consumed members' holders + // by their cleanup tasks, the tail holder by the cancel task. Releasing + // the external holders now must evict each entry cleanly -- a double + // release of any snapshot entry would have CHECK-failed or left the + // accounting broken (and Flush of an already-closed writer would fail). + for (size_t i = 0; i < writers.size(); ++i) { + ASSERT_TRUE(writers[i]->Flush().ok()) << "writer " << inos[i]; + } + EXPECT_EQ(writer_table_->Size(), static_cast(kWriters)); + for (auto* w : writers) { + writer_table_->ReleaseWriter(w); + } + EXPECT_EQ(writer_table_->Size(), 0u) + << "every entry must evict exactly once after the drain"; +#endif +} + +// The pause/resume boundary must not lose wakeups: releasing the gated +// uploads AT the pause point (the sync callback runs right after the atomic +// pause retreat) must let the returned slots resume the scan, and the 65th +// object of the shard must still be processed. +TEST_F(MaintenanceManagerTest, PausedScanResumesWhenSlotsReturn) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic pause staging requires TEST_SYNC_POINT."; +#else + gflags::FlagSaver flag_saver; + + constexpr int kMaintenanceSlots = 64; + constexpr int kWriters = kMaintenanceSlots + 1; + + std::mutex mutex; + std::condition_variable cv; + std::condition_variable gate_cv; + bool allow_uploads = false; + int put_async_entered = 0; + std::unordered_map writeslice_per_ino; + ON_CALL(*mock_block_store_, PutAsync) + .WillByDefault([&](ContextSPtr, PutReq, StatusCallback cb) { + { + std::unique_lock lock(mutex); + ++put_async_entered; + cv.notify_all(); + gate_cv.wait(lock, [&] { return allow_uploads; }); + } + cb(Status::OK()); + }); + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto ino, auto, auto, auto) { + std::lock_guard lock(mutex); + ++writeslice_per_ino[ino]; + cv.notify_all(); + return Status::OK(); + }); + + const auto inos = InosInShard(/*shard=*/6, kWriters, /*base=*/2000); + std::vector writers; + for (uint64_t ino : inos) { + FileWriter* w = writer_table_->AcquireWriter(ino); + ASSERT_NE(w, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + writers.push_back(w); + } + auto cleanup = MakeScopedCleanup([&] { + { + std::lock_guard lock(mutex); + allow_uploads = true; + } + gate_cv.notify_all(); + maintenance_->StopAndDrain(); + for (auto* w : writers) { + EXPECT_TRUE(w->Flush().ok()); + writer_table_->ReleaseWriter(w); + } + }); + + // Release the upload gate exactly when the scan has committed its atomic + // pause: the returning slots race the pause as tightly as possible. + SyncPoint::GetInstance()->SetCallBack("WriterTableTask:paused", [&](void*) { + std::lock_guard lock(mutex); + allow_uploads = true; + gate_cv.notify_all(); + }); + SyncPoint::GetInstance()->EnableProcessing(); + auto disable_syncpoint = MakeScopedCleanup([] { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + + ASSERT_TRUE(maintenance_->Start().ok()); + gate_cv.notify_all(); // wake any upload that entered before the gate opened + + // The paused scan must resume from its position: ALL kWriters (including + // the 65th tail object) get flushed, each exactly once. + EXPECT_TRUE(WaitFor( + [&] { + std::lock_guard lock(mutex); + return writeslice_per_ino.size() == kWriters; + }, + std::chrono::seconds(10))) + << "the tail object must still be processed after the pause"; + { + std::lock_guard lock(mutex); + for (auto& [ino, n] : writeslice_per_ino) { + EXPECT_EQ(n, 1) << "writer " << ino << " must not be flushed twice"; + } + } + + maintenance_->StopAndDrain(); +#endif +} + +// Reader-side tail accounting: stopping mid-shard (the step is gated between +// two batches) must cancel only the unconsumed tail. The 64 readers of the +// consumed prefix were already released; releasing them again would +// underflow their refcounts. Every reader must still be alive with exactly +// its owner reference after the drain. +TEST_F(MaintenanceManagerTest, StopMidShardReleasesOnlyUnconsumedReaderTail) { +#ifdef NDEBUG + GTEST_SKIP() << "Deterministic mid-shard staging requires TEST_SYNC_POINT."; +#else + gflags::FlagSaver flag_saver; + + constexpr int kMaintenanceSlots = 64; + constexpr int kReaders = kMaintenanceSlots + 1; + + // Gate the FIRST reader batch: the step blocks right after processing 64 + // readers, with the 65th still unconsumed in the same shard snapshot. + std::mutex gate_mutex; + std::condition_variable gate_cv; + bool batch_done = false; + bool allow_step = false; + SyncPoint::GetInstance()->SetCallBack( + "ReaderRegistryTask:after_batch", [&](void*) { + std::unique_lock lock(gate_mutex); + if (!batch_done) { + batch_done = true; + gate_cv.notify_all(); + gate_cv.wait(lock, [&] { return allow_step; }); + } + }); + SyncPoint::GetInstance()->EnableProcessing(); + auto disable_syncpoint = MakeScopedCleanup([] { + SyncPoint::GetInstance()->DisableProcessing(); + SyncPoint::GetInstance()->ClearAllCallBacks(); + }); + + const auto inos = InosInShard(/*shard=*/11, kReaders, /*base=*/3000); + std::vector readers; + for (uint64_t ino : inos) { + auto* reader = new FileReader(mock_hub_, /*fh=*/ino, ino); + reader->AcquireRef(); + reader_registry_->Register(reader); + readers.push_back(reader); + } + auto cleanup = MakeScopedCleanup([&] { + for (auto* reader : readers) { + reader_registry_->Unregister(reader); + reader->Close(); + reader->ReleaseRef(); + } + }); + + ASSERT_TRUE(maintenance_->Start().ok()); + { + std::unique_lock lock(gate_mutex); + ASSERT_TRUE(gate_cv.wait_for(lock, std::chrono::seconds(10), + [&] { return batch_done; })); + } + + // Stop while the step is parked mid-shard: it owes the tail cancellation. + auto stopped = std::async(std::launch::async, [&] { + maintenance_->StopAndDrain(); + return true; + }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(100)), + std::future_status::timeout) + << "StopAndDrain must wait for the parked step"; + + { + std::lock_guard lock(gate_mutex); + allow_step = true; + } + gate_cv.notify_all(); + + ASSERT_EQ(stopped.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_TRUE(stopped.get()); + + // Every reader must hold EXACTLY its owner reference: the 64 consumed + // prefix refs were released once by the scan, the tail ref once by the + // cancel. A double release would have CHECK-failed in ReleaseRef (or + // destroyed the object, making this refcount read crash). + for (size_t i = 0; i < readers.size(); ++i) { + EXPECT_EQ(ReaderRegistryTaskTestPeer::RefCount(readers[i]), 1) + << "reader " << inos[i] + << " must hold exactly its owner reference after the drain"; + } +#endif +} + +} // namespace vfs +} // namespace client +} // namespace dingofs diff --git a/test/unit/client/vfs/data/test_writer_table.cc b/test/unit/client/vfs/data/test_writer_table.cc index 13b0bcc0c..c2a65df0e 100644 --- a/test/unit/client/vfs/data/test_writer_table.cc +++ b/test/unit/client/vfs/data/test_writer_table.cc @@ -27,11 +27,11 @@ #include #include +#include "absl/hash/hash.h" #include "client/vfs/data/write_pressure_controller.h" #include "client/vfs/data/writer/file_writer.h" #include "client/vfs/data/writer_table.h" #include "test/unit/client/vfs/test_base.h" -#include "utils/executor/thread/executor_impl.h" #include "utils/scoped_cleanup.h" namespace dingofs { @@ -210,6 +210,10 @@ TEST_F(WriterTableTest, PressureFlushReturnsPagesAndUnblocksFifoWriter) { ExecutorImpl pressure_executor("test_write_pressure", 1); ASSERT_TRUE(pressure_executor.Start()); + ExecutorImpl cleanup_executor("test_cleanup_pressure", 1); + ASSERT_TRUE(cleanup_executor.Start()); + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(&cleanup_executor)); WritePressureController controller(table_.get(), &pressure_executor); tiny_pool.SetPressureObserver(&controller); @@ -239,13 +243,12 @@ TEST_F(WriterTableTest, PressureFlushReturnsPagesAndUnblocksFifoWriter) { auto [status, written] = blocked_write.get(); EXPECT_TRUE(status.ok()) << status.ToString(); EXPECT_EQ(written, second_buf.size()); - ASSERT_TRUE(table_->FlushAll().ok()); tiny_pool.Close(); tiny_pool.SetPressureObserver(nullptr); controller.StopAndDrain(); ASSERT_TRUE(pressure_executor.Stop()); - + ASSERT_TRUE(cleanup_executor.Stop()); table_->ReleaseWriter(first); table_->ReleaseWriter(second); EXPECT_EQ(tiny_pool.GetUsedBytes(), 0); @@ -264,6 +267,10 @@ TEST_F(WriterTableTest, PressureFlushSeesPartialChunkBeforeNextAdmission) { ExecutorImpl pressure_executor("test_write_pressure_cross_chunk", 1); ASSERT_TRUE(pressure_executor.Start()); + ExecutorImpl cleanup_executor("test_cleanup_cross_chunk", 1); + ASSERT_TRUE(cleanup_executor.Start()); + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(&cleanup_executor)); WritePressureController controller(table_.get(), &pressure_executor); tiny_pool.SetPressureObserver(&controller); @@ -292,14 +299,13 @@ TEST_F(WriterTableTest, PressureFlushSeesPartialChunkBeforeNextAdmission) { auto [status, written] = write.get(); EXPECT_TRUE(status.ok()) << status.ToString(); EXPECT_EQ(written, buf.size()); - - EXPECT_TRUE(writer->Flush().ok()); - EXPECT_EQ(tiny_pool.GetUsedBytes(), 0); + EXPECT_TRUE(table_->FlushAll().ok()); tiny_pool.Close(); tiny_pool.SetPressureObserver(nullptr); controller.StopAndDrain(); EXPECT_TRUE(pressure_executor.Stop()); + EXPECT_TRUE(cleanup_executor.Stop()); table_->ReleaseWriter(writer); } @@ -395,6 +401,282 @@ TEST_F(WriterTableTest, ReleaseAfterStop_StillEvicts) { << "ReleaseWriter after Stop must still return the writer"; } +// --- CleanupExecutor regression tests -------------------------------------- +// +// The single-worker chain below mirrors the production topology the +// CleanupExecutor design calls out: a pressure round flushes a dirty writer +// whose member completion runs on the (single-worker) CBExecutor. Before the +// cleanup isolation, that completion synchronously released the last holder, +// Closed the writer and entered ChunkWriter::Stop's DoSyncFlush wait for a +// notification that could only be posted by the same occupied worker: +// deadlock without any backend failure. Now the member callback only hands +// the holder to the independent cleanup executor and returns. +TEST_F(WriterTableTest, PressureRoundLastHolderCleanupOnSingleWorkerCb) { + std::mutex mutex; + std::condition_variable cv; + std::condition_variable gate_cv; + bool upload_entered = false; + bool allow_upload = false; + ON_CALL(*mock_block_store_, PutAsync) + .WillByDefault([&](ContextSPtr, PutReq, StatusCallback cb) { + { + std::unique_lock lock(mutex); + upload_entered = true; + cv.notify_all(); + gate_cv.wait(lock, [&] { return allow_upload; }); + } + cb(Status::OK()); + }); + + ExecutorImpl pressure_executor("test_pressure_single_cb", 1); + ASSERT_TRUE(pressure_executor.Start()); + ExecutorImpl cleanup_executor("test_cleanup_single_cb", 1); + ASSERT_TRUE(cleanup_executor.Start()); + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(&cleanup_executor)); + WritePressureController controller(table_.get(), &pressure_executor); + auto drain = MakeScopedCleanup([&] { + controller.StopAndDrain(); + pressure_executor.Stop(); + cleanup_executor.Stop(); + }); + + FileWriter* writer = table_->AcquireWriter(600); + ASSERT_NE(writer, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(writer->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + controller.OnWritePressure(); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), + [&] { return upload_entered; })); + } + + // Drop the only external holder while the round's flush is in flight: the + // round's snapshot holder becomes the last holder, so the cleanup task's + // ReleaseWriter will Close the writer. + table_->ReleaseWriter(writer); + EXPECT_EQ(table_->Size(), 1u); + + { + std::lock_guard lock(mutex); + allow_upload = true; + } + gate_cv.notify_all(); + + // The whole chain (upload -> CB member callback -> cleanup task -> last + // holder release -> Close) must complete: pre-fix this deadlocks the + // single CB worker, post-fix the round retires and the entry is gone. + const auto deadline = + std::chrono::steady_clock::now() + std::chrono::seconds(5); + while (table_->Size() != 0 && std::chrono::steady_clock::now() < deadline) { + std::this_thread::sleep_for(std::chrono::milliseconds(1)); + } + ASSERT_EQ(table_->Size(), 0u) + << "last-holder cleanup must complete on the cleanup executor"; + + controller.StopAndDrain(); +} + +// StopAndDrain must not return while a member cleanup task is still queued +// on the cleanup executor: round completion is defined by the actual holder +// release, not by the flush callback's return. +TEST_F(WriterTableTest, StopAndDrainWaitsForQueuedCleanupTask) { + ExecutorImpl pressure_executor("test_pressure_queued_cleanup", 1); + ASSERT_TRUE(pressure_executor.Start()); + ExecutorImpl cleanup_executor("test_cleanup_queued_cleanup", 1); + ASSERT_TRUE(cleanup_executor.Start()); + + std::mutex gate_mutex; + std::condition_variable gate_cv; + bool allow_cleanup = false; + ASSERT_TRUE(cleanup_executor.Execute([&] { + std::unique_lock lock(gate_mutex); + gate_cv.wait(lock, [&] { return allow_cleanup; }); + })); + + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(&cleanup_executor)); + WritePressureController controller(table_.get(), &pressure_executor); + auto drain = MakeScopedCleanup([&] { + controller.StopAndDrain(); + pressure_executor.Stop(); + cleanup_executor.Stop(); + }); + + std::mutex mutex; + std::condition_variable cv; + bool write_slice_called = false; + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([&](auto, auto, auto, auto, auto) { + { + std::lock_guard lock(mutex); + write_slice_called = true; + } + cv.notify_all(); + return Status::OK(); + }); + + FileWriter* writer = table_->AcquireWriter(601); + ASSERT_NE(writer, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(writer->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + controller.OnWritePressure(); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), + [&] { return write_slice_called; })); + } + + // The member cleanup task is now queued behind the gate task. + auto stopped = std::async(std::launch::async, [&] { + controller.StopAndDrain(); + return true; + }); + EXPECT_EQ(stopped.wait_for(std::chrono::milliseconds(50)), + std::future_status::timeout) + << "StopAndDrain must wait for the queued member cleanup"; + + { + std::lock_guard lock(gate_mutex); + allow_cleanup = true; + } + gate_cv.notify_all(); + + ASSERT_EQ(stopped.wait_for(std::chrono::seconds(5)), + std::future_status::ready); + EXPECT_TRUE(stopped.get()); + + table_->ReleaseWriter(writer); + EXPECT_EQ(table_->Size(), 0u); +} + +// FlushDirtyAsync completes inline (exactly once, OK) on an empty table; +// for a non-empty table the final callback runs on the cleanup executor +// after every member holder was released there, and entries with external +// holders survive the round. +TEST_F(WriterTableTest, FlushDirtyAsyncCompletionAndHolderAccounting) { + // Empty table: inline completion. + int empty_calls = 0; + table_->FlushDirtyAsync([&](Status s) { + ++empty_calls; + EXPECT_TRUE(s.ok()) << s.ToString(); + }); + EXPECT_EQ(empty_calls, 1); + + FileWriter* w1 = table_->AcquireWriter(602); + FileWriter* w2 = table_->AcquireWriter(603); + ASSERT_NE(w1, nullptr); + ASSERT_NE(w2, nullptr); + + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w1->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + ASSERT_TRUE(w2->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + std::mutex mutex; + std::condition_variable cv; + int final_calls = 0; + Status final_status; + std::thread::id final_thread{}; + table_->FlushDirtyAsync([&](Status s) { + std::lock_guard lock(mutex); + ++final_calls; + final_status = s; + final_thread = std::this_thread::get_id(); + cv.notify_all(); + }); + { + std::unique_lock lock(mutex); + ASSERT_TRUE(cv.wait_for(lock, std::chrono::seconds(5), + [&] { return final_calls == 1; })); + } + EXPECT_TRUE(final_status.ok()) << final_status.ToString(); + EXPECT_NE(final_thread, std::this_thread::get_id()) + << "the final callback of a non-empty round must come from the " + "cleanup executor, not from the submitting thread"; + + // Both external holders still own their entries; no snapshot holder + // leaked from the round. + EXPECT_EQ(table_->Size(), 2u); + table_->ReleaseWriter(w1); + EXPECT_EQ(table_->Size(), 1u); + table_->ReleaseWriter(w2); + EXPECT_EQ(table_->Size(), 0u); +} + +// A failed member flush must propagate to the final callback and still +// return every transient holder: after the round, releasing the external +// holders evicts both entries (no leaked pins). +TEST_F(WriterTableTest, FlushDirtyAsyncFailureReturnsAllHolders) { + FileWriter* w1 = table_->AcquireWriter(604); + FileWriter* w2 = table_->AcquireWriter(605); + ASSERT_NE(w1, nullptr); + ASSERT_NE(w2, nullptr); + + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w1->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + ASSERT_TRUE(w2->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + ON_CALL(*mock_meta_system_, WriteSlice) + .WillByDefault([](auto, auto, auto, auto, auto) { + return Status::Internal("flush failed"); + }); + + std::mutex mutex; + std::condition_variable cv; + Status final_status; + bool done = false; + table_->FlushDirtyAsync([&](Status s) { + std::lock_guard lock(mutex); + final_status = s; + done = true; + cv.notify_all(); + }); + { + std::unique_lock lock(mutex); + ASSERT_TRUE( + cv.wait_for(lock, std::chrono::seconds(5), [&] { return done; })); + } + EXPECT_FALSE(final_status.ok()); + + table_->ReleaseWriter(w1); + table_->ReleaseWriter(w2); + EXPECT_EQ(table_->Size(), 0u) + << "failed round must not leak transient holders"; +} + +// The background single-shard snapshot must pin entries with holders (not +// just refs) against a concurrent last external release, mirroring the +// FlushAll pin contract one shard at a time. +TEST_F(WriterTableTest, SnapshotShardPinsWriterAgainstLastRelease) { + const uint64_t ino = 606; + FileWriter* w = table_->AcquireWriter(ino); + ASSERT_NE(w, nullptr); + const char buf[] = "dirty"; + uint64_t wsize = 0; + ASSERT_TRUE(w->Write(ctx_, buf, sizeof(buf), 0, &wsize).ok()); + + const size_t shard = absl::HashOf(ino) & (WriterTable::kShardCount - 1); + auto snap = table_->SnapshotShard(shard); + ASSERT_EQ(snap.size(), 1u); + ASSERT_EQ(snap[0], w); + + // Last external holder gone: the snapshot holder must keep the entry + // alive until the flush completes and the snapshot releases it. + table_->ReleaseWriter(w); + EXPECT_EQ(table_->Size(), 1u); + + ASSERT_TRUE(snap[0]->Flush().ok()); + table_->ReleaseWriter(snap[0]); + EXPECT_EQ(table_->Size(), 0u); +} + } // namespace vfs } // namespace client } // namespace dingofs diff --git a/test/unit/client/vfs/mock/mock_vfs_hub.h b/test/unit/client/vfs/mock/mock_vfs_hub.h index 3f69cd034..56a974742 100644 --- a/test/unit/client/vfs/mock/mock_vfs_hub.h +++ b/test/unit/client/vfs/mock/mock_vfs_hub.h @@ -42,6 +42,7 @@ class MockVFSHub : public VFSHub { MOCK_METHOD(Executor*, GetWriteBackgroundExecutor, (), (override)); MOCK_METHOD(Executor*, GetFlushExecutor, (), (override)); MOCK_METHOD(Executor*, GetCBExecutor, (), (override)); + MOCK_METHOD(Executor*, GetCleanupExecutor, (), (override)); MOCK_METHOD(WriteMemPool*, GetWriteMemPool, (), (override)); MOCK_METHOD(ReadMemPool*, GetReadMemPool, (), (override)); MOCK_METHOD(ReadMemPool*, GetCompactMemPool, (), (override)); diff --git a/test/unit/client/vfs/test_base.h b/test/unit/client/vfs/test_base.h index 16e270308..a095d2b7d 100644 --- a/test/unit/client/vfs/test_base.h +++ b/test/unit/client/vfs/test_base.h @@ -103,11 +103,13 @@ class VFSTestBase : public ::testing::Test { write_background_executor_ = std::make_unique("test_write_bg", 1); cb_executor_ = std::make_unique("test_cb", 1); + cleanup_executor_ = std::make_unique("test_cleanup", 1); CHECK(read_executor_->Start()); CHECK(flush_executor_->Start()); CHECK(read_cleanup_executor_->Start()); CHECK(write_background_executor_->Start()); CHECK(cb_executor_->Start()); + CHECK(cleanup_executor_->Start()); // --- 7. MockVFSHub defaults (ON_CALL + AnyNumber pattern) --- ON_CALL(*mock_hub_, GetMetaSystem()) @@ -131,24 +133,29 @@ class VFSTestBase : public ::testing::Test { ON_CALL(*mock_hub_, GetCompactor()).WillByDefault(Return(mock_compactor_)); ON_CALL(*mock_hub_, GetReadExecutor()) .WillByDefault(Return(read_executor_.get())); - ON_CALL(*mock_hub_, GetFlushExecutor()) - .WillByDefault(Return(flush_executor_.get())); ON_CALL(*mock_hub_, GetReadCleanupExecutor()) .WillByDefault(Return(read_cleanup_executor_.get())); + ON_CALL(*mock_hub_, GetFlushExecutor()) + .WillByDefault(Return(flush_executor_.get())); ON_CALL(*mock_hub_, GetWriteBackgroundExecutor()) .WillByDefault(Return(write_background_executor_.get())); ON_CALL(*mock_hub_, GetCBExecutor()) .WillByDefault(Return(cb_executor_.get())); + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(cleanup_executor_.get())); ON_CALL(*mock_hub_, GetFsInfo()).WillByDefault(Return(MakeTestFsInfo())); // Delegate to GetFsInfo so a test that overrides the geometry with a // custom MakeTestFsInfo(chunk, block) keeps all three accessors // consistent without overriding each one separately. - ON_CALL(*mock_hub_, GetChunkSize()) - .WillByDefault([this]() { return mock_hub_->GetFsInfo().chunk_size; }); - ON_CALL(*mock_hub_, GetBlockSize()) - .WillByDefault([this]() { return mock_hub_->GetFsInfo().block_size; }); - ON_CALL(*mock_hub_, GetFsId()) - .WillByDefault([this]() { return mock_hub_->GetFsInfo().id; }); + ON_CALL(*mock_hub_, GetChunkSize()).WillByDefault([this]() { + return mock_hub_->GetFsInfo().chunk_size; + }); + ON_CALL(*mock_hub_, GetBlockSize()).WillByDefault([this]() { + return mock_hub_->GetFsInfo().block_size; + }); + ON_CALL(*mock_hub_, GetFsId()).WillByDefault([this]() { + return mock_hub_->GetFsInfo().id; + }); // Null mapper => uid/gid translation passthrough. Tests that exercise the // enabled-mapper path override this with their own real mapper. ON_CALL(*mock_hub_, GetUidGidMapper()).WillByDefault(Return(nullptr)); @@ -161,13 +168,11 @@ class VFSTestBase : public ::testing::Test { EXPECT_CALL(*mock_hub_, GetReadMemPool()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetCompactMemPool()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetWriteMemPool()).Times(AnyNumber()); - EXPECT_CALL(*mock_hub_, GetFileSuffixWatcher()).Times(AnyNumber()); - EXPECT_CALL(*mock_hub_, GetCompactor()).Times(AnyNumber()); - EXPECT_CALL(*mock_hub_, GetReadExecutor()).Times(AnyNumber()); + EXPECT_CALL(*mock_hub_, GetCBExecutor()).Times(AnyNumber()); + EXPECT_CALL(*mock_hub_, GetCleanupExecutor()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetFlushExecutor()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetReadCleanupExecutor()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetWriteBackgroundExecutor()).Times(AnyNumber()); - EXPECT_CALL(*mock_hub_, GetCBExecutor()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetFsInfo()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetUidGidMapper()).Times(AnyNumber()); EXPECT_CALL(*mock_hub_, GetChunkSize()).Times(AnyNumber()); @@ -246,6 +251,7 @@ class VFSTestBase : public ::testing::Test { read_executor_->Stop(); read_cleanup_executor_->Stop(); cb_executor_->Stop(); + cleanup_executor_->Stop(); } protected: @@ -273,6 +279,7 @@ class VFSTestBase : public ::testing::Test { std::unique_ptr read_cleanup_executor_; std::unique_ptr write_background_executor_; std::unique_ptr cb_executor_; + std::unique_ptr cleanup_executor_; ContextSPtr ctx_; }; diff --git a/test/unit/client/vfs/test_vfs_impl.cc b/test/unit/client/vfs/test_vfs_impl.cc index daa4cedc8..496b1bae7 100644 --- a/test/unit/client/vfs/test_vfs_impl.cc +++ b/test/unit/client/vfs/test_vfs_impl.cc @@ -410,10 +410,6 @@ TEST_F(VFSImplTest, Write_PressureFlushFails_CrossChunkWriteReturnsShortWrite) { constexpr Ino kIno = 601; constexpr Ino kInoDonor = 600; - gflags::FlagSaver flag_saver; - FLAGS_vfs_periodic_flush_interval_ms = - 3600 * 1000; // keep periodic flush out - // Local table + tiny pool + the real pressure-controller chain. auto writer_table = std::make_unique(mock_hub_); ASSERT_TRUE(writer_table->Start().ok()); @@ -424,12 +420,14 @@ TEST_F(VFSImplTest, Write_PressureFlushFails_CrossChunkWriteReturnsShortWrite) { WriteMemPool tiny_pool(kPoolPages * kPage, kPage); ON_CALL(*mock_hub_, GetWriteMemPool()).WillByDefault(Return(&tiny_pool)); - ExecutorImpl pressure_executor("test_pressure_flush_fail", 1); ASSERT_TRUE(pressure_executor.Start()); + ExecutorImpl cleanup_executor("test_cleanup_flush_fail", 1); + ASSERT_TRUE(cleanup_executor.Start()); + ON_CALL(*mock_hub_, GetCleanupExecutor()) + .WillByDefault(Return(&cleanup_executor)); WritePressureController controller(writer_table.get(), &pressure_executor); tiny_pool.SetPressureObserver(&controller); - // Every block upload completes inline with the same error (deterministic, // no background timing). The cv turns "the pressure round reached the data // plane" into an event the test can wait for. @@ -472,7 +470,6 @@ TEST_F(VFSImplTest, Write_PressureFlushFails_CrossChunkWriteReturnsShortWrite) { // explicitly fails its flush below. auto* donor = new FileWriter(mock_hub_, kInoDonor); donor->AcquireRef(); - ASSERT_TRUE(donor->Open().ok()); std::vector donor_buf(6144, 'd'); uint64_t donor_wsize = 0; ASSERT_TRUE( @@ -565,6 +562,7 @@ TEST_F(VFSImplTest, Write_PressureFlushFails_CrossChunkWriteReturnsShortWrite) { tiny_pool.SetPressureObserver(nullptr); controller.StopAndDrain(); ASSERT_TRUE(pressure_executor.Stop()); + ASSERT_TRUE(cleanup_executor.Stop()); } // --- 5. GetAttr on .stats inode returns virtual attr --- From 5caf65f15c650d794b0e814f3ef54b32d047dc90 Mon Sep 17 00:00:00 2001 From: chuandew Date: Sun, 20 Sep 2026 10:31:15 +0800 Subject: [PATCH 7/7] [fix][client] Register handles under the shard lock; preserve concurrent opens. --- src/client/vfs/metasystem/mds/file_session.cc | 6 +++--- src/client/vfs/metasystem/mds/file_session.h | 2 ++ 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/client/vfs/metasystem/mds/file_session.cc b/src/client/vfs/metasystem/mds/file_session.cc index dd1537b20..aae94b666 100644 --- a/src/client/vfs/metasystem/mds/file_session.cc +++ b/src/client/vfs/metasystem/mds/file_session.cc @@ -228,7 +228,7 @@ FileSessionSPtr FileSessionMap::Put(Ino ino, uint64_t fh, FileSessionSPtr file_session; shard_map_.withWLock( - [this, ino, fh, &session_id, &file_session](Map& map) { + [this, ino, fh, flags, &session_id, &file_session](Map& map) { auto [it, inserted] = map.try_emplace(ino); if (inserted) { it->second = FileSession::New(ino, chunk_size_); @@ -236,11 +236,11 @@ FileSessionSPtr FileSessionMap::Put(Ino ino, uint64_t fh, } file_session = it->second; + // Keep handle registration atomic with the inode's last close. + file_session->AddSession(fh, session_id, flags); }, ino); - file_session->AddSession(fh, session_id, flags); - return file_session; } diff --git a/src/client/vfs/metasystem/mds/file_session.h b/src/client/vfs/metasystem/mds/file_session.h index 910564403..79ecec6f3 100644 --- a/src/client/vfs/metasystem/mds/file_session.h +++ b/src/client/vfs/metasystem/mds/file_session.h @@ -122,6 +122,8 @@ class FileSessionMap { : inode_cache_(inode_cache), chunk_size_(chunk_size) {} ~FileSessionMap() = default; + // fh must be a new, nonzero handle and session_id must be nonempty. + // The session stays indexed until fh is deleted, even if other handles close. FileSessionSPtr Put(Ino ino, uint64_t fh, const std::string& session_id, uint32_t flags); void Delete(Ino ino, uint64_t fh);