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

[Hannes Reinecke]

On 4/7/22 17:49, Christoph Hellwig wrote:
> On Thu, Apr 07, 2022 at 12:48:07PM +0200, Hannes Reinecke wrote:
>> Add a 'disc_subsys' pointer to each port to specify which discovery
>> subsystem to use.
>> The pointer is set when a discovery subsystem is linked into a port,
>> and reset to the original, built-in discovery subsystem if that link
>> is removed.
> 
> This doesn't really make much sense stanadlone without the next
> patch, so I'd be tempted to say they should be merged.
> 

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.

>> +++ b/drivers/nvme/target/core.c
>> @@ -1496,9 +1496,9 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
>>   		return NULL;
>>   
>>   	if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn)) {
>> -		if (!kref_get_unless_zero(&nvmet_disc_subsys->ref))
>> -			return NULL;
>> -		return nvmet_disc_subsys;
>> +		if (!kref_get_unless_zero(&port->disc_subsys->ref))
>> +				return NULL;
>> +		return port->disc_subsys;
> 
> 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.

>> +	if (req->port->disc_subsys == nvmet_disc_subsys)
>> +		entries++;
> 
>> +	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.

Linking it into the port->subsystems list seemed overly complicated for 
no real gain.

At the same time using the ->disc_subsys link directly here would have 
meant that I have to filter out that subsystem in the 
list_for_each_loop(); also an awkward choice.

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