Create NVMe PDC by default for each unique target port

[martin-gpy]

It would help to create an NVMe PDC (Persistent Discovery Controller) by default for each unique target port listed in the discovery log page data, provided of course the 'Explicit discovery connections' flag is set in the the log page data.

This would help the NVMe host to automatically react to discovery AENs raised whenever there is any change to the discovery log page data (like when a subsystem is added/removed, target ports created/onlined/disabled, etc.).

[igaw]

The default behavior can currently set at configuration time or/and overwritten during runtime via the config.json file.

I don't mind changing the configuration defaults but I 'd like to hear from other users if this is desirable.

@martin-gpy could you briefly describe the impact of this change, e.g. additional resource usage on host or target side.

@martin-belanger @johnmeneghini

[martin-gpy]

@martin-gpy could you briefly describe the impact of this change, e.g. additional resource usage on host or target side.

A PDC doesn't have I/O queues, so these are relatively lightweight in terms of resource usage on both the host and the target.

Secondly, the benefits of having a PDC (i.e. automatically react to any changes in the E2E config such as subsystem addition/removal, target port create/online/disable, etc.) far outweigh the overhead of maintaining such a PDC in the first place.

[johnmeneghini]

The default behavior can currently set at configuration time or/and overwritten during runtime via the config.json file.

With NVMe/FC this can be done by updating the vendor specific udev rules, that would provide a default behavior on a vendor by vendor basis. But I am not sure how a default could be implemented via the config.json file with NVMe/TCP. We don't really ship a default config.json file. So, unless I am wrong, a default config.json file is something that would have to be implemented by the distos...

This is something Red Hat is willing to support, especially with discovery controllers that support TP 8014. This type of discover log page is designed to help the host reduce the number of duplicate persistent discovery controllers.

Every record indicates via the SUBTYPE field if that record is describing the current Discovery Service,
referring to another Discovery Service, or if the record indicates an NVM subsystem composed of
controllers that may expose namespaces. A referral to another Discovery Service (i.e., SUBTYPE field
set to 01h) is a mechanism to find additional Discovery subsystems. An NVM subsystem entry (i.e.,
SUBTYPE field set to 02h) is a mechanism to find NVM subsystems that contain controllers that may
expose namespaces. An entry that describes the current Discovery Service (i.e., SUBTYPE field set to
03h) is a mechanism to find additional access information (e.g., other NVM subsystem ports) for the
current Discovery service. Referrals to another Discovery Service shall not be deeper than eight levels.

For entries that describe the current Discovery Service, the DUPRETINFO bit set to 1 in the Entry
Flags field indicates those ports that return duplicate information. A host need not retrieve the
Discovery Log Page from all NVM subsystem ports described by a Discovery Log Page Entry that
contains the DUPRETINFO bit set to 1 (refer to Figure 263), as each of those ports return the same
information. A host may retrieve the Discovery Log Page from multiple ports with the DUPRETINFO
bit set to 1 to act as redundant access to the same information. A host may choose to enable the
Discovery Log Page Change AEN on one of the connections associated with an entry with the
DUPRETINFO bit set to ‘1’. If the Discovery Log Page Change AEN is enabled on multiple ports
represented by entries with the DUPRETINFO bit set to ‘1’, then the host may process a Discovery Log
Page Change AEN for one connection as applying to all ports represented by entries that also have
the DUPRETINFO bit set to 1.

[igaw]

There is the compile default setting option. It's on or off per default without any addition config option.

The json config already has support for it on host level:

		"persistent_discovery_ctrl": {
			"description": "Enable/disable Persistent Discovery Controller",
			"type": "boolean"
		},

The connect/discovery commands don't have a command line argument yet. This should also be added if
this should be an opt-in thing.

@johnmeneghini so you would prefer to keep it off per default (compile option) and have it to opt-in during runtime?

[johnmeneghini]

There is the compile default setting option. It's on or off per default without any addition config option.

What's the name of the compile setting?

@johnmeneghini so you would prefer to keep it off per default (compile option) and have it to opt-in during runtime?

Let me try this out. I will test this with some of the storage arrays I have in the lab, and get back to you with my thoughts.

[igaw]

There is the compile default setting option. It's on or off per default without any addition config option.

What's the name of the compile setting?

It's pdc-enabled (maybe we should rename it while at it to persistent-discovery-controller or so?):

$ meson configure | grep pdc
  pdc-enabled                    false                            [True, False]                    set default Persistent Discovery Controllers behavior
$ meson setup .build --reconfigure -Dpdc-enabled=true
[...]
  Configuration
    git version       : "2.15-10-g6cfc467"
    pdc enabled       : true
[...]

@johnmeneghini so you would prefer to keep it off per default (compile option) and have it to opt-in during runtime?

Let me try this out. I will test this with some of the storage arrays I have in the lab, and get back to you with my thoughts.

Ah great, this would be excellent! I suggest you should use the last release 2.16 for this and not the current master. I am moving the code around and I am haven't tested all everything yet. So it's likely broken at the moment.

[martin-gpy]

But this pdc-enabled compile option would blindly enable PDCs across all available target ports, right?

Ideally we would want this default enabled only for those unique target ports that support Explicit Persistent Connection Support for Discovery (EPCSD) in the discovery log page data.