Nvmet discovery implementation. Questions and a patch / proposal.

[Hannes Reinecke]

On 3/13/22 22:25, Sagi Grimberg wrote:
> 
> 
> On 3/1/22 09:05, Hannes Reinecke wrote:
>> 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 :-)
> 
> Why aren't we just creating the discovery subsystem as a static configfs
> subsystems and just link it to different ports?
> 
We could be doing that, but that would imply a change in behaviour.
Currently we have one distinct discovery controller per subsystem.
Switching to a static configfs discovery subsystem would inevitably 
result in a _single_ discovery subsystem for all configured subsystems.
Not that I'm against it (quite the contrary), but it should be mentioned.

> The port listener on first linkage is a different matter afaict, don't
> see a strong argument to make the port listener without any subsystems
> exported anyways... maybe if discovery log change event is being used...

Actually, we can just leave the existing mechanism as-is; ports would 
need to be linked explicitly to the discovery subsystem.
Reasoning: Not every port needs to present a discovery subsystem, and we 
might want to have ports which will _only_ present the discovery subsystem.

Hmm. Should be pretty easy to code ...

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