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

[Mark Ruijter <MRuijter@onestopsystems.com>]

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