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

["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.