Re: [PATCH 6/7] nvme: Add connect option 'discovery'

[Hannes Reinecke <hare@suse.de>]

On 9/22/21 6:59 PM, James Smart wrote:
> On 9/21/2021 11:35 PM, Hannes Reinecke wrote:
>> Add a connect option 'discovery' to specify that the connection
>> should be made to a discovery controller, not a normal I/O controller.
>> With discovery controllers supporting unique subsystem NQNs we
>> cannot easily distinguish by the subsystem NQN if this should be
>> a discovery connection, but we need this information to blank out
>> options not supported by discovery controllers.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
>> ---
>>   drivers/nvme/host/core.c    | 7 +++++++
>>   drivers/nvme/host/fabrics.c | 6 +++++-
>>   drivers/nvme/host/fabrics.h | 1 +
>>   3 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>> index 1ed1b7be2812..ad01e2778cb6 100644
>> --- a/drivers/nvme/host/core.c
>> +++ b/drivers/nvme/host/core.c
>> @@ -2715,6 +2715,13 @@ static int nvme_init_subsystem(struct nvme_ctrl 
>> *ctrl, struct nvme_id_ctrl *id)
>>       else
>>           subsys->subtype = NVME_NQN_NVME;
>> +    if (nvme_discovery_ctrl(ctrl) && subsys->subtype != NVME_NQN_DISC) {
>> +        dev_err(ctrl->device,
>> +            "Subsystem %s is not a discovery controller",
>> +            subsys->subnqn);
>> +        kfree(subsys);
>> +        return -EINVAL;
>> +    }
>>       subsys->awupf = le16_to_cpu(id->awupf);
>>   #ifdef CONFIG_NVME_MULTIPATH
>>       subsys->iopolicy = NVME_IOPOLICY_NUMA;
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 668c6bb7a567..c5a2b71c5268 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -548,6 +548,7 @@ static const match_table_t opt_tokens = {
>>       { NVMF_OPT_NR_POLL_QUEUES,    "nr_poll_queues=%d"    },
>>       { NVMF_OPT_TOS,            "tos=%d"        },
>>       { NVMF_OPT_FAIL_FAST_TMO,    "fast_io_fail_tmo=%d"    },
>> +    { NVMF_OPT_DISCOVERY,        "discovery"        },
>>       { NVMF_OPT_ERR,            NULL            }
>>   };
>> @@ -823,6 +824,9 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>>               }
>>               opts->tos = token;
>>               break;
>> +        case NVMF_OPT_DISCOVERY:
>> +            opts->discovery_nqn = true;
>> +            break;
>>           default:
>>               pr_warn("unknown parameter or missing value '%s' in ctrl 
>> creation request\n",
>>                   p);
>> @@ -949,7 +953,7 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
>>   #define NVMF_ALLOWED_OPTS    (NVMF_OPT_QUEUE_SIZE | 
>> NVMF_OPT_NR_IO_QUEUES | \
>>                    NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
>>                    NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT |\
>> -                 NVMF_OPT_DISABLE_SQFLOW |\
>> +                 NVMF_OPT_DISABLE_SQFLOW | NVMF_OPT_DISCOVERY |\
>>                    NVMF_OPT_FAIL_FAST_TMO)
>>   static struct nvme_ctrl *
>> diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index a146cb903869..b61b666e10ec 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -67,6 +67,7 @@ enum {
>>       NVMF_OPT_TOS        = 1 << 19,
>>       NVMF_OPT_FAIL_FAST_TMO    = 1 << 20,
>>       NVMF_OPT_HOST_IFACE    = 1 << 21,
>> +    NVMF_OPT_DISCOVERY    = 1 << 22,
>>   };
>>   /**
>>
> 
> 
> I think you need to be patching __nvme_find_get_subsystem(), which looks 
> for subsystem name and opts out (allowing multiple connects to discovery 
> subsystems) if discovery controller nqn.
> 
> e.g. the loop matching subsys name (assuming discovery ctlr now has a 
> unique name) continues if subsys->subtype is NVME_NQN_DISC
> 
Ah. You spotted it.

That is actually by design.

The sole reason (from my perspective) for this condition is precisely 
because we cannot currently identify discovery subsystems. So we need to 
create individual discovery subsystems for each controller.
But with this patch we can, so we _can_ figure out if two controllers 
are attached to the same discovery subsystem.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@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

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme