linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] nvmet: make ctrl-id configurable
@ 2019-11-03 18:13 Chaitanya Kulkarni
  2019-11-03 18:15 ` Chaitanya Kulkarni
  2019-11-04  9:36 ` Max Gurtovoy
  0 siblings, 2 replies; 16+ messages in thread
From: Chaitanya Kulkarni @ 2019-11-03 18:13 UTC (permalink / raw)
  To: linux-nvme; +Cc: Chaitanya Kulkarni

This patch adds a new target subsys attribute which allows user to
optionally specify target controller which then used in the
nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.

When new attribute is not specified target will fall back to original
cntlid calculation method.

Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
Hi Mark,

This patch allows user to set the contoller ID through configfs
which is target susbsy attribute.

Can you please take a look and provide feedback ? so that we can get
it to upstream ? I'm planning to send this on Sunday.

-Regards,
Chaitanya
---
 drivers/nvme/target/configfs.c | 23 +++++++++++++++++++++++
 drivers/nvme/target/core.c     | 23 +++++++++++++++--------
 drivers/nvme/target/nvmet.h    |  1 +
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a45bd3b..83391e54ed12 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -862,10 +862,33 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
+					     char *page)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
+}
+
+static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
+					  const char *page, size_t count)
+{
+	struct nvmet_subsys *subsys = to_subsys(item);
+
+	down_write(&nvmet_config_sem);
+	/* should this be %x ? */
+	sscanf(page, "%hu\n", &subsys->id);
+	up_write(&nvmet_config_sem);
+
+	return count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_id);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
+	&nvmet_subsys_attr_attr_id,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 28438b833c1b..2396215a23c9 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1267,13 +1267,18 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 	if (!ctrl->sqs)
 		goto out_free_cqs;
 
-	ret = ida_simple_get(&cntlid_ida,
-			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
-			     GFP_KERNEL);
-	if (ret < 0) {
-		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
-		goto out_free_sqs;
-	}
+	if (!subsys->id) {
+		ret = ida_simple_get(&cntlid_ida,
+				NVME_CNTLID_MIN, NVME_CNTLID_MAX,
+				GFP_KERNEL);
+		if (ret < 0) {
+			status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
+			goto out_free_sqs;
+		}
+		ctrl->cntlid = ret;
+	} else
+		ret = subsys->id;
+
 	ctrl->cntlid = ret;
 
 	ctrl->ops = req->ops;
@@ -1330,7 +1335,8 @@ static void nvmet_ctrl_free(struct kref *ref)
 	flush_work(&ctrl->async_event_work);
 	cancel_work_sync(&ctrl->fatal_err_work);
 
-	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
+	if (!subsys->id)
+		ida_simple_remove(&cntlid_ida, ctrl->cntlid);
 
 	kfree(ctrl->sqs);
 	kfree(ctrl->cqs);
@@ -1416,6 +1422,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		kfree(subsys);
 		return ERR_PTR(-ENOMEM);
 	}
+	subsys->id = 0;
 
 	kref_init(&subsys->ref);
 
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 46df45e837c9..a3d3471bdf2d 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -211,6 +211,7 @@ struct nvmet_subsys {
 	struct list_head	namespaces;
 	unsigned int		nr_namespaces;
 	unsigned int		max_nsid;
+	u16			id;
 
 	struct list_head	ctrls;
 
-- 
2.22.1


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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-03 18:13 [PATCH] nvmet: make ctrl-id configurable Chaitanya Kulkarni
@ 2019-11-03 18:15 ` Chaitanya Kulkarni
  2019-11-04  9:36 ` Max Gurtovoy
  1 sibling, 0 replies; 16+ messages in thread
From: Chaitanya Kulkarni @ 2019-11-03 18:15 UTC (permalink / raw)
  To: linux-nvme; +Cc: MRuijter

(+MRuijter@onestopsystems.com).

On 11/03/2019 10:13 AM, Chaitanya Kulkarni wrote:
> This patch adds a new target subsys attribute which allows user to
> optionally specify target controller which then used in the
> nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.
>
> When new attribute is not specified target will fall back to original
> cntlid calculation method.
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
> Hi Mark,
>
> This patch allows user to set the contoller ID through configfs
> which is target susbsy attribute.
>
> Can you please take a look and provide feedback ? so that we can get
> it to upstream ? I'm planning to send this on Sunday.
>
> -Regards,
> Chaitanya
> ---
>   drivers/nvme/target/configfs.c | 23 +++++++++++++++++++++++
>   drivers/nvme/target/core.c     | 23 +++++++++++++++--------
>   drivers/nvme/target/nvmet.h    |  1 +
>   3 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 98613a45bd3b..83391e54ed12 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -862,10 +862,33 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>   }
>   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>
> +static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
> +					     char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
> +}
> +
> +static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
> +					  const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	down_write(&nvmet_config_sem);
> +	/* should this be %x ? */
> +	sscanf(page, "%hu\n", &subsys->id);
> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_id);
> +
>   static struct configfs_attribute *nvmet_subsys_attrs[] = {
>   	&nvmet_subsys_attr_attr_allow_any_host,
>   	&nvmet_subsys_attr_attr_version,
>   	&nvmet_subsys_attr_attr_serial,
> +	&nvmet_subsys_attr_attr_id,
>   	NULL,
>   };
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 28438b833c1b..2396215a23c9 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1267,13 +1267,18 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
>   	if (!ctrl->sqs)
>   		goto out_free_cqs;
>
> -	ret = ida_simple_get(&cntlid_ida,
> -			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
> -			     GFP_KERNEL);
> -	if (ret < 0) {
> -		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
> -		goto out_free_sqs;
> -	}
> +	if (!subsys->id) {
> +		ret = ida_simple_get(&cntlid_ida,
> +				NVME_CNTLID_MIN, NVME_CNTLID_MAX,
> +				GFP_KERNEL);
> +		if (ret < 0) {
> +			status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
> +			goto out_free_sqs;
> +		}
> +		ctrl->cntlid = ret;
> +	} else
> +		ret = subsys->id;
> +
>   	ctrl->cntlid = ret;
>
>   	ctrl->ops = req->ops;
> @@ -1330,7 +1335,8 @@ static void nvmet_ctrl_free(struct kref *ref)
>   	flush_work(&ctrl->async_event_work);
>   	cancel_work_sync(&ctrl->fatal_err_work);
>
> -	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
> +	if (!subsys->id)
> +		ida_simple_remove(&cntlid_ida, ctrl->cntlid);
>
>   	kfree(ctrl->sqs);
>   	kfree(ctrl->cqs);
> @@ -1416,6 +1422,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
>   		kfree(subsys);
>   		return ERR_PTR(-ENOMEM);
>   	}
> +	subsys->id = 0;
>
>   	kref_init(&subsys->ref);
>
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 46df45e837c9..a3d3471bdf2d 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -211,6 +211,7 @@ struct nvmet_subsys {
>   	struct list_head	namespaces;
>   	unsigned int		nr_namespaces;
>   	unsigned int		max_nsid;
> +	u16			id;
>
>   	struct list_head	ctrls;
>
>


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-03 18:13 [PATCH] nvmet: make ctrl-id configurable Chaitanya Kulkarni
  2019-11-03 18:15 ` Chaitanya Kulkarni
@ 2019-11-04  9:36 ` Max Gurtovoy
  2019-11-04 10:45   ` Mark Ruijter
  1 sibling, 1 reply; 16+ messages in thread
From: Max Gurtovoy @ 2019-11-04  9:36 UTC (permalink / raw)
  To: Chaitanya Kulkarni, linux-nvme


On 11/3/2019 8:13 PM, Chaitanya Kulkarni wrote:
> This patch adds a new target subsys attribute which allows user to
> optionally specify target controller which then used in the
> nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.

hi,

can you explain why we should give the user the ability to control this ?

In this case you will have multiple controllers with the same id, and 
I'm not sure that we want that.

Did you try to connect to it with -D (duplication) flag ?

>
> When new attribute is not specified target will fall back to original
> cntlid calculation method.
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
> Hi Mark,
>
> This patch allows user to set the contoller ID through configfs
> which is target susbsy attribute.
>
> Can you please take a look and provide feedback ? so that we can get
> it to upstream ? I'm planning to send this on Sunday.
>
> -Regards,
> Chaitanya
> ---
>   drivers/nvme/target/configfs.c | 23 +++++++++++++++++++++++
>   drivers/nvme/target/core.c     | 23 +++++++++++++++--------
>   drivers/nvme/target/nvmet.h    |  1 +
>   3 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 98613a45bd3b..83391e54ed12 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -862,10 +862,33 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>   }
>   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>   
> +static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
> +					     char *page)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
> +}
> +
> +static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
> +					  const char *page, size_t count)
> +{
> +	struct nvmet_subsys *subsys = to_subsys(item);
> +
> +	down_write(&nvmet_config_sem);
> +	/* should this be %x ? */
> +	sscanf(page, "%hu\n", &subsys->id);
> +	up_write(&nvmet_config_sem);
> +
> +	return count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_id);
> +
>   static struct configfs_attribute *nvmet_subsys_attrs[] = {
>   	&nvmet_subsys_attr_attr_allow_any_host,
>   	&nvmet_subsys_attr_attr_version,
>   	&nvmet_subsys_attr_attr_serial,
> +	&nvmet_subsys_attr_attr_id,
>   	NULL,
>   };
>   
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 28438b833c1b..2396215a23c9 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1267,13 +1267,18 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
>   	if (!ctrl->sqs)
>   		goto out_free_cqs;
>   
> -	ret = ida_simple_get(&cntlid_ida,
> -			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
> -			     GFP_KERNEL);
> -	if (ret < 0) {
> -		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
> -		goto out_free_sqs;
> -	}
> +	if (!subsys->id) {
> +		ret = ida_simple_get(&cntlid_ida,
> +				NVME_CNTLID_MIN, NVME_CNTLID_MAX,
> +				GFP_KERNEL);
> +		if (ret < 0) {
> +			status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
> +			goto out_free_sqs;
> +		}
> +		ctrl->cntlid = ret;
> +	} else
> +		ret = subsys->id;
> +
>   	ctrl->cntlid = ret;
>   
>   	ctrl->ops = req->ops;
> @@ -1330,7 +1335,8 @@ static void nvmet_ctrl_free(struct kref *ref)
>   	flush_work(&ctrl->async_event_work);
>   	cancel_work_sync(&ctrl->fatal_err_work);
>   
> -	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
> +	if (!subsys->id)
> +		ida_simple_remove(&cntlid_ida, ctrl->cntlid);
>   
>   	kfree(ctrl->sqs);
>   	kfree(ctrl->cqs);
> @@ -1416,6 +1422,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
>   		kfree(subsys);
>   		return ERR_PTR(-ENOMEM);
>   	}
> +	subsys->id = 0;
>   
>   	kref_init(&subsys->ref);
>   
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 46df45e837c9..a3d3471bdf2d 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -211,6 +211,7 @@ struct nvmet_subsys {
>   	struct list_head	namespaces;
>   	unsigned int		nr_namespaces;
>   	unsigned int		max_nsid;
> +	u16			id;
>   
>   	struct list_head	ctrls;
>   

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-04  9:36 ` Max Gurtovoy
@ 2019-11-04 10:45   ` Mark Ruijter
  2019-11-04 14:04     ` Max Gurtovoy
  2019-11-05 21:15     ` Chaitanya Kulkarni
  0 siblings, 2 replies; 16+ messages in thread
From: Mark Ruijter @ 2019-11-04 10:45 UTC (permalink / raw)
  To: Max Gurtovoy, Chaitanya Kulkarni, linux-nvme


Hi Max,

When you create a HA cluster using two nodes exporting the same volume the controller IDs can collide.
>> [1122789.054677] nvme nvme1: Duplicate cntlid 4 with nvme0, rejecting
So the use case for this change is related to HA setups.

Are you saying that the suggested solution to this problem would be to force the user to use the -D option?

@Chaitanya : About the proposed patch:
I just tested it. It does work but it has some issues:

1. When you read the attr_id without writing anything to it the controller id seems to be 0, which is not the case.
I think we should fix that since this is confusing.

2. When an initiator has connected and  id_simple_get was called we can still write to attr_id.
In that case the generated ID will no longer call ida_simple_remove() since subsys->id is now set?

Thanks,

Mark Ruijter


Op 04-11-19 10:37 heeft Linux-nvme namens Max Gurtovoy <linux-nvme-bounces@lists.infradead.org namens maxg@mellanox.com> geschreven:

    
    On 11/3/2019 8:13 PM, Chaitanya Kulkarni wrote:
    > This patch adds a new target subsys attribute which allows user to
    > optionally specify target controller which then used in the
    > nvmet_execute_identify_ctrl() to fill up the nvme_id_ctrl structure.
    
    hi,
    
    can you explain why we should give the user the ability to control this ?
    
    In this case you will have multiple controllers with the same id, and 
    I'm not sure that we want that.
    
    Did you try to connect to it with -D (duplication) flag ?
    
    >
    > When new attribute is not specified target will fall back to original
    > cntlid calculation method.
    >
    > Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
    > ---
    > Hi Mark,
    >
    > This patch allows user to set the contoller ID through configfs
    > which is target susbsy attribute.
    >
    > Can you please take a look and provide feedback ? so that we can get
    > it to upstream ? I'm planning to send this on Sunday.
    >
    > -Regards,
    > Chaitanya
    > ---
    >   drivers/nvme/target/configfs.c | 23 +++++++++++++++++++++++
    >   drivers/nvme/target/core.c     | 23 +++++++++++++++--------
    >   drivers/nvme/target/nvmet.h    |  1 +
    >   3 files changed, 39 insertions(+), 8 deletions(-)
    >
    > diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
    > index 98613a45bd3b..83391e54ed12 100644
    > --- a/drivers/nvme/target/configfs.c
    > +++ b/drivers/nvme/target/configfs.c
    > @@ -862,10 +862,33 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
    >   }
    >   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
    >   
    > +static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
    > +					     char *page)
    > +{
    > +	struct nvmet_subsys *subsys = to_subsys(item);
    > +
    > +	return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
    > +}
    > +
    > +static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
    > +					  const char *page, size_t count)
    > +{
    > +	struct nvmet_subsys *subsys = to_subsys(item);
    > +
    > +	down_write(&nvmet_config_sem);
    > +	/* should this be %x ? */
    > +	sscanf(page, "%hu\n", &subsys->id);
    > +	up_write(&nvmet_config_sem);
    > +
    > +	return count;
    > +}
    > +CONFIGFS_ATTR(nvmet_subsys_, attr_id);
    > +
    >   static struct configfs_attribute *nvmet_subsys_attrs[] = {
    >   	&nvmet_subsys_attr_attr_allow_any_host,
    >   	&nvmet_subsys_attr_attr_version,
    >   	&nvmet_subsys_attr_attr_serial,
    > +	&nvmet_subsys_attr_attr_id,
    >   	NULL,
    >   };
    >   
    > diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
    > index 28438b833c1b..2396215a23c9 100644
    > --- a/drivers/nvme/target/core.c
    > +++ b/drivers/nvme/target/core.c
    > @@ -1267,13 +1267,18 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
    >   	if (!ctrl->sqs)
    >   		goto out_free_cqs;
    >   
    > -	ret = ida_simple_get(&cntlid_ida,
    > -			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
    > -			     GFP_KERNEL);
    > -	if (ret < 0) {
    > -		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
    > -		goto out_free_sqs;
    > -	}
    > +	if (!subsys->id) {
    > +		ret = ida_simple_get(&cntlid_ida,
    > +				NVME_CNTLID_MIN, NVME_CNTLID_MAX,
    > +				GFP_KERNEL);
    > +		if (ret < 0) {
    > +			status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
    > +			goto out_free_sqs;
    > +		}
    > +		ctrl->cntlid = ret;
    > +	} else
    > +		ret = subsys->id;
    > +
    >   	ctrl->cntlid = ret;
    >   
    >   	ctrl->ops = req->ops;
    > @@ -1330,7 +1335,8 @@ static void nvmet_ctrl_free(struct kref *ref)
    >   	flush_work(&ctrl->async_event_work);
    >   	cancel_work_sync(&ctrl->fatal_err_work);
    >   
    > -	ida_simple_remove(&cntlid_ida, ctrl->cntlid);
    > +	if (!subsys->id)
    > +		ida_simple_remove(&cntlid_ida, ctrl->cntlid);
    >   
    >   	kfree(ctrl->sqs);
    >   	kfree(ctrl->cqs);
    > @@ -1416,6 +1422,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
    >   		kfree(subsys);
    >   		return ERR_PTR(-ENOMEM);
    >   	}
    > +	subsys->id = 0;
    >   
    >   	kref_init(&subsys->ref);
    >   
    > diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
    > index 46df45e837c9..a3d3471bdf2d 100644
    > --- a/drivers/nvme/target/nvmet.h
    > +++ b/drivers/nvme/target/nvmet.h
    > @@ -211,6 +211,7 @@ struct nvmet_subsys {
    >   	struct list_head	namespaces;
    >   	unsigned int		nr_namespaces;
    >   	unsigned int		max_nsid;
    > +	u16			id;
    >   
    >   	struct list_head	ctrls;
    >   
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme@lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme
    

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-04 10:45   ` Mark Ruijter
@ 2019-11-04 14:04     ` Max Gurtovoy
  2019-11-04 16:36       ` Mark Ruijter
  2019-11-05 21:15     ` Chaitanya Kulkarni
  1 sibling, 1 reply; 16+ messages in thread
From: Max Gurtovoy @ 2019-11-04 14:04 UTC (permalink / raw)
  To: Mark Ruijter, Chaitanya Kulkarni, linux-nvme


On 11/4/2019 12:45 PM, Mark Ruijter wrote:
> Hi Max,
>
> When you create a HA cluster using two nodes exporting the same volume the controller IDs can collide.
>>> [1122789.054677] nvme nvme1: Duplicate cntlid 4 with nvme0, rejecting
> So the use case for this change is related to HA setups.
>
> Are you saying that the suggested solution to this problem would be to force the user to use the -D option?

can you try adding an address check to the condition (ctrl->opts->traddr)?

let me know if that helps.



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-04 14:04     ` Max Gurtovoy
@ 2019-11-04 16:36       ` Mark Ruijter
  2019-11-05 22:38         ` James Smart
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Ruijter @ 2019-11-04 16:36 UTC (permalink / raw)
  To: Max Gurtovoy, Chaitanya Kulkarni, linux-nvme

Hi Max & Chaitanya,

Does the NVME specification even allow duplicate controller id's?
I don't mind adding logic to test ctrl->opts->traddr however my first thought would be that having duplicate controller ids poses a risk?
Even if it works for Linux it could be potentially be problematic for another OS, unless NVME spec clearly says it is allowed.
I'll start reading the nvme specification to see if it contains any information about this.

@Chaitanya when it comes to the patch you provided earlier:
I changed that patch so that it solves the two problems that I reported earlier.
It now _always_ uses ida_simple_get().

A third fix is that it now checks if the requested controller ID is allowed.
>> echo 65534 > attr_id 
-bash: echo: write error: Invalid argument

Thanks,

Mark

-------------------------

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 83d1124..fa1a0b7 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -931,6 +931,35 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
+                                            char *page)
+{
+       struct nvmet_subsys *subsys = to_subsys(item);
+
+       return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
+}
+
+static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
+                                         const char *page, size_t count)
+{
+       struct nvmet_subsys *subsys = to_subsys(item);
+       u16 sid;
+       int ret = 0;
+
+       down_write(&nvmet_config_sem);
+       /* should this be %x ? */
+       sscanf(page, "%hu\n", &sid);
+
+       if (sid >= NVME_CNTLID_MIN && sid <= NVME_CNTLID_MAX)
+               subsys->id = sid;
+       else
+               ret = -EINVAL;
+       up_write(&nvmet_config_sem);
+
+       return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, attr_id);
+
 static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
                                              char *page)
 {
@@ -981,6 +1010,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
        &nvmet_subsys_attr_attr_version,
        &nvmet_subsys_attr_attr_serial,
+       &nvmet_subsys_attr_attr_id,
        NULL,
 };

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index fc63b22..0dc5ede 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1210,6 +1210,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
        struct nvmet_ctrl *ctrl;
        int ret;
        u16 status;
+       u16 id_min = NVME_CNTLID_MIN;
+       u16 id_max = NVME_CNTLID_MAX;
 
        status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
        subsys = nvmet_find_get_subsys(req->port, subsysnqn);
@@ -1271,13 +1273,23 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
        if (!ctrl->sqs)
                goto out_free_cqs;
 
+       if (subsys->id) {
+               id_min = subsys->id;
+               id_max = subsys->id + 1;
+       }
+
+       if (ctrl->cntlid)
+               ida_simple_remove(&cntlid_ida, ctrl->cntlid);
+
        ret = ida_simple_get(&cntlid_ida,
-                            NVME_CNTLID_MIN, NVME_CNTLID_MAX,
-                            GFP_KERNEL);
+               id_min, id_max, GFP_KERNEL);
        if (ret < 0) {
+               pr_err("ida_simple_get: failed to get controller id\n");
                status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
                goto out_free_sqs;
        }
+
+       subsys->id = ret;
        ctrl->cntlid = ret;
 
        ctrl->ops = req->ops;
@@ -1427,6 +1439,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
                return ERR_PTR(-ENOMEM);
        }
 
+       subsys->id = 0;
        kref_init(&subsys->ref);
 
        mutex_init(&subsys->lock);

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index e17321f..fc52e42 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -213,6 +213,7 @@ struct nvmet_subsys {
        struct list_head        namespaces;
        unsigned int            nr_namespaces;
        unsigned int            max_nsid;
+       u16                     id;
 
        struct list_head        ctrls;


Op 04-11-19 15:05 heeft Max Gurtovoy <maxg@mellanox.com> geschreven:

    
    On 11/4/2019 12:45 PM, Mark Ruijter wrote:
    > Hi Max,
    >
    > When you create a HA cluster using two nodes exporting the same volume the controller IDs can collide.
    >>> [1122789.054677] nvme nvme1: Duplicate cntlid 4 with nvme0, rejecting
    > So the use case for this change is related to HA setups.
    >
    > Are you saying that the suggested solution to this problem would be to force the user to use the -D option?
    
    can you try adding an address check to the condition (ctrl->opts->traddr)?
    
    let me know if that helps.
    
    
    

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-04 10:45   ` Mark Ruijter
  2019-11-04 14:04     ` Max Gurtovoy
@ 2019-11-05 21:15     ` Chaitanya Kulkarni
  1 sibling, 0 replies; 16+ messages in thread
From: Chaitanya Kulkarni @ 2019-11-05 21:15 UTC (permalink / raw)
  To: Mark Ruijter, Max Gurtovoy, linux-nvme

On 11/04/2019 02:45 AM, Mark Ruijter wrote:
> Hi Max,
>
> When you create a HA cluster using two nodes exporting the same volume the controller IDs can collide.
>>> >>[1122789.054677] nvme nvme1: Duplicate cntlid 4 with nvme0, rejecting
> So the use case for this change is related to HA setups.
>
> Are you saying that the suggested solution to this problem would be to force the user to use the -D option?
>
> @Chaitanya : About the proposed patch:
> I just tested it. It does work but it has some issues:
>
> 1. When you read the attr_id without writing anything to it the controller id seems to be 0, which is not the case.
> I think we should fix that since this is confusing.
>
Okay, will fix in next version.

> 2. When an initiator has connected and  id_simple_get was called we can still write to attr_id.
> In that case the generated ID will no longer call ida_simple_remove() since subsys->id is now set?
>
Hmmm, let me take a look.

> Thanks,
>
> Mark Ruijter


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-04 16:36       ` Mark Ruijter
@ 2019-11-05 22:38         ` James Smart
  2019-11-06 16:51           ` Sagi Grimberg
  0 siblings, 1 reply; 16+ messages in thread
From: James Smart @ 2019-11-05 22:38 UTC (permalink / raw)
  To: Mark Ruijter, Max Gurtovoy, Chaitanya Kulkarni, linux-nvme



On 11/4/2019 8:36 AM, Mark Ruijter wrote:
> Hi Max & Chaitanya,
>
> Does the NVME specification even allow duplicate controller id's?
Not within a single subsystem.
But across multiple subsystems there can be.

Discovery controllers (aka subsystem is the discovery NQN) are seen as 
independent subsystems.  Granted I don't know if the spec actually says 
that but it has to work that way.

-- james

> I don't mind adding logic to test ctrl->opts->traddr however my first thought would be that having duplicate controller ids poses a risk?
> Even if it works for Linux it could be potentially be problematic for another OS, unless NVME spec clearly says it is allowed.
> I'll start reading the nvme specification to see if it contains any information about this.
>
> @Chaitanya when it comes to the patch you provided earlier:
> I changed that patch so that it solves the two problems that I reported earlier.
> It now _always_ uses ida_simple_get().
>
> A third fix is that it now checks if the requested controller ID is allowed.
>>> echo 65534 > attr_id
> -bash: echo: write error: Invalid argument
>
> Thanks,
>
> Mark
>
> -------------------------
>
> diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
> index 83d1124..fa1a0b7 100644
> --- a/drivers/nvme/target/configfs.c
> +++ b/drivers/nvme/target/configfs.c
> @@ -931,6 +931,35 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
>   }
>   CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
>   
> +static ssize_t nvmet_subsys_attr_id_show(struct config_item *item,
> +                                            char *page)
> +{
> +       struct nvmet_subsys *subsys = to_subsys(item);
> +
> +       return snprintf(page, PAGE_SIZE, "%d\n", subsys->id);
> +}
> +
> +static ssize_t nvmet_subsys_attr_id_store(struct config_item *item,
> +                                         const char *page, size_t count)
> +{
> +       struct nvmet_subsys *subsys = to_subsys(item);
> +       u16 sid;
> +       int ret = 0;
> +
> +       down_write(&nvmet_config_sem);
> +       /* should this be %x ? */
> +       sscanf(page, "%hu\n", &sid);
> +
> +       if (sid >= NVME_CNTLID_MIN && sid <= NVME_CNTLID_MAX)
> +               subsys->id = sid;
> +       else
> +               ret = -EINVAL;
> +       up_write(&nvmet_config_sem);
> +
> +       return ret ? ret : count;
> +}
> +CONFIGFS_ATTR(nvmet_subsys_, attr_id);
> +
>   static ssize_t nvmet_subsys_attr_model_show(struct config_item *item,
>                                                char *page)
>   {
> @@ -981,6 +1010,7 @@ static struct configfs_attribute *nvmet_subsys_attrs[] = {
>          &nvmet_subsys_attr_attr_version,
>          &nvmet_subsys_attr_attr_serial,
> +       &nvmet_subsys_attr_attr_id,
>          NULL,
>   };
>
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index fc63b22..0dc5ede 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1210,6 +1210,8 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
>          struct nvmet_ctrl *ctrl;
>          int ret;
>          u16 status;
> +       u16 id_min = NVME_CNTLID_MIN;
> +       u16 id_max = NVME_CNTLID_MAX;
>   
>          status = NVME_SC_CONNECT_INVALID_PARAM | NVME_SC_DNR;
>          subsys = nvmet_find_get_subsys(req->port, subsysnqn);
> @@ -1271,13 +1273,23 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
>          if (!ctrl->sqs)
>                  goto out_free_cqs;
>   
> +       if (subsys->id) {
> +               id_min = subsys->id;
> +               id_max = subsys->id + 1;
> +       }
> +
> +       if (ctrl->cntlid)
> +               ida_simple_remove(&cntlid_ida, ctrl->cntlid);
> +
>          ret = ida_simple_get(&cntlid_ida,
> -                            NVME_CNTLID_MIN, NVME_CNTLID_MAX,
> -                            GFP_KERNEL);
> +               id_min, id_max, GFP_KERNEL);
>          if (ret < 0) {
> +               pr_err("ida_simple_get: failed to get controller id\n");
>                  status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
>                  goto out_free_sqs;
>          }
> +
> +       subsys->id = ret;
>          ctrl->cntlid = ret;
>   
>          ctrl->ops = req->ops;
> @@ -1427,6 +1439,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
>                  return ERR_PTR(-ENOMEM);
>          }
>   
> +       subsys->id = 0;
>          kref_init(&subsys->ref);
>   
>          mutex_init(&subsys->lock);
>
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index e17321f..fc52e42 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -213,6 +213,7 @@ struct nvmet_subsys {
>          struct list_head        namespaces;
>          unsigned int            nr_namespaces;
>          unsigned int            max_nsid;
> +       u16                     id;
>   
>          struct list_head        ctrls;
>
>
> Op 04-11-19 15:05 heeft Max Gurtovoy <maxg@mellanox.com> geschreven:
>
>      
>      On 11/4/2019 12:45 PM, Mark Ruijter wrote:
>      > Hi Max,
>      >
>      > When you create a HA cluster using two nodes exporting the same volume the controller IDs can collide.
>      >>> [1122789.054677] nvme nvme1: Duplicate cntlid 4 with nvme0, rejecting
>      > So the use case for this change is related to HA setups.
>      >
>      > Are you saying that the suggested solution to this problem would be to force the user to use the -D option?
>      
>      can you try adding an address check to the condition (ctrl->opts->traddr)?
>      
>      let me know if that helps.
>      
>      
>      
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-05 22:38         ` James Smart
@ 2019-11-06 16:51           ` Sagi Grimberg
  2019-11-08 16:15             ` Mark Ruijter
  0 siblings, 1 reply; 16+ messages in thread
From: Sagi Grimberg @ 2019-11-06 16:51 UTC (permalink / raw)
  To: James Smart, Mark Ruijter, Max Gurtovoy, Chaitanya Kulkarni, linux-nvme


>> Hi Max & Chaitanya,
>>
>> Does the NVME specification even allow duplicate controller id's?
> Not within a single subsystem.
> But across multiple subsystems there can be.

Yes, this patch is broken..

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-06 16:51           ` Sagi Grimberg
@ 2019-11-08 16:15             ` Mark Ruijter
  2019-11-12 19:01               ` Sagi Grimberg
  0 siblings, 1 reply; 16+ messages in thread
From: Mark Ruijter @ 2019-11-08 16:15 UTC (permalink / raw)
  To: Sagi Grimberg, James Smart, Max Gurtovoy, Chaitanya Kulkarni, linux-nvme

[-- Attachment #1: Type: text/plain, Size: 2591 bytes --]


I have setup a cluster using two nodes with a dual ported nvme drive and tested exporting the drive from both nodes.

The configuration on both nodes, with the exception of the ip address, looks like this:
    {
      "addr": {
        "adrfam": "ipv4", 
        "traddr": "192.168.1.11", 
        "treq": "not specified", 
        "trsvcid": "4420", 
        "trtype": "tcp"
      }, 
      "portid": 322, 
      "referrals": [], 
      "subsystems": [
        "clvol"
      ]
    },

  "subsystems": [
    {
      "allowed_hosts": [], 
      "attr": {
        "allow_any_host": "1", 
        "model": "clpool/clvol", 
        "serial": "e94430920f6103af", 
        "version": "1.3"
      }, 
      "namespaces": [
        {
          "device": {
            "nguid": "00000000-0000-0000-0000-000000000000", 
            "path": "/dev/clpool/clvol", 
            "uuid": "99451596-9675-4382-bff8-b78ee34de567"
          }, 
          "enable": 1, 
          "nsid": 1
        }
      ], 
      "nqn": "clvol"
    },

When I now connect the initiator to the two cluster nodes without the patch the result will be a controller id collision.
And the initiator refuses to connect to the second node that you try to connect.
--
[ 7895.052302] nvme nvme0: new ctrl: NQN "clvol", addr 192.168.8.10:4420
[ 7895.053297] nvme0n1: detected capacity change from 0 to 107374182400
[ 7898.188321] nvme nvme1: Duplicate cntlid 1 with nvme0, rejecting
--

With the attached patch I am able to force the second node to use a higher offset when enumerating controller ids:
nodea:
/sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min 
1
nodeb:/sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min 
32

In real life the number used for the offsets could be derived from something like the Pacemaker node number.
And the Pacemaker + DLM could be used for nvme persistent reservations.

The controller id collision now no longer occurs.
And the initiator can connect, and two paths will be shown:

root@r11i1:~# nvme list-subsys
nvme-subsys0 - NQN=clvol
\
 +- nvme0 tcp traddr=192.168.1.10 trsvcid=4420
 +- nvme1 tcp traddr=192.168.1.11 trsvcid=4420

Does this all make sense?

Thanks,

Mark



Op 06-11-19 17:51 heeft Sagi Grimberg <sagi@grimberg.me> geschreven:

    
    >> Hi Max & Chaitanya,
    >>
    >> Does the NVME specification even allow duplicate controller id's?
    > Not within a single subsystem.
    > But across multiple subsystems there can be.
    
    Yes, this patch is broken..
    


[-- Attachment #2: cntlid_min.patch --]
[-- Type: application/octet-stream, Size: 2611 bytes --]

diff --git a/drivers/nvme/target/configfs.c b/drivers/nvme/target/configfs.c
index 98613a4..95849bc 100644
--- a/drivers/nvme/target/configfs.c
+++ b/drivers/nvme/target/configfs.c
@@ -862,10 +862,40 @@ static ssize_t nvmet_subsys_attr_serial_store(struct config_item *item,
 }
 CONFIGFS_ATTR(nvmet_subsys_, attr_serial);
 
+static ssize_t nvmet_subsys_cntlid_min_show(struct config_item *item,
+                                             char *page)
+{
+        struct nvmet_subsys *subsys = to_subsys(item);
+
+        return snprintf(page, PAGE_SIZE, "%d\n", subsys->cntlid_min);
+}
+
+static ssize_t nvmet_subsys_cntlid_min_store(struct config_item *item,
+                                          const char *page, size_t count)
+{
+        struct nvmet_subsys *subsys = to_subsys(item);
+        u16 sid;
+        int ret = 0;
+
+        down_write(&nvmet_config_sem);
+        /* should this be %x ? */
+        sscanf(page, "%hu\n", &sid);
+
+        if (sid >= NVME_CNTLID_MIN && sid <= NVME_CNTLID_MAX)
+                subsys->cntlid_min = sid;
+        else
+                ret = -EINVAL;
+        up_write(&nvmet_config_sem);
+
+        return ret ? ret : count;
+}
+CONFIGFS_ATTR(nvmet_subsys_, cntlid_min);
+
 static struct configfs_attribute *nvmet_subsys_attrs[] = {
 	&nvmet_subsys_attr_attr_allow_any_host,
 	&nvmet_subsys_attr_attr_version,
 	&nvmet_subsys_attr_attr_serial,
+	&nvmet_subsys_attr_cntlid_min,
 	NULL,
 };
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 3a67e24..a92b1ad 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1268,10 +1268,9 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const char *hostnqn,
 			GFP_KERNEL);
 	if (!ctrl->sqs)
 		goto out_free_cqs;
-
 	ret = ida_simple_get(&cntlid_ida,
-			     NVME_CNTLID_MIN, NVME_CNTLID_MAX,
-			     GFP_KERNEL);
+			subsys->cntlid_min, NVME_CNTLID_MAX,
+			GFP_KERNEL);
 	if (ret < 0) {
 		status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
 		goto out_free_sqs;
@@ -1418,7 +1417,7 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char *subsysnqn,
 		kfree(subsys);
 		return ERR_PTR(-ENOMEM);
 	}
-
+	subsys->cntlid_min = NVME_CNTLID_MIN;
 	kref_init(&subsys->ref);
 
 	mutex_init(&subsys->lock);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index c51f8dd..c6c721a 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -211,6 +211,7 @@ struct nvmet_subsys {
 	struct list_head	namespaces;
 	unsigned int		nr_namespaces;
 	unsigned int		max_nsid;
+	u16                     cntlid_min;
 
 	struct list_head	ctrls;
 

[-- Attachment #3: Type: text/plain, Size: 158 bytes --]

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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-08 16:15             ` Mark Ruijter
@ 2019-11-12 19:01               ` Sagi Grimberg
  2019-11-12 21:19                 ` Mark Ruijter
  2019-11-13 23:40                 ` Chaitanya Kulkarni
  0 siblings, 2 replies; 16+ messages in thread
From: Sagi Grimberg @ 2019-11-12 19:01 UTC (permalink / raw)
  To: Mark Ruijter, James Smart, Max Gurtovoy, Chaitanya Kulkarni, linux-nvme



On 11/8/19 8:15 AM, Mark Ruijter wrote:
> 
> I have setup a cluster using two nodes with a dual ported nvme drive and tested exporting the drive from both nodes.
> 
> The configuration on both nodes, with the exception of the ip address, looks like this:
>      {
>        "addr": {
>          "adrfam": "ipv4",
>          "traddr": "192.168.1.11",
>          "treq": "not specified",
>          "trsvcid": "4420",
>          "trtype": "tcp"
>        },
>        "portid": 322,
>        "referrals": [],
>        "subsystems": [
>          "clvol"
>        ]
>      },
> 
>    "subsystems": [
>      {
>        "allowed_hosts": [],
>        "attr": {
>          "allow_any_host": "1",
>          "model": "clpool/clvol",
>          "serial": "e94430920f6103af",
>          "version": "1.3"
>        },
>        "namespaces": [
>          {
>            "device": {
>              "nguid": "00000000-0000-0000-0000-000000000000",
>              "path": "/dev/clpool/clvol",
>              "uuid": "99451596-9675-4382-bff8-b78ee34de567"
>            },
>            "enable": 1,
>            "nsid": 1
>          }
>        ],
>        "nqn": "clvol"
>      },
> 
> When I now connect the initiator to the two cluster nodes without the patch the result will be a controller id collision.
> And the initiator refuses to connect to the second node that you try to connect.
> --
> [ 7895.052302] nvme nvme0: new ctrl: NQN "clvol", addr 192.168.8.10:4420
> [ 7895.053297] nvme0n1: detected capacity change from 0 to 107374182400
> [ 7898.188321] nvme nvme1: Duplicate cntlid 1 with nvme0, rejecting
> --
> 
> With the attached patch I am able to force the second node to use a higher offset when enumerating controller ids:
> nodea:
> /sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min
> 1
> nodeb:/sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min
> 32
> 
> In real life the number used for the offsets could be derived from something like the Pacemaker node number.
> And the Pacemaker + DLM could be used for nvme persistent reservations.
> 
> The controller id collision now no longer occurs.
> And the initiator can connect, and two paths will be shown:
> 
> root@r11i1:~# nvme list-subsys
> nvme-subsys0 - NQN=clvol
> \
>   +- nvme0 tcp traddr=192.168.1.10 trsvcid=4420
>   +- nvme1 tcp traddr=192.168.1.11 trsvcid=4420
> 
> Does this all make sense?

I think we need a cntlid_max to make sure we don't have collisions..

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-12 19:01               ` Sagi Grimberg
@ 2019-11-12 21:19                 ` Mark Ruijter
  2019-11-13  0:44                   ` James Smart
  2019-11-13 23:40                 ` Chaitanya Kulkarni
  1 sibling, 1 reply; 16+ messages in thread
From: Mark Ruijter @ 2019-11-12 21:19 UTC (permalink / raw)
  To: Sagi Grimberg; +Cc: Max Gurtovoy, linux-nvme, James Smart, Chaitanya Kulkarni


>> I think we need a cntlid_max.

I initially thought about that and decided it was not worth it. When a user selects the minimum it is highly unlikely that a collision will occur.

With a symmetrical configuration even an offset of 1 would be enough to prevent that?

I don’t mind adding it if the consensus is that it’s worth the effort.

Mark


> Op 12 nov. 2019 om 20:02 heeft Sagi Grimberg <sagi@grimberg.me> het volgende geschreven:
> 
> 
> 
>> On 11/8/19 8:15 AM, Mark Ruijter wrote:
>> I have setup a cluster using two nodes with a dual ported nvme drive and tested exporting the drive from both nodes.
>> The configuration on both nodes, with the exception of the ip address, looks like this:
>>     {
>>       "addr": {
>>         "adrfam": "ipv4",
>>         "traddr": "192.168.1.11",
>>         "treq": "not specified",
>>         "trsvcid": "4420",
>>         "trtype": "tcp"
>>       },
>>       "portid": 322,
>>       "referrals": [],
>>       "subsystems": [
>>         "clvol"
>>       ]
>>     },
>>   "subsystems": [
>>     {
>>       "allowed_hosts": [],
>>       "attr": {
>>         "allow_any_host": "1",
>>         "model": "clpool/clvol",
>>         "serial": "e94430920f6103af",
>>         "version": "1.3"
>>       },
>>       "namespaces": [
>>         {
>>           "device": {
>>             "nguid": "00000000-0000-0000-0000-000000000000",
>>             "path": "/dev/clpool/clvol",
>>             "uuid": "99451596-9675-4382-bff8-b78ee34de567"
>>           },
>>           "enable": 1,
>>           "nsid": 1
>>         }
>>       ],
>>       "nqn": "clvol"
>>     },
>> When I now connect the initiator to the two cluster nodes without the patch the result will be a controller id collision.
>> And the initiator refuses to connect to the second node that you try to connect.
>> --
>> [ 7895.052302] nvme nvme0: new ctrl: NQN "clvol", addr 192.168.8.10:4420
>> [ 7895.053297] nvme0n1: detected capacity change from 0 to 107374182400
>> [ 7898.188321] nvme nvme1: Duplicate cntlid 1 with nvme0, rejecting
>> --
>> With the attached patch I am able to force the second node to use a higher offset when enumerating controller ids:
>> nodea:
>> /sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min
>> 1
>> nodeb:/sys/kernel/config/nvmet/subsystems/clvol # cat cntlid_min
>> 32
>> In real life the number used for the offsets could be derived from something like the Pacemaker node number.
>> And the Pacemaker + DLM could be used for nvme persistent reservations.
>> The controller id collision now no longer occurs.
>> And the initiator can connect, and two paths will be shown:
>> root@r11i1:~# nvme list-subsys
>> nvme-subsys0 - NQN=clvol
>> \
>>  +- nvme0 tcp traddr=192.168.1.10 trsvcid=4420
>>  +- nvme1 tcp traddr=192.168.1.11 trsvcid=4420
>> Does this all make sense?
> 
> I think we need a cntlid_max to make sure we don't have collisions..
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-12 21:19                 ` Mark Ruijter
@ 2019-11-13  0:44                   ` James Smart
  0 siblings, 0 replies; 16+ messages in thread
From: James Smart @ 2019-11-13  0:44 UTC (permalink / raw)
  To: Mark Ruijter, Sagi Grimberg; +Cc: Max Gurtovoy, linux-nvme, Chaitanya Kulkarni



On 11/12/2019 1:19 PM, Mark Ruijter wrote:
>>> I think we need a cntlid_max.
> I initially thought about that and decided it was not worth it. When a user selects the minimum it is highly unlikely that a collision will occur.

why would you leave that to chance ?

>
> With a symmetrical configuration even an offset of 1 would be enough to prevent that?

ahh.. it seems you are answering with your specific solution in sight. I 
think we would want it be more generic in scope.

>
> I don’t mind adding it if the consensus is that it’s worth the effort.
>
> Mark
>

I second the cntlid_max add.

-- james



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-12 19:01               ` Sagi Grimberg
  2019-11-12 21:19                 ` Mark Ruijter
@ 2019-11-13 23:40                 ` Chaitanya Kulkarni
  2019-11-14 15:55                   ` Mark Ruijter
  2019-11-20 18:51                   ` Christoph Hellwig
  1 sibling, 2 replies; 16+ messages in thread
From: Chaitanya Kulkarni @ 2019-11-13 23:40 UTC (permalink / raw)
  To: Sagi Grimberg, Mark Ruijter, James Smart, Max Gurtovoy, linux-nvme

On 11/12/2019 11:01 AM, Sagi Grimberg wrote:
>> >root@r11i1:~# nvme list-subsys
>> >nvme-subsys0 - NQN=clvol
>> >\
>> >   +- nvme0 tcp traddr=192.168.1.10 trsvcid=4420
>> >   +- nvme1 tcp traddr=192.168.1.11 trsvcid=4420
>> >
>> >Does this all make sense?
> I think we need a cntlid_max to make sure we don't have collisions..
>
How about following patch which used cntl_min and cntl_max ?

(compile only)

+static bool nvmet_subsys_cntlid_store(struct nvmet_subsys *s, const 
char *page,
+                                     bool min)
+{
+       bool ret = true;
+       u16 cid;
+
+       down_write(&nvmet_config_sem);
+       sscanf(page, "%hu\n", &cid);
+       if (cid <  NVME_CNTLID_MIN || cid > NVME_CNTLID_MAX) {
+               ret = false;
+               goto out;
+       }
+
+       if (min)
+               s->cntlid_min = cid;
+       else
+               s->cntlid_max = cid;
+out:
+       up_write(&nvmet_config_sem);
+
+       return ret;
+}
+
+static bool nvmet_subsys_cntlid_show(struct nvmet_subsys *s, char *page,
+                                    bool min)
+{
+       u16 cid = min ? s->cntlid_min : s->cntlid_max;
+
+       return snprintf(page, PAGE_SIZE, "%u\n", cid);
+}
+
+static ssize_t nvmet_subsys_cntlid_min_show(struct config_item *item,
+                                           char *page)
+{
+       return nvmet_subsys_cntlid_show(to_subsys(item), page, true);
+}
+
+static ssize_t nvmet_subsys_cntlid_min_store(struct config_item *item,
+                                            const char *page, size_t count)
+{
+       struct nvmet_subsys *s = to_subsys(item);
+
+       return nvmet_subsys_cntlid_store(s, page, true) ? count : -EINVAL;
+}
+CONFIGFS_ATTR(nvmet_subsys_, cntlid_min);
+
+static ssize_t nvmet_subsys_cntlid_max_show(struct config_item *item,
+                                           char *page)
+{
+       return nvmet_subsys_cntlid_show(to_subsys(item), page, false);
+}
+
+static ssize_t nvmet_subsys_cntlid_max_store(struct config_item *item,
+                                            const char *page, size_t count)
+{
+       struct nvmet_subsys *s = to_subsys(item);
+
+       return nvmet_subsys_cntlid_store(s, page, false) ? count : -EINVAL;
+}
+CONFIGFS_ATTR(nvmet_subsys_, cntlid_max);
+
  static struct configfs_attribute *nvmet_subsys_attrs[] = {
         &nvmet_subsys_attr_attr_allow_any_host,
         &nvmet_subsys_attr_attr_version,
         &nvmet_subsys_attr_attr_serial,
+       &nvmet_subsys_attr_cntlid_min,
+       &nvmet_subsys_attr_cntlid_max,
         NULL,
  };

diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 28438b833c1b..d5a5af3f21b6 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -1268,7 +1268,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const 
char *hostnqn,
                 goto out_free_cqs;

         ret = ida_simple_get(&cntlid_ida,
-                            NVME_CNTLID_MIN, NVME_CNTLID_MAX,
+                            subsys->cntlid_min, subsys->cntlid_min,
                              GFP_KERNEL);
         if (ret < 0) {
                 status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
@@ -1416,7 +1416,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char 
*subsysnqn,
                 kfree(subsys);
                 return ERR_PTR(-ENOMEM);
         }
-
+       subsys->cntlid_min = NVME_CNTLID_MIN;
+       subsys->cntlid_max = NVME_CNTLID_MAX;
         kref_init(&subsys->ref);

         mutex_init(&subsys->lock);
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 46df45e837c9..ecbd16f52973 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -211,6 +211,8 @@ struct nvmet_subsys {
         struct list_head        namespaces;
         unsigned int            nr_namespaces;
         unsigned int            max_nsid;
+       u16                     cntlid_min;
+       u16                     cntlid_max;

         struct list_head        ctrls;

Although we still have the outstanding issue pointed by Mark.
Since we define controller id at the level of subsystem having min and
max still result in the error when we are trying to export same
subsys with two different ports.

Mark please correct me if I'm wrong.

For that case should we use subsys-id + port-id as a combination of the
controller id ?

Any thoughts ?






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

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-13 23:40                 ` Chaitanya Kulkarni
@ 2019-11-14 15:55                   ` Mark Ruijter
  2019-11-20 18:51                   ` Christoph Hellwig
  1 sibling, 0 replies; 16+ messages in thread
From: Mark Ruijter @ 2019-11-14 15:55 UTC (permalink / raw)
  To: Chaitanya Kulkarni, Sagi Grimberg, James Smart, Max Gurtovoy, linux-nvme

   >> Although we still have the outstanding issue pointed by Mark.

Assuming that the range min/max is greater or equal than the number
of target ports you use for exporting the subsystem you will not run into problems.

   >> Since we define controller id at the level of subsystem having min and
   >> max still result in the error when we are trying to export same
   >> subsys with two different ports.
That is no longer the case when cntlid_max - cntlid_min >= number of exports.
The first patch we created hard wired the cntlid, which was a problem.

When you set cntlid_max to cntlid_min + the (expected) number of target ports in the system you should always be fine. 
In a cluster the port_id can be identical well so adding that does not make much sense either.

As I see it, this patch is sufficient:
Either you don't do anything, in which case things work like before. The cntlid is automatically assigned.
Or you can configure the cntlid. Should you make a mistake the initiator will simply refuse to connect.
The initiator will also clearly log the problem.
In that case no real harm is done and you can simply change the target configuration to make it work.

Mark


Op 14-11-19 00:40 heeft Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> geschreven:

    On 11/12/2019 11:01 AM, Sagi Grimberg wrote:
    >> >root@r11i1:~# nvme list-subsys
    >> >nvme-subsys0 - NQN=clvol
    >> >\
    >> >   +- nvme0 tcp traddr=192.168.1.10 trsvcid=4420
    >> >   +- nvme1 tcp traddr=192.168.1.11 trsvcid=4420
    >> >
    >> >Does this all make sense?
    > I think we need a cntlid_max to make sure we don't have collisions..
    >
    How about following patch which used cntl_min and cntl_max ?
    
    (compile only)
    
    +static bool nvmet_subsys_cntlid_store(struct nvmet_subsys *s, const 
    char *page,
    +                                     bool min)
    +{
    +       bool ret = true;
    +       u16 cid;
    +
    +       down_write(&nvmet_config_sem);
    +       sscanf(page, "%hu\n", &cid);
    +       if (cid <  NVME_CNTLID_MIN || cid > NVME_CNTLID_MAX) {
    +               ret = false;
    +               goto out;
    +       }
    +
    +       if (min)
    +               s->cntlid_min = cid;
    +       else
    +               s->cntlid_max = cid;
    +out:
    +       up_write(&nvmet_config_sem);
    +
    +       return ret;
    +}
    +
    +static bool nvmet_subsys_cntlid_show(struct nvmet_subsys *s, char *page,
    +                                    bool min)
    +{
    +       u16 cid = min ? s->cntlid_min : s->cntlid_max;
    +
    +       return snprintf(page, PAGE_SIZE, "%u\n", cid);
    +}
    +
    +static ssize_t nvmet_subsys_cntlid_min_show(struct config_item *item,
    +                                           char *page)
    +{
    +       return nvmet_subsys_cntlid_show(to_subsys(item), page, true);
    +}
    +
    +static ssize_t nvmet_subsys_cntlid_min_store(struct config_item *item,
    +                                            const char *page, size_t count)
    +{
    +       struct nvmet_subsys *s = to_subsys(item);
    +
    +       return nvmet_subsys_cntlid_store(s, page, true) ? count : -EINVAL;
    +}
    +CONFIGFS_ATTR(nvmet_subsys_, cntlid_min);
    +
    +static ssize_t nvmet_subsys_cntlid_max_show(struct config_item *item,
    +                                           char *page)
    +{
    +       return nvmet_subsys_cntlid_show(to_subsys(item), page, false);
    +}
    +
    +static ssize_t nvmet_subsys_cntlid_max_store(struct config_item *item,
    +                                            const char *page, size_t count)
    +{
    +       struct nvmet_subsys *s = to_subsys(item);
    +
    +       return nvmet_subsys_cntlid_store(s, page, false) ? count : -EINVAL;
    +}
    +CONFIGFS_ATTR(nvmet_subsys_, cntlid_max);
    +
      static struct configfs_attribute *nvmet_subsys_attrs[] = {
             &nvmet_subsys_attr_attr_allow_any_host,
             &nvmet_subsys_attr_attr_version,
             &nvmet_subsys_attr_attr_serial,
    +       &nvmet_subsys_attr_cntlid_min,
    +       &nvmet_subsys_attr_cntlid_max,
             NULL,
      };
    
    diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
    index 28438b833c1b..d5a5af3f21b6 100644
    --- a/drivers/nvme/target/core.c
    +++ b/drivers/nvme/target/core.c
    @@ -1268,7 +1268,7 @@ u16 nvmet_alloc_ctrl(const char *subsysnqn, const 
    char *hostnqn,
                     goto out_free_cqs;
    
             ret = ida_simple_get(&cntlid_ida,
    -                            NVME_CNTLID_MIN, NVME_CNTLID_MAX,
    +                            subsys->cntlid_min, subsys->cntlid_min,
                                  GFP_KERNEL);
             if (ret < 0) {
                     status = NVME_SC_CONNECT_CTRL_BUSY | NVME_SC_DNR;
    @@ -1416,7 +1416,8 @@ struct nvmet_subsys *nvmet_subsys_alloc(const char 
    *subsysnqn,
                     kfree(subsys);
                     return ERR_PTR(-ENOMEM);
             }
    -
    +       subsys->cntlid_min = NVME_CNTLID_MIN;
    +       subsys->cntlid_max = NVME_CNTLID_MAX;
             kref_init(&subsys->ref);
    
             mutex_init(&subsys->lock);
    diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
    index 46df45e837c9..ecbd16f52973 100644
    --- a/drivers/nvme/target/nvmet.h
    +++ b/drivers/nvme/target/nvmet.h
    @@ -211,6 +211,8 @@ struct nvmet_subsys {
             struct list_head        namespaces;
             unsigned int            nr_namespaces;
             unsigned int            max_nsid;
    +       u16                     cntlid_min;
    +       u16                     cntlid_max;
    
             struct list_head        ctrls;
    
    Although we still have the outstanding issue pointed by Mark.
    Since we define controller id at the level of subsystem having min and
    max still result in the error when we are trying to export same
    subsys with two different ports.
    
    Mark please correct me if I'm wrong.
    
    For that case should we use subsys-id + port-id as a combination of the
    controller id ?
    
    Any thoughts ?
    
    
    
    
    
    

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] nvmet: make ctrl-id configurable
  2019-11-13 23:40                 ` Chaitanya Kulkarni
  2019-11-14 15:55                   ` Mark Ruijter
@ 2019-11-20 18:51                   ` Christoph Hellwig
  1 sibling, 0 replies; 16+ messages in thread
From: Christoph Hellwig @ 2019-11-20 18:51 UTC (permalink / raw)
  To: Chaitanya Kulkarni
  Cc: Mark Ruijter, Max Gurtovoy, Sagi Grimberg, linux-nvme, James Smart

The min/max approach looks sensible to me, at least with some good
documentation on how to use multiple Linux systems to implement
a single subsystem.

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-11-20 18:51 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-03 18:13 [PATCH] nvmet: make ctrl-id configurable Chaitanya Kulkarni
2019-11-03 18:15 ` Chaitanya Kulkarni
2019-11-04  9:36 ` Max Gurtovoy
2019-11-04 10:45   ` Mark Ruijter
2019-11-04 14:04     ` Max Gurtovoy
2019-11-04 16:36       ` Mark Ruijter
2019-11-05 22:38         ` James Smart
2019-11-06 16:51           ` Sagi Grimberg
2019-11-08 16:15             ` Mark Ruijter
2019-11-12 19:01               ` Sagi Grimberg
2019-11-12 21:19                 ` Mark Ruijter
2019-11-13  0:44                   ` James Smart
2019-11-13 23:40                 ` Chaitanya Kulkarni
2019-11-14 15:55                   ` Mark Ruijter
2019-11-20 18:51                   ` Christoph Hellwig
2019-11-05 21:15     ` Chaitanya Kulkarni

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).