Re: [PATCH] nvmet: make ctrl-id configurable

[James Smart <james.smart@broadcom.com>]

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