* [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 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
* 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
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).