[PATCH 1/3] nvme-fabrics: introduce existing_ctrl operation

[Max Gurtovoy]

On 15/01/2023 21:51, James Smart wrote:
> On 1/15/2023 2:03 AM, Max Gurtovoy wrote:
>> The check if the connection request matches an existing controller is
>> duplicated in several transports (such as RDMA and TCP). Move it to
>> common code as optional operation.
>>
>> Also, this check can be done before creating the controller to simplify
>> the flow.
>>
>> Reviewed-by: Israel Rukshin <israelr at nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy at nvidia.com>
>> ---
>>   drivers/nvme/host/fabrics.c | 7 +++++++
>>   drivers/nvme/host/fabrics.h | 3 +++
>>   2 files changed, 10 insertions(+)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index ce27276f552d..3d5035331b9f 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -1100,6 +1100,13 @@ nvmf_create_ctrl(struct device *dev, const 
>> char *buf)
>>       if (ret)
>>           goto out_module_put;
>>   +    if (ops->existing_ctrl) {
>> +        if (!opts->duplicate_connect && ops->existing_ctrl(opts)) {
>> +            ret = -EALREADY;
>> +            goto out_module_put;
>> +        }
>> +    }
>> +
>>       ctrl = ops->create_ctrl(dev, opts);
>>       if (IS_ERR(ctrl)) {
>>           ret = PTR_ERR(ctrl); > diff --git 
>> a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
>> index a6e22116e139..587b92575a9b 100644
>> --- a/drivers/nvme/host/fabrics.h
>> +++ b/drivers/nvme/host/fabrics.h
>> @@ -153,6 +153,8 @@ struct nvmf_ctrl_options {
>>    *            that would go into starting up that fabric
>>    *            for the purpose of conneciton to an NVMe controller
>>    *            using that fabric technology.
>> + * @existing_ctrl():    optional function pointer that checks if a 
>> connection
>> + *             request matches an existing controller.
>>    *
>>    * Notes:
>>    *    1. At minimum, 'required_opts' and 'allowed_opts' should
>> @@ -170,6 +172,7 @@ struct nvmf_transport_ops {
>>       int            allowed_opts;
>>       struct nvme_ctrl    *(*create_ctrl)(struct device *dev,
>>                       struct nvmf_ctrl_options *opts);
>> +    bool            (*existing_ctrl)(struct nvmf_ctrl_options *opts);
>>   };
>>     static inline bool
>
> Reviewed-by: James Smart <jsmart2021 at gmail.com>
>
> A bit disheartening we're "commonizing" in manner that FC can't 
> participate.   I looked at FC supporting this, and although it could, 
> it's a bunch of code duplication, more code than leaving rdma/tcp code 
> in w/o optimization, and FC still has to repeat what it does in 
> controller create as there are other checks to be made. It's more 
> optimal to leave things as is on FC.

I tried to make this change to be applicable to FC as well but I wasn't 
able to figure it out so I didn't add changes there.

As you mentioned, it's better to leave things there as is for now.

I don't see any reason not to add the FC as an incremental patch to this 
series.

>
> Max: mind pinging me ahead of time when creating these common items so 
> we can get the 3rd transport participating if possible ?

Sure.

I think that a lot of logic can be moved from the transports to the core 
and I did an attempt in the past to do so.

But we need to make some preparations in the FC code so it will be able 
to enjoy it as well.

>
> -- james
>