Re: [PATCH 2/3] nvmet: per-port discovery subsystem

[Christoph Hellwig <hch@lst.de>]

On Thu, Apr 07, 2022 at 07:21:17PM +0200, Hannes Reinecke wrote:
> Sure. I just didn't want to make the patches too complicated initially.
>
>>>   	down_write(&nvmet_config_sem);
>>> +	if (subsys->type == NVME_NQN_CURR &&
>>> +	    port->disc_subsys != nvmet_disc_subsys) {
>>
>> Curious, would NULL not be a better encoding for the default discovery
>> subsystem?
>>
> Hmm. Sure, could do.

Looking at the rest of the patches it might not be a very good idea
after all.  So I'll let you decide.

>> This has an extra tab indent.  But: should we even redirect from the
>> well known discovery NQN for a configured discovery subsystem here?
>> If yes at least this needs a big fat comment explaining why we do it.
>>
> Yes. This is mandated by the spec
> (section 3.1.2.3 Discovery Controller):
> If the Discovery subsystem provides a unique NQN, then that Discovery 
> subsystem shall support both the unique subsystem NQN and the well-known 
> discovery service NQN.
>
> Will be adding a comment.

Thanks!

>>> +	if (req->port->disc_subsys == nvmet_disc_subsys) {
>>> +		nvmet_format_discovery_entry(hdr, req->port,
>>> +				nvmet_disc_subsys->subsysnqn,
>>> +				traddr, NVME_NQN_CURR, numrec);
>>> +		numrec++;
>>> +	}
>>
>> Why don't we report the current discovery subsystem for if it isn't
>> the well known one?
>
> Thing is, unique discovery subsytems are just 'normal' subsystems; the only 
> thing which is different is the type.
> The static discovery subsystem OTOH is special as it's not linked to the 
> normal subsystem list and consequently doesn't show up in the 
> port->subsystem list.
> Hence the extra entry here.

Ok. Please add a comment that the unique discovery controllers will be
handled later in the loop.