Nvmet discovery implementation. Questions and a patch / proposal.

[Mark Ruijter]

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

This makes sense to me since it's a flexible solution.

Maybe slightly off-topic but somewhat related:
Let's assume that a user wants to temporarily disable initiators to use a certain target.
For example, when working on a switch or cabling.
 
The older (IET/SCST and so on) SCSI targets allow to enable/disable a target port using a single attribute.
For example:
/sys/kernel/scst_tgt/targets/qla2x00t/21:00:00:0e:1e:29:a0:70 # cat enabled 
1

The current nvmet target implementation requires to unlink all the subsystems when you merely want to disable a port temporarily.
Wouldn't it be much easier if enabling / disabling a port is also possible without unlinking the subsystems?
The second patch I provided allows this without breaking enabling the port when the first subsystem is linked.

Thanks,

Mark

On 14/03/2022, 08:00, "Hannes Reinecke" <hare at suse.de> wrote:

    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