DAOS-19173 pool: destroy timeout tiering and serialization by kccain · Pull Request #18518 · daos-stack/daos

kccain · 2026-06-18T20:03:25Z

For MD on SSD configurations, engine local SCM capacity based corpc timeout configuration:

Add pool_destroy_local_scm_size() get engine-local SCM size
Add pool_destroy_rpc_timeout() to customize destroy CoRPC timeout for ds_mgmt_tgt_pool_destroy_ranks()
non-MD-on-SSD case falls back to default timeout

And serialize pool target destroy handling for when a previous handler invocation (whose CoRPC initiator timed out) is still busy performing expensive subtree destruction / file unlinking:

Refactor ds_pooltgts with dual synchronization domains: Create: dpt_create_mutex/cv + dpt_creates_ht (existing create-cancel) Destroy: dpt_destroy_mutex/cv + dpt_destroys_ht (new serialization)
Add ds_pooltgts_destroy_rec for destroy-in-flight tracking hash table
Add to ds_mgmt_hdlr_tgt_destroy() the destroy serialization

Features: pool

Steps for the author:

Commit message follows the guidelines.
Appropriate Features or Test-tag pragmas were used.
Appropriate Functional Test Stages were run.
At least two positive code reviews including at least one code owner from each category referenced in the PR.
Testing is complete. If necessary, forced-landing label added and a reason added in a comment.

After all prior steps are complete:

Gatekeeper requested (daos-gatekeeper added as a reviewer).

For MD on SSD configurations, engine local SCM capacity based corpc timeout configuration: - Add pool_destroy_local_scm_size() get engine-local SCM size - Add pool_destroy_rpc_timeout() to customize destroy CoRPC timeout for ds_mgmt_tgt_pool_destroy_ranks() - non-MD-on-SSD case falls back to default timeout And serialize pool target destroy handling for when a previous handler invocation (whose CoRPC initiator timed out) is still busy performing expensive subtree destruction / file unlinking: - Refactor ds_pooltgts with dual synchronization domains: Create: dpt_create_mutex/cv + dpt_creates_ht (existing create-cancel) Destroy: dpt_destroy_mutex/cv + dpt_destroys_ht (new serialization) - Add ds_pooltgts_destroy_rec for destroy-in-flight tracking hash table - Add to ds_mgmt_hdlr_tgt_destroy() the destroy serialization Features: pool Signed-off-by: Kenneth Cain <kenneth.cain@hpe.com>

github-actions · 2026-06-18T20:04:14Z

Ticket title is 'dmg pool destroy very slow'
Status is 'In Progress'
https://daosio.atlassian.net/browse/DAOS-19173

daosbuild3 · 2026-06-18T20:47:54Z

Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18518/1/testReport/

daosbuild3 · 2026-06-19T05:14:02Z

Test stage Functional Hardware Large MD on SSD completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18518/1/execution/node/1319/log

kccain · 2026-07-06T14:08:17Z

reviewing this master patch in parallel to @Michael-Hennecke patch deployment and test of the backport PR #18523. That backport is a candidate for 2.8-rc2 (subject to reviewing/landing this master PR first, and approval for merging to 2.8).

wangshilong · 2026-07-07T01:14:20Z

+		return 0;
+
+	rc = smd_pool_get_info(pool_uuid, &pool_info);
+	if (rc != 0 || pool_info == NULL)


[minor] looks rc == 0, pool_info is always no NULL.

You're right. Checking pool_info is unnecessarily defensive for the existing API. If I need to push another version of the patch I can simplify the check to be based on rc only.

wangshilong · 2026-07-07T01:14:44Z

+	if (rc != 0 || pool_info == NULL)
+		return 0;
+
+	if (pool_info->spi_scm_sz > 0 && dss_tgt_nr > 0)


[question] is it possible that dss_tgt_nr == 0? maybe not.

I am not aware of a code path in normal operation with dss_tgt_nr == 0. Even in an unexpected case, the timeout would fall back to a default value.

wangshilong · 2026-07-07T01:17:47Z

+		timeout = 90;
+	else
+		timeout = 180;
+


[nit] probably define a macro is better.

This makes sense. If I refresh the patch, I can convert the values to named macros. It probably makes sense to do the same for SCM thresholds in pool_create_rpc_timeout() to be consistent.

wangshilong · 2026-07-07T01:27:20Z

+		break;
+	} while (1);
+	ABT_mutex_unlock(pooltgts->dpt_destroy_mutex);
+


[Question] Introduce two mutex for create/destroy looks a bit strange, normally I think just one mutex to serialize create/destroy might be fine? The reason why we do this is we want to allow parallel for create and destroy for different pools?

The intent was to be conservative. The existing create/destroy conflict handling already had shared lock synchronization. When adding destroy/destroy serialization for the same pool in this patch, I took the same general approach, but kept its synchronization coordination separate from the create/destroy code path to avoid any unintended interactions between the two while fixing this timeout issue. It could be good to unify the conflict model in follow-up work, but I wanted to scope this patch narrowly.

liw

I think we should use one ds_pooltgts record for all conflicts (i.e., create-create, create-destroy, and destroy-destroy) of one pool. (When requesting ds_pooltgts many years ago, I was secretly requesting a lock table---I plan to use such a concept elsewhere too.) That said, it does seem okay to me to leave this to future work.

liw · 2026-07-07T01:48:24Z

+				       &dtrec->dptr_hlink, true);
+		if (rc != 0) {
+			D_FREE(dtrec);
+			dtrec = NULL;


[Nit] Just FYI, D_FREE already sets the pointer to NULL.

Good point. I can clean that up if I push another version of the patch.

kccain · 2026-07-07T15:42:13Z

I think we should use one ds_pooltgts record for all conflicts (i.e., create-create, create-destroy, and destroy-destroy) of one pool. (When requesting ds_pooltgts many years ago, I was secretly requesting a lock table---I plan to use such a concept elsewhere too.) That said, it does seem okay to me to leave this to future work.

I see this suggestion as calling for a common record for all conflict types for a given pool. I am not sure whether the suggestion also implies a specific lock structure, such as one lock per pool or a shared lock protecting a table of per-pool records. This seems like it could be a good follow-up direction. In this patch I chose an incremental approach.

kccain · 2026-07-09T18:33:01Z

I did some testing on a small system to verify the SCM/tmpfs-only system gets a default timeout (60 sec), and made a temporary hack to the PR code to induce the destroy-destroy conflict logic to be detected, and proved it was engaged and worked as expected by looking at all engine logs. @Michael-Hennecke ran the 2.8 backport PR #18523 on a large NVMe system, but was sharing storage with another user, so was unable to exercise at the single pool size from the original problem. The pool destroy worked without any timeout/retry (due to both the smaller tmpfs usage / smaller pool size and the fact that the tgt destroy CoRPC timeout is configured to be 90 seconds instead of 60).

Based on this, I think it is OK to go ahead with gatekeeper review / landing of this PR, and then consider the 2.8 backport.

daltonbohning · 2026-07-09T21:11:22Z

NLT failure seems to be in the GO runtime, and this PR does not include the recent change to stabilize that. Therefore, unrelated.

kccain marked this pull request as ready for review July 6, 2026 14:08

kccain requested review from liw and wangshilong July 6, 2026 14:08

wangshilong reviewed Jul 7, 2026

View reviewed changes

wangshilong approved these changes Jul 7, 2026

View reviewed changes

liw approved these changes Jul 7, 2026

View reviewed changes

kccain requested a review from a team July 9, 2026 18:33

daltonbohning added the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Jul 9, 2026

daltonbohning merged commit 7642c40 into master Jul 9, 2026
39 of 42 checks passed

daltonbohning deleted the kccain/daos_19173 branch July 9, 2026 21:11

Uh oh!

Conversation

kccain commented Jun 18, 2026

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Jun 18, 2026

Uh oh!

daosbuild3 commented Jun 18, 2026

Uh oh!

daosbuild3 commented Jun 19, 2026

Uh oh!

kccain commented Jul 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kccain commented Jul 7, 2026

Uh oh!

kccain commented Jul 9, 2026

Uh oh!

daltonbohning commented Jul 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants