[PATCH v2 1/2] lightnvm: hold lock until finish the target creation

[PATCH v2 1/2] lightnvm: hold lock until finish the target creation

[Wenwei Tao]

From: Wenwei Tao <ww.tao0320@gmail.com>

When create a target, we check whether the target is
already exist first. If the answer is no, we release
the lock and continue the creation. This cannot prevent
concurrent creation of the same target, so hold the lock
until finish the target creation.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
Changes since v1
-rebase to for-4.8/core

 drivers/lightnvm/gennvm.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
index c65fb67..39ff0af 100644
--- a/drivers/lightnvm/gennvm.c
+++ b/drivers/lightnvm/gennvm.c
@@ -44,6 +44,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	struct nvm_tgt_type *tt;
 	struct nvm_target *t;
 	void *targetdata;
+	int ret = -ENOMEM;
 
 	tt = nvm_find_target_type(create->tgttype, 1);
 	if (!tt) {
@@ -55,14 +56,13 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	t = gen_find_target(gn, create->tgtname);
 	if (t) {
 		pr_err("nvm: target name already exists.\n");
-		mutex_unlock(&gn->lock);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err_unlock;
 	}
-	mutex_unlock(&gn->lock);
 
 	t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL);
 	if (!t)
-		return -ENOMEM;
+		goto err_unlock;
 
 	tqueue = blk_alloc_queue_node(GFP_KERNEL, dev->q->node);
 	if (!tqueue)
@@ -95,8 +95,6 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	t->type = tt;
 	t->disk = tdisk;
 	t->dev = dev;
-
-	mutex_lock(&gn->lock);
 	list_add_tail(&t->list, &gn->targets);
 	mutex_unlock(&gn->lock);
 
@@ -107,7 +105,9 @@ err_queue:
 	blk_cleanup_queue(tqueue);
 err_t:
 	kfree(t);
-	return -ENOMEM;
+err_unlock:
+	mutex_unlock(&gn->lock);
+	return ret;
 }
 
 static void __gen_remove_target(struct nvm_target *t)
-- 
1.8.3.1

[RFC PATCH 2/2] lightnvm: Append device name to target name

[Wenwei Tao]

From: Wenwei Tao <ww.tao0320@gmail.com>

We may create targets with same name on different
backend devices, this is not what we want, so append
the device name to target name to make the new target
name unique in the system.

Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
---
 drivers/lightnvm/gennvm.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
index 39ff0af..ecb09cb 100644
--- a/drivers/lightnvm/gennvm.c
+++ b/drivers/lightnvm/gennvm.c
@@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	struct gendisk *tdisk;
 	struct nvm_tgt_type *tt;
 	struct nvm_target *t;
+	char tgtname[DISK_NAME_LEN];
 	void *targetdata;
 	int ret = -ENOMEM;
 
+	if (strlen(dev->name) + strlen(create->tgtname) + 1 > DISK_NAME_LEN) {
+		pr_err("nvm: target name too long. %s:%s\n",
+				dev->name, create->tgtname);
+		return -EINVAL;
+	}
+
+	sprintf(tgtname, "%s%s", dev->name, create->tgtname);
+
 	tt = nvm_find_target_type(create->tgttype, 1);
 	if (!tt) {
 		pr_err("nvm: target type %s not found\n", create->tgttype);
@@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	}
 
 	mutex_lock(&gn->lock);
-	t = gen_find_target(gn, create->tgtname);
+	t = gen_find_target(gn, tgtname);
 	if (t) {
 		pr_err("nvm: target name already exists.\n");
 		ret = -EINVAL;
@@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
 	if (!tdisk)
 		goto err_queue;
 
-	sprintf(tdisk->disk_name, "%s", create->tgtname);
+	sprintf(tdisk->disk_name, "%s", tgtname);
 	tdisk->flags = GENHD_FL_EXT_DEVT;
 	tdisk->major = 0;
 	tdisk->first_minor = 0;
-- 
1.8.3.1

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Matias Bjørling]

On 05/23/2016 11:13 AM, Wenwei Tao wrote:
> From: Wenwei Tao <ww.tao0320@gmail.com>
>
> We may create targets with same name on different
> backend devices, this is not what we want, so append
> the device name to target name to make the new target
> name unique in the system.
>
> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
> ---
>   drivers/lightnvm/gennvm.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
> index 39ff0af..ecb09cb 100644
> --- a/drivers/lightnvm/gennvm.c
> +++ b/drivers/lightnvm/gennvm.c
> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>   	struct gendisk *tdisk;
>   	struct nvm_tgt_type *tt;
>   	struct nvm_target *t;
> +	char tgtname[DISK_NAME_LEN];
>   	void *targetdata;
>   	int ret = -ENOMEM;
>
> +	if (strlen(dev->name) + strlen(create->tgtname) + 1 > DISK_NAME_LEN) {
> +		pr_err("nvm: target name too long. %s:%s\n",
> +				dev->name, create->tgtname);
> +		return -EINVAL;
> +	}
> +
> +	sprintf(tgtname, "%s%s", dev->name, create->tgtname);
> +
>   	tt = nvm_find_target_type(create->tgttype, 1);
>   	if (!tt) {
>   		pr_err("nvm: target type %s not found\n", create->tgttype);
> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>   	}
>
>   	mutex_lock(&gn->lock);
> -	t = gen_find_target(gn, create->tgtname);
> +	t = gen_find_target(gn, tgtname);
>   	if (t) {
>   		pr_err("nvm: target name already exists.\n");
>   		ret = -EINVAL;
> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create)
>   	if (!tdisk)
>   		goto err_queue;
>
> -	sprintf(tdisk->disk_name, "%s", create->tgtname);
> +	sprintf(tdisk->disk_name, "%s", tgtname);
>   	tdisk->flags = GENHD_FL_EXT_DEVT;
>   	tdisk->major = 0;
>   	tdisk->first_minor = 0;
>

Hi Wenwei, what about the case where a target instance has multiple 
devices associated?

I am okay with having the user choosing a unique name for the target to 
be exposed.

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Wenwei Tao]

2016-05-23 17:16 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 05/23/2016 11:13 AM, Wenwei Tao wrote:
>>
>> From: Wenwei Tao <ww.tao0320@gmail.com>
>>
>> We may create targets with same name on different
>> backend devices, this is not what we want, so append
>> the device name to target name to make the new target
>> name unique in the system.
>>
>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>> ---
>>   drivers/lightnvm/gennvm.c | 13 +++++++++++--
>>   1 file changed, 11 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
>> index 39ff0af..ecb09cb 100644
>> --- a/drivers/lightnvm/gennvm.c
>> +++ b/drivers/lightnvm/gennvm.c
>> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>> nvm_ioctl_create *create)
>>         struct gendisk *tdisk;
>>         struct nvm_tgt_type *tt;
>>         struct nvm_target *t;
>> +       char tgtname[DISK_NAME_LEN];
>>         void *targetdata;
>>         int ret = -ENOMEM;
>>
>> +       if (strlen(dev->name) + strlen(create->tgtname) + 1 >
>> DISK_NAME_LEN) {
>> +               pr_err("nvm: target name too long. %s:%s\n",
>> +                               dev->name, create->tgtname);
>> +               return -EINVAL;
>> +       }
>> +
>> +       sprintf(tgtname, "%s%s", dev->name, create->tgtname);
>> +
>>         tt = nvm_find_target_type(create->tgttype, 1);
>>         if (!tt) {
>>                 pr_err("nvm: target type %s not found\n",
>> create->tgttype);
>> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>> nvm_ioctl_create *create)
>>         }
>>
>>         mutex_lock(&gn->lock);
>> -       t = gen_find_target(gn, create->tgtname);
>> +       t = gen_find_target(gn, tgtname);
>>         if (t) {
>>                 pr_err("nvm: target name already exists.\n");
>>                 ret = -EINVAL;
>> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>> nvm_ioctl_create *create)
>>         if (!tdisk)
>>                 goto err_queue;
>>
>> -       sprintf(tdisk->disk_name, "%s", create->tgtname);
>> +       sprintf(tdisk->disk_name, "%s", tgtname);
>>         tdisk->flags = GENHD_FL_EXT_DEVT;
>>         tdisk->major = 0;
>>         tdisk->first_minor = 0;
>>
>
> Hi Wenwei, what about the case where a target instance has multiple devices
> associated?
>
You mean a target may be build on multiple backend devices ?

> I am okay with having the user choosing a unique name for the target to be
> exposed.
You mean user should check the name before create the target?
Before move target mgmt into media mgr, that would be okay(after apply
lightnvm: hold lock until finish the target creation), since all
targets are in the global list.
Now consider below case:
There are two users A and B. A want to create target test0 upon
device0 B want to create test0 upon device1,
before creation they both check whether test0 is exist (e.g. by list
/dev/test0) , they all find test0 is not exist now, and they continue
their
creation. Both of them use disk name test0 to call add_disk, that
would cause panic.

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Matias Bjørling]

On 05/23/2016 01:05 PM, Wenwei Tao wrote:
> 2016-05-23 17:16 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>> On 05/23/2016 11:13 AM, Wenwei Tao wrote:
>>>
>>> From: Wenwei Tao <ww.tao0320@gmail.com>
>>>
>>> We may create targets with same name on different
>>> backend devices, this is not what we want, so append
>>> the device name to target name to make the new target
>>> name unique in the system.
>>>
>>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>>> ---
>>>    drivers/lightnvm/gennvm.c | 13 +++++++++++--
>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
>>> index 39ff0af..ecb09cb 100644
>>> --- a/drivers/lightnvm/gennvm.c
>>> +++ b/drivers/lightnvm/gennvm.c
>>> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>> nvm_ioctl_create *create)
>>>          struct gendisk *tdisk;
>>>          struct nvm_tgt_type *tt;
>>>          struct nvm_target *t;
>>> +       char tgtname[DISK_NAME_LEN];
>>>          void *targetdata;
>>>          int ret = -ENOMEM;
>>>
>>> +       if (strlen(dev->name) + strlen(create->tgtname) + 1 >
>>> DISK_NAME_LEN) {
>>> +               pr_err("nvm: target name too long. %s:%s\n",
>>> +                               dev->name, create->tgtname);
>>> +               return -EINVAL;
>>> +       }
>>> +
>>> +       sprintf(tgtname, "%s%s", dev->name, create->tgtname);
>>> +
>>>          tt = nvm_find_target_type(create->tgttype, 1);
>>>          if (!tt) {
>>>                  pr_err("nvm: target type %s not found\n",
>>> create->tgttype);
>>> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>> nvm_ioctl_create *create)
>>>          }
>>>
>>>          mutex_lock(&gn->lock);
>>> -       t = gen_find_target(gn, create->tgtname);
>>> +       t = gen_find_target(gn, tgtname);
>>>          if (t) {
>>>                  pr_err("nvm: target name already exists.\n");
>>>                  ret = -EINVAL;
>>> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>> nvm_ioctl_create *create)
>>>          if (!tdisk)
>>>                  goto err_queue;
>>>
>>> -       sprintf(tdisk->disk_name, "%s", create->tgtname);
>>> +       sprintf(tdisk->disk_name, "%s", tgtname);
>>>          tdisk->flags = GENHD_FL_EXT_DEVT;
>>>          tdisk->major = 0;
>>>          tdisk->first_minor = 0;
>>>
>>
>> Hi Wenwei, what about the case where a target instance has multiple devices
>> associated?
>>
> You mean a target may be build on multiple backend devices ?

Yes. Over time, we want a single target to manage many devices.

>
>> I am okay with having the user choosing a unique name for the target to be
>> exposed.
> You mean user should check the name before create the target?

Sure. It is him that decides the name of the device. Your lock patch 
fixes the panic that could happen. I am happy with that.

> Before move target mgmt into media mgr, that would be okay(after apply
> lightnvm: hold lock until finish the target creation), since all
> targets are in the global list.
> Now consider below case:
> There are two users A and B. A want to create target test0 upon
> device0 B want to create test0 upon device1,
> before creation they both check whether test0 is exist (e.g. by list
> /dev/test0) , they all find test0 is not exist now, and they continue
> their
> creation. Both of them use disk name test0 to call add_disk, that
> would cause panic.
>

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Wenwei Tao]

Eh.. my lock patch can only prevent concurrent creation of the same
name target on the same backend device, not the concurrent creation of
same name target on different backend devices, since target management
is protect by per  device's gn->lock not
the global nvm_lock now.

2016-05-23 20:24 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 05/23/2016 01:05 PM, Wenwei Tao wrote:
>>
>> 2016-05-23 17:16 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>>>
>>> On 05/23/2016 11:13 AM, Wenwei Tao wrote:
>>>>
>>>>
>>>> From: Wenwei Tao <ww.tao0320@gmail.com>
>>>>
>>>> We may create targets with same name on different
>>>> backend devices, this is not what we want, so append
>>>> the device name to target name to make the new target
>>>> name unique in the system.
>>>>
>>>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>>>> ---
>>>>    drivers/lightnvm/gennvm.c | 13 +++++++++++--
>>>>    1 file changed, 11 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
>>>> index 39ff0af..ecb09cb 100644
>>>> --- a/drivers/lightnvm/gennvm.c
>>>> +++ b/drivers/lightnvm/gennvm.c
>>>> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>> nvm_ioctl_create *create)
>>>>          struct gendisk *tdisk;
>>>>          struct nvm_tgt_type *tt;
>>>>          struct nvm_target *t;
>>>> +       char tgtname[DISK_NAME_LEN];
>>>>          void *targetdata;
>>>>          int ret = -ENOMEM;
>>>>
>>>> +       if (strlen(dev->name) + strlen(create->tgtname) + 1 >
>>>> DISK_NAME_LEN) {
>>>> +               pr_err("nvm: target name too long. %s:%s\n",
>>>> +                               dev->name, create->tgtname);
>>>> +               return -EINVAL;
>>>> +       }
>>>> +
>>>> +       sprintf(tgtname, "%s%s", dev->name, create->tgtname);
>>>> +
>>>>          tt = nvm_find_target_type(create->tgttype, 1);
>>>>          if (!tt) {
>>>>                  pr_err("nvm: target type %s not found\n",
>>>> create->tgttype);
>>>> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>> nvm_ioctl_create *create)
>>>>          }
>>>>
>>>>          mutex_lock(&gn->lock);
>>>> -       t = gen_find_target(gn, create->tgtname);
>>>> +       t = gen_find_target(gn, tgtname);
>>>>          if (t) {
>>>>                  pr_err("nvm: target name already exists.\n");
>>>>                  ret = -EINVAL;
>>>> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>> nvm_ioctl_create *create)
>>>>          if (!tdisk)
>>>>                  goto err_queue;
>>>>
>>>> -       sprintf(tdisk->disk_name, "%s", create->tgtname);
>>>> +       sprintf(tdisk->disk_name, "%s", tgtname);
>>>>          tdisk->flags = GENHD_FL_EXT_DEVT;
>>>>          tdisk->major = 0;
>>>>          tdisk->first_minor = 0;
>>>>
>>>
>>> Hi Wenwei, what about the case where a target instance has multiple
>>> devices
>>> associated?
>>>
>> You mean a target may be build on multiple backend devices ?
>
>
> Yes. Over time, we want a single target to manage many devices.
>
>>
>>> I am okay with having the user choosing a unique name for the target to
>>> be
>>> exposed.
>>
>> You mean user should check the name before create the target?
>
>
> Sure. It is him that decides the name of the device. Your lock patch fixes
> the panic that could happen. I am happy with that.
>
>
>> Before move target mgmt into media mgr, that would be okay(after apply
>> lightnvm: hold lock until finish the target creation), since all
>> targets are in the global list.
>> Now consider below case:
>> There are two users A and B. A want to create target test0 upon
>> device0 B want to create test0 upon device1,
>> before creation they both check whether test0 is exist (e.g. by list
>> /dev/test0) , they all find test0 is not exist now, and they continue
>> their
>> creation. Both of them use disk name test0 to call add_disk, that
>> would cause panic.
>>
>

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Matias Bjørling]

On 05/23/2016 03:31 PM, Wenwei Tao wrote:
> Eh.. my lock patch can only prevent concurrent creation of the same
> name target on the same backend device, not the concurrent creation of
> same name target on different backend devices, since target management
> is protect by per  device's gn->lock not
> the global nvm_lock now.
>
> 2016-05-23 20:24 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>> On 05/23/2016 01:05 PM, Wenwei Tao wrote:
>>>
>>> 2016-05-23 17:16 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>>>>
>>>> On 05/23/2016 11:13 AM, Wenwei Tao wrote:
>>>>>
>>>>>
>>>>> From: Wenwei Tao <ww.tao0320@gmail.com>
>>>>>
>>>>> We may create targets with same name on different
>>>>> backend devices, this is not what we want, so append
>>>>> the device name to target name to make the new target
>>>>> name unique in the system.
>>>>>
>>>>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>>>>> ---
>>>>>     drivers/lightnvm/gennvm.c | 13 +++++++++++--
>>>>>     1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
>>>>> index 39ff0af..ecb09cb 100644
>>>>> --- a/drivers/lightnvm/gennvm.c
>>>>> +++ b/drivers/lightnvm/gennvm.c
>>>>> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>>> nvm_ioctl_create *create)
>>>>>           struct gendisk *tdisk;
>>>>>           struct nvm_tgt_type *tt;
>>>>>           struct nvm_target *t;
>>>>> +       char tgtname[DISK_NAME_LEN];
>>>>>           void *targetdata;
>>>>>           int ret = -ENOMEM;
>>>>>
>>>>> +       if (strlen(dev->name) + strlen(create->tgtname) + 1 >
>>>>> DISK_NAME_LEN) {
>>>>> +               pr_err("nvm: target name too long. %s:%s\n",
>>>>> +                               dev->name, create->tgtname);
>>>>> +               return -EINVAL;
>>>>> +       }
>>>>> +
>>>>> +       sprintf(tgtname, "%s%s", dev->name, create->tgtname);
>>>>> +
>>>>>           tt = nvm_find_target_type(create->tgttype, 1);
>>>>>           if (!tt) {
>>>>>                   pr_err("nvm: target type %s not found\n",
>>>>> create->tgttype);
>>>>> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>>> nvm_ioctl_create *create)
>>>>>           }
>>>>>
>>>>>           mutex_lock(&gn->lock);
>>>>> -       t = gen_find_target(gn, create->tgtname);
>>>>> +       t = gen_find_target(gn, tgtname);
>>>>>           if (t) {
>>>>>                   pr_err("nvm: target name already exists.\n");
>>>>>                   ret = -EINVAL;
>>>>> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev, struct
>>>>> nvm_ioctl_create *create)
>>>>>           if (!tdisk)
>>>>>                   goto err_queue;
>>>>>
>>>>> -       sprintf(tdisk->disk_name, "%s", create->tgtname);
>>>>> +       sprintf(tdisk->disk_name, "%s", tgtname);
>>>>>           tdisk->flags = GENHD_FL_EXT_DEVT;
>>>>>           tdisk->major = 0;
>>>>>           tdisk->first_minor = 0;
>>>>>
>>>>
>>>> Hi Wenwei, what about the case where a target instance has multiple
>>>> devices
>>>> associated?
>>>>
>>> You mean a target may be build on multiple backend devices ?
>>
>>
>> Yes. Over time, we want a single target to manage many devices.
>>
>>>
>>>> I am okay with having the user choosing a unique name for the target to
>>>> be
>>>> exposed.
>>>
>>> You mean user should check the name before create the target?
>>
>>
>> Sure. It is him that decides the name of the device. Your lock patch fixes
>> the panic that could happen. I am happy with that.
>>
>>
>>> Before move target mgmt into media mgr, that would be okay(after apply
>>> lightnvm: hold lock until finish the target creation), since all
>>> targets are in the global list.
>>> Now consider below case:
>>> There are two users A and B. A want to create target test0 upon
>>> device0 B want to create test0 upon device1,
>>> before creation they both check whether test0 is exist (e.g. by list
>>> /dev/test0) , they all find test0 is not exist now, and they continue
>>> their
>>> creation. Both of them use disk name test0 to call add_disk, that
>>> would cause panic.
>>>
>>

You are right. The right way is properly to not allow the user to define 
a device name, and instead rely on generic naming and UUID.

Re: [RFC PATCH 2/2] lightnvm: Append device name to target name

[Wenwei Tao]

It's fine to allow the user to define a device name as long as hold
the global lock and link the targets into a global list but that may
against the idea to move the target management into media mgr.

2016-05-24 22:17 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
> On 05/23/2016 03:31 PM, Wenwei Tao wrote:
>>
>> Eh.. my lock patch can only prevent concurrent creation of the same
>> name target on the same backend device, not the concurrent creation of
>> same name target on different backend devices, since target management
>> is protect by per  device's gn->lock not
>> the global nvm_lock now.
>>
>> 2016-05-23 20:24 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>>>
>>> On 05/23/2016 01:05 PM, Wenwei Tao wrote:
>>>>
>>>>
>>>> 2016-05-23 17:16 GMT+08:00 Matias Bjørling <mb@lightnvm.io>:
>>>>>
>>>>>
>>>>> On 05/23/2016 11:13 AM, Wenwei Tao wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> From: Wenwei Tao <ww.tao0320@gmail.com>
>>>>>>
>>>>>> We may create targets with same name on different
>>>>>> backend devices, this is not what we want, so append
>>>>>> the device name to target name to make the new target
>>>>>> name unique in the system.
>>>>>>
>>>>>> Signed-off-by: Wenwei Tao <ww.tao0320@gmail.com>
>>>>>> ---
>>>>>>     drivers/lightnvm/gennvm.c | 13 +++++++++++--
>>>>>>     1 file changed, 11 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/lightnvm/gennvm.c b/drivers/lightnvm/gennvm.c
>>>>>> index 39ff0af..ecb09cb 100644
>>>>>> --- a/drivers/lightnvm/gennvm.c
>>>>>> +++ b/drivers/lightnvm/gennvm.c
>>>>>> @@ -43,9 +43,18 @@ static int gen_create_tgt(struct nvm_dev *dev,
>>>>>> struct
>>>>>> nvm_ioctl_create *create)
>>>>>>           struct gendisk *tdisk;
>>>>>>           struct nvm_tgt_type *tt;
>>>>>>           struct nvm_target *t;
>>>>>> +       char tgtname[DISK_NAME_LEN];
>>>>>>           void *targetdata;
>>>>>>           int ret = -ENOMEM;
>>>>>>
>>>>>> +       if (strlen(dev->name) + strlen(create->tgtname) + 1 >
>>>>>> DISK_NAME_LEN) {
>>>>>> +               pr_err("nvm: target name too long. %s:%s\n",
>>>>>> +                               dev->name, create->tgtname);
>>>>>> +               return -EINVAL;
>>>>>> +       }
>>>>>> +
>>>>>> +       sprintf(tgtname, "%s%s", dev->name, create->tgtname);
>>>>>> +
>>>>>>           tt = nvm_find_target_type(create->tgttype, 1);
>>>>>>           if (!tt) {
>>>>>>                   pr_err("nvm: target type %s not found\n",
>>>>>> create->tgttype);
>>>>>> @@ -53,7 +62,7 @@ static int gen_create_tgt(struct nvm_dev *dev,
>>>>>> struct
>>>>>> nvm_ioctl_create *create)
>>>>>>           }
>>>>>>
>>>>>>           mutex_lock(&gn->lock);
>>>>>> -       t = gen_find_target(gn, create->tgtname);
>>>>>> +       t = gen_find_target(gn, tgtname);
>>>>>>           if (t) {
>>>>>>                   pr_err("nvm: target name already exists.\n");
>>>>>>                   ret = -EINVAL;
>>>>>> @@ -73,7 +82,7 @@ static int gen_create_tgt(struct nvm_dev *dev,
>>>>>> struct
>>>>>> nvm_ioctl_create *create)
>>>>>>           if (!tdisk)
>>>>>>                   goto err_queue;
>>>>>>
>>>>>> -       sprintf(tdisk->disk_name, "%s", create->tgtname);
>>>>>> +       sprintf(tdisk->disk_name, "%s", tgtname);
>>>>>>           tdisk->flags = GENHD_FL_EXT_DEVT;
>>>>>>           tdisk->major = 0;
>>>>>>           tdisk->first_minor = 0;
>>>>>>
>>>>>
>>>>> Hi Wenwei, what about the case where a target instance has multiple
>>>>> devices
>>>>> associated?
>>>>>
>>>> You mean a target may be build on multiple backend devices ?
>>>
>>>
>>>
>>> Yes. Over time, we want a single target to manage many devices.
>>>
>>>>
>>>>> I am okay with having the user choosing a unique name for the target to
>>>>> be
>>>>> exposed.
>>>>
>>>>
>>>> You mean user should check the name before create the target?
>>>
>>>
>>>
>>> Sure. It is him that decides the name of the device. Your lock patch
>>> fixes
>>> the panic that could happen. I am happy with that.
>>>
>>>
>>>> Before move target mgmt into media mgr, that would be okay(after apply
>>>> lightnvm: hold lock until finish the target creation), since all
>>>> targets are in the global list.
>>>> Now consider below case:
>>>> There are two users A and B. A want to create target test0 upon
>>>> device0 B want to create test0 upon device1,
>>>> before creation they both check whether test0 is exist (e.g. by list
>>>> /dev/test0) , they all find test0 is not exist now, and they continue
>>>> their
>>>> creation. Both of them use disk name test0 to call add_disk, that
>>>> would cause panic.
>>>>
>>>
>
> You are right. The right way is properly to not allow the user to define a
> device name, and instead rely on generic naming and UUID.