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

[James Smart]

On 1/23/2023 5:34 AM, Max Gurtovoy wrote:
> Hi Sagi,
> 
> On 23/01/2023 14:33, Sagi Grimberg 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
>>
>> Maybe do nvmf_existing_ctrl() that iterates with device_for_each_child 
>> and filters by transport?
>>
> do you propose some incremental logic to this series or to change some 
> logic in the existing patches ?
> 
> 
>> This will eliminate the identical code from rdma/tcp and potentially
>> from fc?
> 
> We agreed that FC will use this common code after some cleanup/changes.

no - we agreed FC is better off the way it is and not using the common 
routine. Adding the routine would increase code and add locking for no 
benefit as the same searching has to be repeated in init_ctrl for checks 
other than opts.


> 
> We can't hold improvements to the subsystem if some specific transport 
> has to do some preparations/re-designs to fit common code.
> 

Only way to make this meaningfully common is to have a different list 
mechanism to search through existing controllers.

If we can globalize the controllers so there's a list of them off a 
transport object then doing something like Sagi suggests works, and no 
need for a callout.

-- james