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

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

[Max Gurtovoy]

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@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@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
-- 
2.18.1

[PATCH 2/3] nvme-rdma: implement existing_ctrl operation

[Max Gurtovoy]

The logic of checking matching connections moved to the core layer.
Implement the needed optional operation and remove the common logic.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/rdma.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index bbad26b82b56..79412e4bfe3e 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -2326,11 +2326,6 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 		}
 	}
 
-	if (!opts->duplicate_connect && nvme_rdma_existing_controller(opts)) {
-		ret = -EALREADY;
-		goto out_free_ctrl;
-	}
-
 	INIT_DELAYED_WORK(&ctrl->reconnect_work,
 			nvme_rdma_reconnect_ctrl_work);
 	INIT_WORK(&ctrl->err_work, nvme_rdma_error_recovery_work);
@@ -2390,6 +2385,7 @@ static struct nvmf_transport_ops nvme_rdma_transport = {
 			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
 			  NVMF_OPT_TOS,
 	.create_ctrl	= nvme_rdma_create_ctrl,
+	.existing_ctrl	= nvme_rdma_existing_controller,
 };
 
 static void nvme_rdma_remove_one(struct ib_device *ib_device, void *client_data)
-- 
2.18.1

[PATCH 3/3] nvme-tcp: implement existing_ctrl operation

[Max Gurtovoy]

The logic of checking matching connections moved to the core layer.
Implement the needed optional operation and remove the common logic.

Reviewed-by: Israel Rukshin <israelr@nvidia.com>
Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>
---
 drivers/nvme/host/tcp.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 8cedc1ef496c..233e6c3a85de 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -2608,11 +2608,6 @@ static struct nvme_ctrl *nvme_tcp_create_ctrl(struct device *dev,
 		}
 	}
 
-	if (!opts->duplicate_connect && nvme_tcp_existing_controller(opts)) {
-		ret = -EALREADY;
-		goto out_free_ctrl;
-	}
-
 	ctrl->queues = kcalloc(ctrl->ctrl.queue_count, sizeof(*ctrl->queues),
 				GFP_KERNEL);
 	if (!ctrl->queues) {
@@ -2666,6 +2661,7 @@ static struct nvmf_transport_ops nvme_tcp_transport = {
 			  NVMF_OPT_NR_WRITE_QUEUES | NVMF_OPT_NR_POLL_QUEUES |
 			  NVMF_OPT_TOS | NVMF_OPT_HOST_IFACE,
 	.create_ctrl	= nvme_tcp_create_ctrl,
+	.existing_ctrl	= nvme_tcp_existing_controller,
 };
 
 static int __init nvme_tcp_init_module(void)
-- 
2.18.1

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

[James Smart]

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@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@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@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.

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

-- james

Re: [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@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@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@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
>

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

[Christoph Hellwig]

> +	if (ops->existing_ctrl) {
> +		if (!opts->duplicate_connect && ops->existing_ctrl(opts)) {

Nitpick, but the logic would read easier at least to me as:

	if (!opts->duplicate_connect &&
            ops->existing_ctrl && ops->existing_ctrl(opts)) {

I can fix this up if we get some more positive reviews and this gets
applied.

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

[Chaitanya Kulkarni]

On 1/15/23 02:03, 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@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@nvidia.com>

The series looks good to me, did you run the blocktests for
tcp and rdma for this ?

-ck

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

[Sagi Grimberg]

> 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@nvidia.com>
> Signed-off-by: Max Gurtovoy <mgurtovoy@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?

This will eliminate the identical code from rdma/tcp and potentially
from fc?

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

[Max Gurtovoy]

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@nvidia.com>
>> Signed-off-by: Max Gurtovoy <mgurtovoy@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.

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

Re: [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@nvidia.com>
>>> Signed-off-by: Max Gurtovoy <mgurtovoy@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