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

[Christoph Hellwig <hch@lst.de>]

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