Use persistent discovery controller by default by Mr-Bossman · Pull Request #3472 · linux-nvme/nvme-cli

Mr-Bossman · 2026-06-22T21:17:48Z

No description provided.

igaw · 2026-06-24T08:30:08Z

The idea of #3112 is to make the DC persistent based on the info from the DC. The global setting is basically wrong. It should always be off by default and only when the DC announces it wants be used as persistent, nvme-cli should do it.

Mr-Bossman · 2026-06-24T18:39:24Z

@igaw is more along the lines of what the feature request meant?

igaw · 2026-06-24T18:45:20Z

Yes as far I can tell from a quick glance. Did you have a chance to test this against a target?

@martin-gpy in case you have a cycle for testing, I would appreciate it!

Mr-Bossman · 2026-06-24T20:31:18Z

@igaw Yes i did and it does seems to work as expected. Though one thing that doesn't make sense to me is why _nvmf_discovery, which uses very similar code, checks for NVMF_DISC_EFLAGS_DUPRETINFO and if subnqn === NVME_DISC_SUBSYS_NAME. For both arrays that have EPCSD i tested on the NVMF_DISC_EFLAGS_DUPRETINFO is set for all the discovery entry. And for one the subnqn is not NVME_DISC_SUBSYS_NAME

hreinecke · 2026-06-25T06:36:12Z

The idea of #3112 is to make the DC persistent based on the info from the DC. The global setting is basically wrong. It should always be off by default and only when the DC announces it wants be used as persistent, nvme-cli should do it.

Well, thing it that the decision to create a persistent discovery controller is pretty much an admin choice. For some fabrics (like FC) creating a PDC is pretty much pointless as the fabrics always knows which ports are attached, and will always be notified if new ports are accessible. And creating a PDC eats up resources on the target, so admins might decide not to use them to save resources on the target. So yes, an admin-settable option is still required.

igaw · 2026-06-25T07:44:37Z

Oops, pushed the wrong button. Sorry.

Mr-Bossman · 2026-06-25T17:44:17Z

@igaw is there a way to generate the docs and the accesors for libnvme_host?

hreinecke · 2026-06-26T06:20:00Z

Why is the 'pdc supported' flag tagged to the host? It is the subsystem (well, the description of the subsystem in the discovery log page) which specifies whether PDC is supported. I think it would be better to move the flag to the subsystem...

Mr-Bossman · 2026-06-29T14:02:36Z

Why is the 'pdc supported' flag tagged to the host? It is the subsystem (well, the description of the subsystem in the discovery log page) which specifies whether PDC is supported. I think it would be better to move the flag to the subsystem...

Same reason libnvme_host_set_pdc_enabled exists. This flag only exists for discovery controllers which means the nqn is not known. To have it in the subsystem would require the json file to allow an empty nqn parameter

NVMe PDC (Persistent Discovery Controller) Closes: linux-nvme#3112 Link: linux-nvme#3112 Signed-off-by: Jesse Taube <jtaubepe@redhat.com>

igaw · 2026-06-29T15:13:59Z

fwiw, I think with your change we can drop the global pdc setting. From what I learned in this discussion, it never made sense.

hreinecke · 2026-06-30T09:05:26Z

The current implementation is the result of the request to have a compile-time setting on whether PDC should be created per default or not. But yes, I do agree

Why is the 'pdc supported' flag tagged to the host? It is the subsystem (well, the description of the subsystem in the discovery log page) which specifies whether PDC is supported. I think it would be better to move the flag to the subsystem...

Same reason libnvme_host_set_pdc_enabled exists. This flag only exists for discovery controllers which means the nqn is not known. To have it in the subsystem would require the json file to allow an empty nqn parameter

But then we should rather have this as an option in 'discovery.conf' ...

Mr-Bossman · 2026-06-30T15:11:27Z

fwiw, I think with your change we can drop the global pdc setting. From what I learned in this discussion, it never made sense.

If it is enabled there is no way to disable it too. Imo it's good to keep as the logic for it makes significantly more sence than the logic that uses the fabric context. Though having the ability to set the default value probably should be removed

Mr-Bossman · 2026-06-30T15:14:00Z

The current implementation is the result of the request to have a compile-time setting on whether PDC should be created per default or not. But yes, I do agree

Why is the 'pdc supported' flag tagged to the host? It is the subsystem (well, the description of the subsystem in the discovery log page) which specifies whether PDC is supported. I think it would be better to move the flag to the subsystem...

Same reason libnvme_host_set_pdc_enabled exists. This flag only exists for discovery controllers which means the nqn is not known. To have it in the subsystem would require the json file to allow an empty nqn parameter

But then we should rather have this as an option in 'discovery.conf' ...

I added it as both. as of now there isn't really a way to have global settings in discovery.conf. There was talks on matrix about having the ability to have global configs, which would remove the need for a compile tile setting.

igaw · 2026-07-01T15:34:02Z

+		libnvme_host_set_epcsd_enabled(h, true);
+
+	if (epcsd_disabled)
+		libnvme_host_set_epcsd_enabled(h, false);


tempted to argue for

if (epcsd_enabled || epcsd_disabled) libnvme_host_set_epcsd_enabled(h, epcsd_enabled);

but this might be a bit too obfuscated...

igaw · 2026-07-01T15:35:57Z

+		    (eflags & NVMF_DISC_EFLAGS_EPCSD) &&
+		    libnvme_host_is_epcsd_enabled(h, DEFAULT_EPCSD_ENABLED)) {
+			libnvmf_context_set_persistent(fctx, true);
+			break;


could you move this part into the existing loop where we already have the NVMF_DISC_EFLAGS_EPCSD check?

igaw · 2026-07-01T15:38:19Z

Looks good. Just a couple of nitpicks.

Could add a prefix to the subject, e.g. "fabrics: ..." and drop the links to github. The commit message should contain everything it needs to make it self explenatory. We might have to move eventually to a different hosters and then all those links are pretty useless. Thanks!

hreinecke · 2026-07-02T06:18:09Z

I wonder: do we really need this complexity? IE is it really worth it to have two additional settings?
We already have a global setting 'pdc enabled'; why do we need another setting 'pdc enabled if supported'?
If 'pdc enabled' is set, why do we need to set 'pdc enabled if supported' additionally? (We already said we want a PDC with the first option ...) And if PDC is not supported, why should we allow the user to create a PDC?

igaw · 2026-07-02T07:22:38Z

-Dpdc-enabled sets the default in libnvmf_context->persistent to true.

			/*
			 * Are we supposed to keep the discovery
			 * controller around?
			 */
			disconnect = !nfctx.persistent;

			if (strcmp(e->subnqn, NVME_DISC_SUBSYS_NAME)) {
				/*
				 * Does this discovery controller doesn't
				 * support explicit persistent connection?
				 */
				if (!(eflags & NVMF_DISC_EFLAGS_EPCSD))
					disconnect = true;
				else
					disconnect = false;
			}

Thus disconnect will be false on for any DC with an unique nqn. I suppose we could reuse -Dpdc-enabled and make the logic work correctly?

hreinecke · 2026-07-02T08:06:03Z

-Dpdc-enabled sets the default in libnvmf_context->persistent to true.

			/*
			 * Are we supposed to keep the discovery
			 * controller around?
			 */
			disconnect = !nfctx.persistent;

			if (strcmp(e->subnqn, NVME_DISC_SUBSYS_NAME)) {
				/*
				 * Does this discovery controller doesn't
				 * support explicit persistent connection?
				 */
				if (!(eflags & NVMF_DISC_EFLAGS_EPCSD))
					disconnect = true;
				else
					disconnect = false;
			}

Thus disconnect will be false on for any DC with an unique nqn. I suppose we could reuse -Dpdc-enabled and make the logic work correctly?

That would be preferable. I would think the decision matrix should be:

-> Does the controller support explicit persistent connections (ie NVMF_DISC_EFLAGS_EPCSD is set)?
-> No -> No PDC
-> Does the subsystem have the 'pdc_enabled' flag specified?
-> No -> Use default.
-> Yes -> Use PDC

This requires unique discovery subsystems to work correctly. Which is fine as I would not create a PDC for the default discovery NQN; the kernel will create a new discovery controller for every default discovery NQN, so we're having a hard time identifying which of the tons of discovery subsystems we should be using.

Mr-Bossman force-pushed the dev/jesse/pdc-default branch 3 times, most recently from a13506d to c1c9bd9 Compare June 24, 2026 18:36

igaw closed this Jun 25, 2026

igaw reopened this Jun 25, 2026

Mr-Bossman force-pushed the dev/jesse/pdc-default branch 2 times, most recently from 4ccd4fc to bdccb46 Compare June 25, 2026 17:37

Mr-Bossman force-pushed the dev/jesse/pdc-default branch 4 times, most recently from 7403d70 to 80a57f9 Compare June 29, 2026 15:10

Add option to create a pdc for controllers with epdc

ecb174c

NVMe PDC (Persistent Discovery Controller) Closes: linux-nvme#3112 Link: linux-nvme#3112 Signed-off-by: Jesse Taube <jtaubepe@redhat.com>

Mr-Bossman force-pushed the dev/jesse/pdc-default branch from 80a57f9 to ecb174c Compare June 29, 2026 15:13

Mr-Bossman changed the title ~~DRAFT: Use persistent discovery controller by default~~ Use persistent discovery controller by default Jun 29, 2026

igaw reviewed Jun 30, 2026

View reviewed changes

Comment thread fabrics.c

igaw reviewed Jul 1, 2026

View reviewed changes

hreinecke mentioned this pull request Jul 2, 2026

Make persistent discovery controller configurable for the subsystem #3536

Open

Uh oh!

Conversation

Mr-Bossman commented Jun 22, 2026

Uh oh!

igaw commented Jun 24, 2026

Uh oh!

Mr-Bossman commented Jun 24, 2026

Uh oh!

igaw commented Jun 24, 2026

Uh oh!

Mr-Bossman commented Jun 24, 2026

Uh oh!

hreinecke commented Jun 25, 2026

Uh oh!

igaw commented Jun 25, 2026

Uh oh!

Mr-Bossman commented Jun 25, 2026

Uh oh!

hreinecke commented Jun 26, 2026

Uh oh!

Mr-Bossman commented Jun 29, 2026

Uh oh!

igaw commented Jun 29, 2026

Uh oh!

hreinecke commented Jun 30, 2026

Uh oh!

Uh oh!

Mr-Bossman commented Jun 30, 2026

Uh oh!

Mr-Bossman commented Jun 30, 2026

Uh oh!

igaw Jul 1, 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!

igaw Jul 1, 2026

Choose a reason for hiding this comment

Uh oh!

igaw commented Jul 1, 2026

Uh oh!

hreinecke commented Jul 2, 2026

Uh oh!

igaw commented Jul 2, 2026

Uh oh!

hreinecke commented Jul 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

igaw Jul 1, 2026 •

edited

Loading