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

[Christoph Hellwig <hch@lst.de>]

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.

>  	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?

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

> +	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?