DAOS-18431 bio: Set power management register on NVMe by tanabarr · Pull Request #17355 · daos-stack/daos

tanabarr · 2026-01-08T12:25:55Z

Set NVMe power management values for SSDs by setting the new
engine DAOS_NVME_POWER_MGMT environment variable to an integer
(sets register bits 0-4). Value will be applied by SPDK on devices
attached to an engine process. The value will not be reset on engine
exit.

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).

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

github-actions · 2026-01-08T12:26:15Z

Ticket title is 'NVMe power control feature'
Status is 'Awaiting Verification'
https://daosio.atlassian.net/browse/DAOS-18431

daosbuild3 · 2026-01-08T14:15:48Z

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

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

kjacque

Overall looks reasonable. Just trying to understand the right way to interact with the spdk stuff.

If we're letting users configure this generally, it might be good to add an explicit parameter to the config file with more intuitively named values, based on whatever the 0-4 stand for. Not necessary in this PR, but something to think about.

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

NiuYawei · 2026-01-13T06:54:14Z

 	bb->bb_dev_health.bdh_io_channel = channel;

+	/* Set NVMe power management */
+	rc = bio_set_power_mgmt(bb->bb_dev, channel);


I think we don't want this being skipped when health monitor is disabled? (see bypass_health_collect() at the beginning of this function).

is it okay to move this function and the channel fetch to before the bypass check?

Hmm, this function uses the io_channel created by bio_init_health_monitoring().

NiuYawei · 2026-01-13T07:00:54Z

+	cmd.cdw10 = SPDK_NVME_FEAT_POWER_MANAGEMENT;
+	cmd.cdw11 = bio_spdk_power_mgmt_val;
+
+	rc = spdk_bdev_nvme_admin_passthru(d_bdev->bb_desc, channel, &cmd, NULL, 0,


This uses bdh_io_channel but doesn't hold reference count, that could cause issue when bio_dev_health being finalized before the completion. See bio_fini_health_monitoring().

I've decoupled implementation from health monitoring, is it better now?

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

daosbuild3 · 2026-01-17T03:36:37Z

Test stage Functional Hardware Medium 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-17355/7/execution/node/1334/log

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

daosbuild3 · 2026-01-19T00:19:23Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17355/8/testReport/

NiuYawei · 2026-01-19T02:33:05Z

+	struct spdk_nvme_cmd cmd;
+	int                         rc = 0;
+
+	D_INFO("Power management 1\n");


All these "Power management x" info messages in this function should be removed.

NiuYawei · 2026-01-19T02:36:02Z

+}
+
+int
+bio_set_power_mgmt(const char *bdev_name, struct spdk_bdev *bdev)


It looks redundant to pass in both "bdev_name" & "bdev".

NiuYawei · 2026-01-19T02:36:33Z

+	}
+
+	if (!spdk_bdev_io_type_supported(bdev, SPDK_BDEV_IO_TYPE_NVME_ADMIN)) {
+		D_ERROR("Bdev NVMe admin passthru not supported for %s\n", bdev_name);


Is it an error?

moved to debug

NiuYawei · 2026-01-19T02:45:03Z

+	}
+
+	/* Writable descriptor required for applying power management settings */
+	rc = spdk_bdev_open_ext(bdev_name, true, bdev_event_cb, NULL, &pm_ctx.bdev_desc);


I think the bio_bdev_event_cb() should be used as the event callback.

NiuYawei · 2026-01-19T02:49:34Z

+
+	D_INFO("Power management 3\n");
+	rc = spdk_bdev_nvme_admin_passthru(pm_ctx.bdev_desc, pm_ctx.bdev_io_channel, &cmd, NULL, 0,
+					   set_power_mgmt_completion, &pm_ctx);


bio_xsctxt_alloc() isn't completed at this moment, and NVMe polling ULT isn't created yet, you need to explicitly poll completion by your self (see xs_poll_completion())

So you don't need ABT_eventual for such self polling. You could refer to common_fini_cb() & xs_poll_completion().

NiuYawei · 2026-01-19T02:54:44Z

+
+	spdk_bdev_free_io(bdev_io);
+	spdk_put_io_channel(pm_ctx->bdev_io_channel);
+	spdk_bdev_close(pm_ctx->bdev_desc);


It's not callback driven model anymore. The io_channel & dev_desc could be put/closed by caller.

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr · 2026-01-24T14:19:25Z

@NiuYawei I tried to address your comments, I'm now getting SPDK_NVME_SC_INVALID_FIELD if I try to set value to 0x01, fine if setting to 0x00 but I think this maybe to do with the SSD not supporting changes. @Michael-Hennecke please could you verify on your side?
Logging example:

2026/01/25 23:05:46.136270 edaos-15 DAOS[241271/0/3] bio DBUG src/bio/bio_device.c:1166 bio_set_power_mgmt() Setting power state 0 on device Nvme_edaos-15.daos.hpc.amslabs.hpecorp.net_0_1_7n1

2026/01/25 23:05:46.136306 edaos-15 DAOS[241271/0/3] bio INFO src/bio/bio_device.c:1095 set_power_mgmt_completion() Power management value set on device Nvme_edaos-15.daos.hpc.amslabs.hpecorp.net_0_1_7n1

daosbuild3 · 2026-01-24T15:08:44Z

Test stage NLT on EL 8.8 completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17355/9/testReport/

daosbuild3 · 2026-01-25T08:57:16Z

Test stage Functional Hardware Medium MD on SSD completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-17355/9/testReport/

NiuYawei · 2026-01-26T09:40:21Z

 }
+
+struct power_mgmt_context_t {
+	ABT_eventual            eventual;


This isn't useful, can be removed.

has been addressed

NiuYawei · 2026-01-26T09:43:46Z

+		D_DEBUG(DB_MGMT, "Bdev NVMe admin passthru not supported for %s\n", bdev_name);
+		rc = -DER_NOTSUPPORTED;
+		goto out_desc;
+	}


'bdev' can be acquired by spdk_bdev_get_by_name(), these checking should be done before spdk_bdev_open_ext()

has been addressed

kjacque

A couple minor comments. Nothing I'd block on for this PR.

tanabarr · 2026-01-28T13:23:46Z

@Michael-Hennecke can you please verify the PR and then we can land the change? TIA

Michael-Hennecke

in the src/bio/bio_device.c:1096 set_power_mgmt_completion() Power management value set on device Nvme_N0003_0_1_7n1 messages, can we print the actual value that it was set to? like .. value set to 0 on device ...

yes there's the global bio_nvme_init_ext() NVMe power management setting to be applied is 1 message before. but for example you may have different NVMe types in the engine, some may support that value and some may not.

so both the success and the failure messages (src/bio/bio_device.c:1087 set_power_mgmt_completion() Set power management failed for device Nvme_N0003_0_1_7n1, NVMe status code/type: 0x2/0x0) should always print the values, for each NVMe disk operation.

Michael-Hennecke · 2026-02-09T12:09:38Z

I have validated with the Samsung drives (that support power states 0 to 3) that the setting works as expected:

2026/02/09 05:00:44.712366 N0003 DAOS[111568/-1/0] bio  INFO src/bio/bio_xstream.c:277 bio_nvme_init_ext() NVMe power management setting to be applied is 3
2026/02/09 05:00:46.288579 N0003 DAOS[111568/0/1] bio  INFO src/bio/bio_device.c:1096 set_power_mgmt_completion() Power management value set on device Nvme_N0003_0_1_7n1

but as expected:

2026/02/09 04:57:17.216038 N0003 DAOS[107878/-1/0] bio  INFO src/bio/bio_xstream.c:277 bio_nvme_init_ext() NVMe power management setting to be applied is 5
2026/02/09 04:57:18.764518 N0003 DAOS[107878/0/1] bio  ERR  src/bio/bio_device.c:1087 set_power_mgmt_completion() Set power management failed for device Nvme_N0003_0_1_7n1, NVMe status code/type: 0x2/0x0
2026/02/09 04:57:18.764563 N0003 DAOS[107878/0/1] bio  ERR  src/bio/bio_device.c:1091 set_power_mgmt_completion()  - INVALID_FIELD: Device may not support requested power state

I also verified that the engines still come up even after the failure to set the power state, which is what we want.

Michael-Hennecke · 2026-02-09T12:13:02Z

Overall looks reasonable. Just trying to understand the right way to interact with the spdk stuff.

If we're letting users configure this generally, it might be good to add an explicit parameter to the config file with more intuitively named values, based on whatever the 0-4 stand for. Not necessary in this PR, but something to think about.

the supported values (other than 0) and their meaning are vendor-specific, so it's not possible for us to use descriptive names here...

Michael-Hennecke

looks good, verified to work on Samsung drives. PR can be landed, but see my proposal to print power state values in log messages.

tanabarr · 2026-02-09T13:44:47Z

in the src/bio/bio_device.c:1096 set_power_mgmt_completion() Power management value set on device Nvme_N0003_0_1_7n1 messages, can we print the actual value that it was set to? like .. value set to 0 on device ...

yes there's the global bio_nvme_init_ext() NVMe power management setting to be applied is 1 message before. but for example you may have different NVMe types in the engine, some may support that value and some may not.

so both the success and the failure messages (src/bio/bio_device.c:1087 set_power_mgmt_completion() Set power management failed for device Nvme_N0003_0_1_7n1, NVMe status code/type: 0x2/0x0) should always print the values, for each NVMe disk operation.

this has been addressed

daltonbohning · 2026-02-09T20:35:02Z

@tanabarr Any strong reason for not fixing the clang-format? And FWIW if you install it in your dev env the githooks will do it for you
https://github.com/daos-stack/daos/tree/master/utils/githooks#2-install-the-tools

tanabarr · 2026-02-09T21:08:25Z

@tanabarr Any strong reason for not fixing the clang-format? And FWIW if you install it in your dev env the githooks will do it for you https://github.com/daos-stack/daos/tree/master/utils/githooks#2-install-the-tools

I didn't want to waste CI cycles on what essentially is an unnecessary reformat of indentation. would you like me to fix and refresh?
I have the Hook applied and not sure why it didn't work.

this has been addressed

…age-patch Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr · 2026-02-10T18:33:35Z

@Michael-Hennecke do you want to ask about expediating this PR's landing so that we can get it in TB4? @daltonbohning do you think that would be possible? force landing maybe?

daltonbohning · 2026-02-10T18:42:26Z

@Michael-Hennecke do you want to ask about expediating this PR's landing so that we can get it in TB4? @daltonbohning do you think that would be possible? force landing maybe?

TB4 was tagged an hour ago
103e8b3

DAOS-18431 bio: Set power management register on NVMe

667181c

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr self-assigned this Jan 8, 2026

tanabarr requested review from a team as code owners January 8, 2026 12:25

Michael-Hennecke self-requested a review January 8, 2026 12:51

tanabarr force-pushed the tanabarr/bio-powmanage-patch branch from 5e0062d to 0be43f7 Compare January 8, 2026 17:26

read from DAOS_NVME_POWER_MGMT env

8b31c49

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr requested review from NiuYawei and kjacque January 8, 2026 17:35

tanabarr force-pushed the tanabarr/bio-powmanage-patch branch from 0be43f7 to 8b31c49 Compare January 8, 2026 17:38

kjacque requested changes Jan 9, 2026

View reviewed changes

Comment thread src/bio/bio_device.c

Comment thread src/bio/bio_device.c Outdated

Comment thread src/bio/bio_monitor.c Outdated

Comment thread src/bio/bio_device.c Outdated

Comment thread src/bio/bio_xstream.c

tanabarr requested a review from kjacque January 12, 2026 12:50

address review comments

6854e02

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

NiuYawei requested changes Jan 13, 2026

View reviewed changes

tanabarr requested a review from NiuYawei January 16, 2026 15:44

decouple set_power_mgmt() from health monitoring

2b38adf

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr force-pushed the tanabarr/bio-powmanage-patch branch from cde98e1 to 2b38adf Compare January 16, 2026 15:47

tanabarr added 2 commits January 18, 2026 15:11

use existing bdev descriptor during initial setup

b2b3725

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

separate pow mgmt to use independent writable descriptor

969471c

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

NiuYawei requested changes Jan 19, 2026

View reviewed changes

tanabarr added 3 commits January 19, 2026 14:18

validate power value

9d30a2a

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

address review comments

9a6d387

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

address review comments

d455f3b

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

NiuYawei previously approved these changes Jan 26, 2026

View reviewed changes

kjacque previously approved these changes Jan 26, 2026

View reviewed changes

Comment thread src/bio/bio_device.c Outdated

Comment thread src/bio/bio_device.c Outdated

Michael-Hennecke reviewed Feb 9, 2026

View reviewed changes

Michael-Hennecke self-requested a review February 9, 2026 12:06

Michael-Hennecke previously approved these changes Feb 9, 2026

View reviewed changes

tanabarr requested a review from a team February 9, 2026 13:49

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

tanabarr added 2 commits February 10, 2026 12:39

Merge remote-tracking branch 'origin/master' into tanabarr/bio-powman…

48ed7c7

…age-patch Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

address review comments

3408dc3

Signed-off-by: Tom Nabarro <thomas.nabarro@hpe.com>

tanabarr dismissed stale reviews from Michael-Hennecke, kjacque, and NiuYawei via 3408dc3 February 10, 2026 14:22

tanabarr requested review from Michael-Hennecke, NiuYawei and kjacque and removed request for a team February 10, 2026 14:23

NiuYawei approved these changes Feb 11, 2026

View reviewed changes

kjacque approved these changes Feb 11, 2026

View reviewed changes

tanabarr removed the forced-landing The PR has known failures or has intentionally reduced testing, but should still be landed. label Feb 12, 2026

tanabarr requested a review from a team February 12, 2026 10:21

daltonbohning merged commit 32c1281 into master Feb 12, 2026
57 of 75 checks passed

daltonbohning deleted the tanabarr/bio-powmanage-patch branch February 12, 2026 15:14

Conversation

tanabarr commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Steps for the author:

After all prior steps are complete:

Uh oh!

github-actions Bot commented Jan 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Jan 8, 2026

Uh oh!

kjacque left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

daosbuild3 commented Jan 17, 2026

Uh oh!

daosbuild3 commented Jan 19, 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!

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!

tanabarr commented Jan 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

daosbuild3 commented Jan 24, 2026

Uh oh!

daosbuild3 commented Jan 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanabarr Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanabarr Feb 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kjacque left a comment

Choose a reason for hiding this comment

tanabarr commented Jan 8, 2026 •

edited

Loading

github-actions Bot commented Jan 8, 2026 •

edited

Loading

tanabarr commented Jan 24, 2026 •

edited

Loading

tanabarr Feb 9, 2026 •

edited

Loading

tanabarr Feb 9, 2026 •

edited

Loading

Michael-Hennecke left a comment •

edited

Loading

Michael-Hennecke commented Feb 9, 2026 •

edited

Loading

tanabarr commented Feb 9, 2026 •

edited

Loading

tanabarr commented Feb 9, 2026 •

edited

Loading