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

[Mark Ruijter <MRuijter@onestopsystems.com>]

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