deepseek-ai/3FS

Fork 0

mirror of https://github.com/deepseek-ai/3FS.git synced 2026-11-05 00:47:47 +00:00

[PR #429] Fix admin_cli shutdown order: stop IBManager after the clients #429

New issue

Open

opened 2026-10-26 09:50:11 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 09:50:11 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/deepseek-ai/3FS/pull/429
Author: @PerryLink
Created: 9/17/2026
Status: 🔄 Open

Base: main ← Head: fix/admin-cli-ibmanager-shutdown-order

📝 Commits (1)

a1c35ed Fix admin_cli shutdown order: stop IBManager after the clients

📊 Changes

1 file changed (+9 additions, -2 deletions)

View changed files

📝 src/client/bin/admin_cli.cc (+9 -2)

📄 Description

Fixes the shutdown order reported in #426.

Summary

admin_cli declared its clients first and registered the IBManager::stop() guard after them
(storageClient on line 100, the guard on line 111), so at scope exit the guard ran first:
IBManager::stop() executed while the clients - and their RDMA queue pairs - were still alive, and
the clients were destroyed afterwards. For any admin command that touches the storage client
(create-target, remove-target, ...) the storage-facing RC QP was therefore destroyed without
closeGracefully(): the storage side kept an orphan QP and only noticed it when its periodic
WRType::CHECK exhausted the RC retry budget.

This moves the guard above the client declarations, so it is registered first and therefore runs
last: clients first, IBManager::stop() last. That is the order src/fuse/hf3fs_fuse.cpp already
uses (the IBManager guard is registered right after IBManager::start(), the
SCOPE_EXIT { d.stop(); } for the clients later), and the order FuseClients::stop()
implements explicitly (meta -> storage -> mgmtd -> net client).

Root cause

Before:

std::shared_ptr<StorageClient> storageClient;    // line 100
...
SCOPE_EXIT { hf3fs::net::IBManager::stop(); };   // line 111

Automatic objects are destroyed in reverse order of declaration, so the guard registered at line
111 was destroyed before storageClient at line 100. IBManager::stop() ->
IBManager::reset() stops and resets socketManager_. When the storage client is finally
destroyed, StorageClientImpl::~StorageClientImpl() -> stop() -> messenger_.stopAndJoin() ->
IOWorker::stopAndJoin() -> dropConnections() destroys the transports, and
Transport::~Transport() calls IBManager::close(). With socketManager_ == nullptr,
IBManager::closeImpl() just lets the IBSocket die, and IBSocket::~IBSocket() only moves the
QP to IBV_QPS_ERR: the ImmData::close() message is never posted, so the peer never learns that
the connection is gone.

The other clients were already fine: mgmtdClient->stop() and client->stopAndJoin() run while
IBManager is still alive, which is why the mgmtd QP closed gracefully in the report while the
storage-facing QP did not.

Changes

src/client/bin/admin_cli.cc: move SCOPE_EXIT { hf3fs::net::IBManager::stop(); }; above the
client declarations, with a comment explaining why the position matters. One line moved, no API
change, no other binary touched.

Why this is safe

If no command needed IB, IBManager was never started; IBManager::stop() -> reset() is then
a no-op on default-constructed members.
The --release_version early return still happens before the guard is registered, so that path
is unchanged.
Monitor::stop() still runs before the clients are destroyed, exactly as before.
The window in which an IBSocket could outlive IBManager::reset() (socket manager and event
loop already gone) is closed. The storage QPs are now dropped while the IB event loop is still
running, so IBSocketManager::stopAndJoin() - called from IBManager::stop() - still has its
drain window to push the close message out.
Nothing else in this file depends on IBManager being stopped early.

What was not validated

This is a static analysis. I could not build or run 3FS (no RDMA hardware), so the ordering is
confirmed from the source and from the runtime log in #426, but the effect on a live fabric was
not measured.

The retry horizon quoted in #426 (~1764 s) is not reproducible from the shipped configuration:
retry_cnt = 7 and timeout = 14 in configs/admin_cli.toml and configs/storage_main.toml
imply sub-second RC retry exhaustion, and check_connections_interval defaults to 60 s, which
would surface the orphan QP within about a minute. That deployment must override these values.
The defect and this fix do not depend on the exact horizon.

Possible follow-up (not in this patch): IBManager::closeImpl() currently drops a socket silently
when socketManager_ == nullptr; logging or rejecting a not-closed/READY socket there would make
this class of shutdown-order bug self-reporting.

Prepared with AI assistance; reviewed before submission.

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

## 📋 Pull Request Information **Original PR:** https://github.com/deepseek-ai/3FS/pull/429 **Author:** [@PerryLink](https://github.com/PerryLink) **Created:** 9/17/2026 **Status:** 🔄 Open **Base:** `main` ← **Head:** `fix/admin-cli-ibmanager-shutdown-order` --- ### 📝 Commits (1) - [`a1c35ed`](https://github.com/deepseek-ai/3FS/commit/a1c35edc0d27369d0487053fa518b74ee53d214d) Fix admin_cli shutdown order: stop IBManager after the clients ### 📊 Changes **1 file changed** (+9 additions, -2 deletions) <details> <summary>View changed files</summary> 📝 `src/client/bin/admin_cli.cc` (+9 -2) </details> ### 📄 Description Fixes the shutdown order reported in #426. ## Summary `admin_cli` declared its clients first and registered the `IBManager::stop()` guard after them (`storageClient` on line 100, the guard on line 111), so at scope exit the guard ran first: `IBManager::stop()` executed while the clients - and their RDMA queue pairs - were still alive, and the clients were destroyed afterwards. For any admin command that touches the storage client (`create-target`, `remove-target`, ...) the storage-facing RC QP was therefore destroyed without `closeGracefully()`: the storage side kept an orphan QP and only noticed it when its periodic `WRType::CHECK` exhausted the RC retry budget. This moves the guard above the client declarations, so it is registered first and therefore runs last: clients first, `IBManager::stop()` last. That is the order `src/fuse/hf3fs_fuse.cpp` already uses (the `IBManager` guard is registered right after `IBManager::start()`, the `SCOPE_EXIT { d.stop(); }` for the clients later), and the order `FuseClients::stop()` implements explicitly (meta -> storage -> mgmtd -> net client). ## Root cause Before: std::shared_ptr<StorageClient> storageClient; // line 100 ... SCOPE_EXIT { hf3fs::net::IBManager::stop(); }; // line 111 Automatic objects are destroyed in reverse order of declaration, so the guard registered at line 111 was destroyed before `storageClient` at line 100. `IBManager::stop()` -> `IBManager::reset()` stops and resets `socketManager_`. When the storage client is finally destroyed, `StorageClientImpl::~StorageClientImpl()` -> `stop()` -> `messenger_.stopAndJoin()` -> `IOWorker::stopAndJoin()` -> `dropConnections()` destroys the transports, and `Transport::~Transport()` calls `IBManager::close()`. With `socketManager_ == nullptr`, `IBManager::closeImpl()` just lets the `IBSocket` die, and `IBSocket::~IBSocket()` only moves the QP to `IBV_QPS_ERR`: the `ImmData::close()` message is never posted, so the peer never learns that the connection is gone. The other clients were already fine: `mgmtdClient->stop()` and `client->stopAndJoin()` run while IBManager is still alive, which is why the mgmtd QP closed gracefully in the report while the storage-facing QP did not. ## Changes `src/client/bin/admin_cli.cc`: move `SCOPE_EXIT { hf3fs::net::IBManager::stop(); };` above the client declarations, with a comment explaining why the position matters. One line moved, no API change, no other binary touched. ## Why this is safe - If no command needed IB, `IBManager` was never started; `IBManager::stop()` -> `reset()` is then a no-op on default-constructed members. - The `--release_version` early return still happens before the guard is registered, so that path is unchanged. - `Monitor::stop()` still runs before the clients are destroyed, exactly as before. - The window in which an `IBSocket` could outlive `IBManager::reset()` (socket manager and event loop already gone) is closed. The storage QPs are now dropped while the IB event loop is still running, so `IBSocketManager::stopAndJoin()` - called from `IBManager::stop()` - still has its drain window to push the close message out. - Nothing else in this file depends on IBManager being stopped early. ## What was not validated This is a static analysis. I could not build or run 3FS (no RDMA hardware), so the ordering is confirmed from the source and from the runtime log in #426, but the effect on a live fabric was not measured. The retry horizon quoted in #426 (~1764 s) is not reproducible from the shipped configuration: `retry_cnt = 7` and `timeout = 14` in `configs/admin_cli.toml` and `configs/storage_main.toml` imply sub-second RC retry exhaustion, and `check_connections_interval` defaults to 60 s, which would surface the orphan QP within about a minute. That deployment must override these values. The defect and this fix do not depend on the exact horizon. Possible follow-up (not in this patch): `IBManager::closeImpl()` currently drops a socket silently when `socketManager_ == nullptr`; logging or rejecting a not-closed/READY socket there would make this class of shutdown-order bug self-reporting. Prepared with AI assistance; reviewed before submission. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi added the

pull-request

label

2026-10-26 09:50:11 +00:00

No labels

pull-request

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

deepseek-ai/3FS#429

No description provided.

Rows
Columns