[PATCH 3/3] nvmet: include all configured ports in the discovery log page

[Hannes Reinecke]

On 4/12/22 12:49, Sagi Grimberg wrote:
> 
>>>> When a per-port discovery subsystem is used we should include
>>>> all configured ports in the discovery log page, not just that one
>>>> through which the controller was connected.
>>>>
>>>> - /
>>>>    o- ports
>>>>    | o- 1 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=8009]
>>>>    | | o- subsystems
>>>>    | |   o- nqn.discovery
>>>>    | o- 2 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4420]
>>>>    | | o- subsystems
>>>>    | |   o- nqn.discovery
>>>>    | |   o- nqn.io-1
>>>>    | o- 3 .. [trtype=tcp, traddr=127.0.0.1, trsvcid=4421]
>>>>    |   o- subsystems
>>>>    |     o- nqn.io-2
>>>>    o- subsystems
>>>>      o- nqn.discovery
>>>>      o- nqn.io-1
>>>>      | o- namespaces
>>>>      o- nqn.io-2
>>>>        o- namespaces
>>>>
>>>> A discovery on port 8009 will return information about
>>>> - nqn.discovery at port 8009
>>>> - nqn.discovery at port 4420
>>>> - nqn.io-1 at port 4420
>>>> A discovery on port 4420 will return the same information.
>>>> A discovery on port 4421 will return information about
>>>> - standard discovery subsystem on port 4421
>>>> - nqn.io-2 on port 4421
>>>
>>> This is a different functionality than supporting unique discovery
>>> NQNs.
>>>
>>
>> Yes.
>>
>>> If I want in your example to use a unique discovery subsystem but to
>>> report only on the port local I have to use two different unique
>>> discovery subsystems. Which is different than the standard discovery
>>> subsystem, why?
>>>
>> Because this was a logical step from the way I've implemented unique 
>> discovery subsystems.
>>
>> As discovery subsystems (in my implementation) are treated like 
>> 'normal' subsystems, they should be able to be linked to ports. If 
>> they are not linked we would have to implement some magic on which 
>> ports this subsystem will show up, and also making it inconsistent 
>> with all other subsystems.
> 
> They can be linked to ports, but why do they need to return subsystems
> that are attached to all ports differently from the normal discovery
> subsystem?
> 
>>> I'd submit to you that IMO unique discovery subsystems should not behave
>>> differently than the standard discovery subsystem. Please explain why
>>> do you think that unique discovery subsystems should behave differently
>>> with respect to the content of the log page.
>>>
>> Well, arguably.
>> But I haven't been able to find a good way of keeping the original 
>> behaviour _and_ support unique discovery subsystems.
>>
>> Sure, one could implement some magic variable in configfs like 
>> discovery_nqn, but that would have to be in configfs root directory, 
>> and really doesn't match up with current configfs layout.
>> One could go with your suggestion of having a discovery_subsystem 
>> entry per port, but we're back to the issue which killed my original 
>> patch:
>> How do we ensure uniqueness?
>> Each NQN here _need_ to be validated with the existing (and future) 
>> I/O subsystem NQNs, as they need to be unique. That requires a lookup 
>> over all ports and all referenced subsystems.
>> And we have to figure out if we allow for duplicated discovery NQNs; 
>> one can find arguments for either side.
>>
>> That's why I came up with this approach.
> 
> I'm just talking about the content of the log page. You are making the
> functionality of unique discovery subsystems different from the standard
> one. I'd either make the unique return subsystems attached to the
> current port like the global one, or change the standard one to return
> on all ports, or make it explicitly configurable that you can turn on
> for each discovery subsys. But making the unique and the standard behave
> inherently different is not coherent.
> 
>>
>> And that's also why this is a separate patch, as it does expand 
>> existing functionality :-)
> 
> My issue is that its causing the unique disc-susbsys behavior diverge
> from the standard one...

Ah. Okay.
That is true.

But having the default setup reflected in configfs is hard, as then you 
would need to create links from the default discovery subsystem to each 
port (on port creation!).
The current configfs design simply doesn't allow you to do that, so 
attempting something like that would break compability.

Which is the big plus for this patchset; it doesn't change the user 
interface, and nvmetcli can be used as-is.

So sure, I can have a look to expose the default discovery subsystem, 
too. But that would require a module or configfs parameter, _and_ will 
require changes to nvmetcli.

Cheers,

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