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@xxxxxxxxxxx>:
> > On 05/23/2016 01:05 PM, Wenwei Tao wrote:
> > > 2016-05-23 17:16 GMT+08:00 Matias BjÃrling <mb@xxxxxxxxxxx>:
> > > > On 05/23/2016 11:13 AM, Wenwei Tao wrote:
> > > > > From: Wenwei Tao <ww.tao0320@xxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxx>
> > > > > ---
> > > > >     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.