[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guan Junxiong]

From: Junxiong Guan <guanjunxiong@huawei.com>

Current code has the defect that we will get more /dev/nvme#n#
generated in the /dev node if we execute the same connect command
with nvme-cli tools more than once. To fix it, this patch iterate
the allocated controller list to check if the same subsystem with
the same path of topology exists before create new controller. If
exists, just log and return. The path identifier is combined with
tranport type, transport address, target subsystem nqn and hostnqn.
---
 drivers/nvme/host/core.c   | 33 +++++++++++++++++++++++++++++++++
 drivers/nvme/host/nvme.h   |  1 +
 drivers/nvme/host/rdma.c   |  8 ++++++++
 drivers/nvme/target/loop.c |  6 ++++++
 4 files changed, 48 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 6dcd7a123b50..1d3a077cba78 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2318,6 +2318,39 @@ void nvme_put_ctrl(struct nvme_ctrl *ctrl)
 }
 EXPORT_SYMBOL_GPL(nvme_put_ctrl);
 
+bool nvme_is_ctrl_existed(const struct nvmf_ctrl_options *opts)
+{
+	struct nvme_ctrl *ctrl;
+	bool find = false;
+
+	spin_lock(&dev_list_lock);
+	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {
+		if(strcmp(ctrl->opts->transport, opts->transport))
+			continue;
+		if((opts->mask&NVMF_OPT_TRADDR) \
+				&& (ctrl->opts->traddr != NULL)) {
+			if(strcmp(ctrl->opts->traddr, opts->traddr))
+				continue;
+		}
+		if((opts->mask & NVMF_OPT_TRSVCID) \
+				&& (ctrl->opts->trsvcid != NULL)) {
+			if(strcmp(ctrl->opts->trsvcid, opts->trsvcid))
+				continue;
+		}
+		if(strcmp(ctrl->opts->subsysnqn, opts->subsysnqn))
+			continue;
+		if(strcmp(ctrl->opts->host->nqn, opts->host->nqn))
+			continue;
+		find = true;
+		break;
+	}
+	spin_unlock(&dev_list_lock);
+
+	return find;
+}
+EXPORT_SYMBOL_GPL(nvme_is_ctrl_existed);
+
+
 /*
  * Initialize a NVMe controller structures.  This needs to be called during
  * earliest initialization so that we have the initialized structured around
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 72dc98f6a6c6..07635522b691 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -261,6 +261,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
 int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
 int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
+bool nvme_is_ctrl_existed(const struct nvmf_ctrl_options *opts);
 int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
 		const struct nvme_ctrl_ops *ops, unsigned long quirks);
 void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 53b611f9ba5d..6696ed0fdd87 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1858,6 +1858,14 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 	bool changed;
 	char *port;
 
+	if(!(opts->mask & NVMF_OPT_TRSVCID))
+		opts->trsvcid = __stringify(NVME_RDMA_IP_PORT);
+	if(nvme_is_ctrl_existed(opts)) {
+		pr_err("connect to the same subsystem from the same path\n");
+		ret = -EBUSY;
+		return ERR_PTR(ret);
+	}
+
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl)
 		return ERR_PTR(-ENOMEM);
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index 8260ee1f8e48..1e0fa2397aa7 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -621,6 +621,12 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
 	bool changed;
 	int ret;
 
+	if(nvme_is_ctrl_existed(opts)) {
+		pr_err("connect to the same subsystem from the same path\n");
+		ret = -EBUSY;
+		return ERR_PTR(ret);
+	}
+
 	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
 	if (!ctrl)
 		return ERR_PTR(-ENOMEM);
-- 
2.11.1

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Christoph Hellwig]

On Mon, Apr 24, 2017@09:26:57PM +0800, Guan Junxiong wrote:
> From: Junxiong Guan <guanjunxiong at huawei.com>
> 
> Current code has the defect that we will get more /dev/nvme#n#
> generated in the /dev node if we execute the same connect command
> with nvme-cli tools more than once.

That's not a bug, it's a a feature.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guan Junxiong]

On 2017/4/24 22:58, Christoph Hellwig wrote:
> On Mon, Apr 24, 2017@09:26:57PM +0800, Guan Junxiong wrote:
>> From: Junxiong Guan <guanjunxiong at huawei.com>
>>
>> Current code has the defect that we will get more /dev/nvme#n#
>> generated in the /dev node if we execute the same connect command
>> with nvme-cli tools more than once.
> 
> That's not a bug, it's a a feature.
> 
> .
Hi ,Christoph:
   What's the benefits of this feature? If we execute the following cmd
10 times, it will generate 10 character and at least 20 block devices.

   nvme connect -r rdma -a 192.168.2.1 -n tgtnqn -q hostnqn
   10 times repeating, then list nvme devices:
   nvme list
   we get a flood of nvme devices now.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guan Junxiong]

On 2017/4/25 9:20, Guan Junxiong wrote:
> 
> Hi ,Christoph:
>    What's the benefits of this feature? If we execute the following cmd
> 10 times, it will generate 10 character and at least 10 block devices.
> 
>    nvme connect -r rdma -a 192.168.2.1 -n tgtnqn -q hostnqn
>    10 times repeating, then list nvme devices:
>    nvme list
>    we get a flood of nvme devices now.
> 
> 
> 
Hi, Christoph:
   I am looking forward to your response of this feature.
Thanks a lot.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guilherme G. Piccoli]

On 04/24/2017 10:26 AM, Guan Junxiong wrote:
> From: Junxiong Guan <guanjunxiong at huawei.com>
> 
> Current code has the defect that we will get more /dev/nvme#n#
> generated in the /dev node if we execute the same connect command
> with nvme-cli tools more than once. To fix it, this patch iterate
> the allocated controller list to check if the same subsystem with
> the same path of topology exists before create new controller. If
> exists, just log and return. The path identifier is combined with
> tranport type, transport address, target subsystem nqn and hostnqn.

Agree with you, and I'm curious too on why this is considered a feature
instead of a problematic behavior.

Minor suggestions to the patch:

1) On title: s/gain/again/g

2) s/with nvme-cli tools/with nvme-cli tool/g

3) s/tranport/transport/g

Thanks for the good fix!
Cheers,


Guilherme

> ---
>  drivers/nvme/host/core.c   | 33 +++++++++++++++++++++++++++++++++
>  drivers/nvme/host/nvme.h   |  1 +
>  drivers/nvme/host/rdma.c   |  8 ++++++++
>  drivers/nvme/target/loop.c |  6 ++++++
>  4 files changed, 48 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 6dcd7a123b50..1d3a077cba78 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2318,6 +2318,39 @@ void nvme_put_ctrl(struct nvme_ctrl *ctrl)
>  }
>  EXPORT_SYMBOL_GPL(nvme_put_ctrl);
> 
> +bool nvme_is_ctrl_existed(const struct nvmf_ctrl_options *opts)
> +{
> +	struct nvme_ctrl *ctrl;
> +	bool find = false;
> +
> +	spin_lock(&dev_list_lock);
> +	list_for_each_entry(ctrl, &nvme_ctrl_list, node) {
> +		if(strcmp(ctrl->opts->transport, opts->transport))
> +			continue;
> +		if((opts->mask&NVMF_OPT_TRADDR) \
> +				&& (ctrl->opts->traddr != NULL)) {
> +			if(strcmp(ctrl->opts->traddr, opts->traddr))
> +				continue;
> +		}
> +		if((opts->mask & NVMF_OPT_TRSVCID) \
> +				&& (ctrl->opts->trsvcid != NULL)) {
> +			if(strcmp(ctrl->opts->trsvcid, opts->trsvcid))
> +				continue;
> +		}
> +		if(strcmp(ctrl->opts->subsysnqn, opts->subsysnqn))
> +			continue;
> +		if(strcmp(ctrl->opts->host->nqn, opts->host->nqn))
> +			continue;
> +		find = true;
> +		break;
> +	}
> +	spin_unlock(&dev_list_lock);
> +
> +	return find;
> +}
> +EXPORT_SYMBOL_GPL(nvme_is_ctrl_existed);
> +
> +
>  /*
>   * Initialize a NVMe controller structures.  This needs to be called during
>   * earliest initialization so that we have the initialized structured around
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 72dc98f6a6c6..07635522b691 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -261,6 +261,7 @@ bool nvme_change_ctrl_state(struct nvme_ctrl *ctrl,
>  int nvme_disable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
>  int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap);
>  int nvme_shutdown_ctrl(struct nvme_ctrl *ctrl);
> +bool nvme_is_ctrl_existed(const struct nvmf_ctrl_options *opts);
>  int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
>  		const struct nvme_ctrl_ops *ops, unsigned long quirks);
>  void nvme_uninit_ctrl(struct nvme_ctrl *ctrl);
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index 53b611f9ba5d..6696ed0fdd87 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -1858,6 +1858,14 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
>  	bool changed;
>  	char *port;
> 
> +	if(!(opts->mask & NVMF_OPT_TRSVCID))
> +		opts->trsvcid = __stringify(NVME_RDMA_IP_PORT);
> +	if(nvme_is_ctrl_existed(opts)) {
> +		pr_err("connect to the same subsystem from the same path\n");
> +		ret = -EBUSY;
> +		return ERR_PTR(ret);
> +	}
> +
>  	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>  	if (!ctrl)
>  		return ERR_PTR(-ENOMEM);
> diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
> index 8260ee1f8e48..1e0fa2397aa7 100644
> --- a/drivers/nvme/target/loop.c
> +++ b/drivers/nvme/target/loop.c
> @@ -621,6 +621,12 @@ static struct nvme_ctrl *nvme_loop_create_ctrl(struct device *dev,
>  	bool changed;
>  	int ret;
> 
> +	if(nvme_is_ctrl_existed(opts)) {
> +		pr_err("connect to the same subsystem from the same path\n");
> +		ret = -EBUSY;
> +		return ERR_PTR(ret);
> +	}
> +
>  	ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>  	if (!ctrl)
>  		return ERR_PTR(-ENOMEM);
> 

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Christoph Hellwig]

On Tue, Apr 25, 2017@09:20:00AM +0800, Guan Junxiong wrote:
> Hi ,Christoph:
>    What's the benefits of this feature? If we execute the following cmd
> 10 times, it will generate 10 character and at least 20 block devices.

The number of block devices will depend on the number of namespaces
configured, but yes, this is expected.  The NVMe architecture allows
for multiple connections from a given host to a subsystem.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guan Junxiong]

On 2017/4/27 14:40, Christoph Hellwig wrote:
> On Tue, Apr 25, 2017@09:20:00AM +0800, Guan Junxiong wrote:
>> Hi ,Christoph:
>>    What's the benefits of this feature? If we execute the following cmd
>> 10 times, it will generate 10 character and at least 20 block devices.
> 
> The number of block devices will depend on the number of namespaces
> configured, but yes, this is expected.  The NVMe architecture allows
> for multiple connections from a given host to a subsystem.
> 
> .
> 
Hi, Christoph,Thanks for your reply!

Currently, the number of block devices will depend on both the number of
namespaces configured in a subsystem and the number of connections created by
connect command.

In addition , I still have the suggestion:
Can this feature or problematic behavior be adjusted to prevent generating
another block devices when a new connection is created from a given host to
a subsystem via the same patho<\x1f Maybe a character device (controller) or
block device (namespaces) can represent more connections given the same path.

Looking forward for you response. Thanks.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guan Junxiong]

On 2017/4/26 23:33, Guilherme G. Piccoli wrote:
> On 04/24/2017 10:26 AM, Guan Junxiong wrote:
>> From: Junxiong Guan <guanjunxiong at huawei.com>

> Agree with you, and I'm curious too on why this is considered a feature
> instead of a problematic behavior.
> 
> Minor suggestions to the patch:
> 
> 1) On title: s/gain/again/g
> 
> 2) s/with nvme-cli tools/with nvme-cli tool/g
> 
> 3) s/tranport/transport/g
> 
> Thanks for the good fix!
> Cheers,
> 
> 
> Guilherme

Hi,Guilherme:
   Thanks for your review. I will send next modified version of patch according
your suggestion if Christoph Hellwig confirms this is really needed to be improved.

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Guilherme G. Piccoli]

On 04/27/2017 04:21 AM, Guan Junxiong wrote:
> 
> 
> On 2017/4/26 23:33, Guilherme G. Piccoli wrote:
>> On 04/24/2017 10:26 AM, Guan Junxiong wrote:
>>> From: Junxiong Guan <guanjunxiong at huawei.com>
> 
>> Agree with you, and I'm curious too on why this is considered a feature
>> instead of a problematic behavior.
>>
>> Minor suggestions to the patch:
>>
>> 1) On title: s/gain/again/g
>>
>> 2) s/with nvme-cli tools/with nvme-cli tool/g
>>
>> 3) s/tranport/transport/g
>>
>> Thanks for the good fix!
>> Cheers,
>>
>>
>> Guilherme
> 
> Hi,Guilherme:
>    Thanks for your review. I will send next modified version of patch according
> your suggestion if Christoph Hellwig confirms this is really needed to be improved.
> 

Great, thank you!

[PATCH] nvme: avoid connecting to the same subsystem from the same path gain.

[Narasimhan V]

On 2017-04-27 12:46, Guan Junxiong wrote:
> On 2017/4/27 14:40, Christoph Hellwig wrote:
>> On Tue, Apr 25, 2017@09:20:00AM +0800, Guan Junxiong wrote:
>>> Hi ,Christoph:
>>>    What's the benefits of this feature? If we execute the following 
>>> cmd
>>> 10 times, it will generate 10 character and at least 20 block 
>>> devices.
>> 
>> The number of block devices will depend on the number of namespaces
>> configured, but yes, this is expected.  The NVMe architecture allows
>> for multiple connections from a given host to a subsystem.
>> 
>> .
>> 
> Hi, Christoph,Thanks for your reply!
> 
> Currently, the number of block devices will depend on both the number 
> of
> namespaces configured in a subsystem and the number of connections 
> created by
> connect command.
> 
> In addition , I still have the suggestion:
> Can this feature or problematic behavior be adjusted to prevent 
> generating
> another block devices when a new connection is created from a given 
> host to
> a subsystem via the same patho<\x1f Maybe a character device (controller) 
> or
> block device (namespaces) can represent more connections given the same 
> path.
> 
> Looking forward for you response. Thanks.

Hellwig,

I think this is more of a confusing/problematic behaviour than a 
feature, as stated.

We might connect to same subsystem again, and see a different nvme 
subsystem, block devices.
Tools like fdisk will discover duplicate block devices as different 
ones.
So, we might think it is a different subsystem. And thus, might have 
data corruption issues.

Looking forward for your clarification in such scenarios, and probably 
your take on this
being feature vs issue.

-- 
Regards
Narasimhan V