[PATCH 1/3] nvmet: check for subsystem type in nvmet_find_get_subsys()

[Hannes Reinecke]

On 4/5/22 07:45, Christoph Hellwig wrote:
> 
> 
> On Thu, Mar 17, 2022 at 03:26:32PM +0100, Hannes Reinecke wrote:
>> When looking for a subsystem we have two ways of finding the
>> subsystem: either looking for the subsystem NQN itself, or check
>> the type of the subsystem when looking for a discovery controller.
>> This patch implements this check, and also moves the check for
>> the static discovery controller to the end such that we can
>> return unique discovery controllers.
>>
>> Signed-off-by: Hannes Reinecke <hare at suse.de>
>> ---
>>   drivers/nvme/target/core.c | 20 ++++++++++++++------
>>   1 file changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
>> index b0dc6230d1b9..83eba511d098 100644
>> --- a/drivers/nvme/target/core.c
>> +++ b/drivers/nvme/target/core.c
>> @@ -1508,12 +1508,6 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
>>   	if (!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;
>> -	}
>> -
>>   	down_read(&nvmet_config_sem);
>>   	list_for_each_entry(p, &port->subsystems, entry) {
>>   		if (!strncmp(p->subsys->subsysnqn, subsysnqn,
>> @@ -1523,8 +1517,22 @@ static struct nvmet_subsys *nvmet_find_get_subsys(struct nvmet_port *port,
>>   			up_read(&nvmet_config_sem);
>>   			return p->subsys;
>>   		}
>> +		if (!strcmp(NVME_DISC_SUBSYS_NAME, subsysnqn) &&
>> +		    nvmet_is_disc_subsys(p->subsys)) {
> 
> I don't get this.  The strcmp is on the passed in subqnen that we are
> looking for.  So with this patch we now do the fairly expensive string
> compare for every subsystem in the loop, and the just retuns the first
> discovery subsystems when we look for the well known discovery
> subsystem.  As-is there is only one, so this is pointless.  If we add
> discovery subsystems with different names (as I assume the later patches
> do), this changes beahvior in a rather unexpected way.

Will have to check; might be that this is a left-over from the previous 
attempt of handling discovery subsystems just like any other subsystem.

However, this is just part of a larger patchset to support unique 
discovery controller NQNs. And the overarching question here is:

Do we want to support unique discovery controller NQNs in nvmet?

Previously there was a rather strict policy of implementing only the 
bare necessities in nvmet, and unique discovery controller NQNs is 
arguably not a necessary thing.
So the whole discussion gets pretty pointless if we decide _not_ to 
support it after all.

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