Nvmet discovery implementation. Questions and a patch / proposal.

[Hannes Reinecke]

On 2/28/22 17:39, Mark Ruijter wrote:
> 
> The current discovery implementation only works after a subsystem has been created and has been linked to a port.
> As a result, 'nvme discover' from an initiator will only show a discovery device after the first subsystem has
> been exported.
> 
> The patch that I propose adds these features:
> A. Make discovery work immediately after the target port has been configured, without needing to configure
>    subsystems first.
> B. Move the discovery_nqn configuration option to the port instead of having the same configuration option
>    duplicated in every subsystem.
> C. Allow to enable and disable a target port without needing to unlink target subsystems.
> 
The current nvmet discovery implementation was designed to the original 
NVMe spec, where a discovery controller could only return informations 
about the attached subsystems.
But with the recent changes to the spec a discovery controller can now 
return information about itself (ie which port it's running on), and so
it does makes sense to have it enabled even with no subsystems attached.

But: the current implementation only has a _single_ discovery subsystem 
(nvmet_disc_subsys), which is shared across all configured subsystems.
And there's a filter when generating the discovery log page such that 
only the information for the current subsystem is returned (cf 
nvmet_execute_disc_get_log_page()).
So in effect each subsystem has its own discovery subsystem.

Moving the discovery nqn to the port will only confuse matters, as then 
it's quite unclear what will happen if you change the discovery 
subsystem nqn on a per-port basis; with the current design you would
change it for _all_ discovery subsystem ...

So if you want to do something like this, you'd need to change the 
implementation to have a discovery subsystem per port, and also change 
the logic in drivers/nvme/target/discovery.c such that the discovery
subsystems will return informations for _all_ configured subsystems.

It's actually something I wanted to do, too, as I didn't like the 
current implementation :-)

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare at suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer