| Message ID | 20170627104315.GA52146@dhcp-216.srv.tuxera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: > While creating new device with NVM_DEV_CREATE if LUNs are already > allocated ioctl would return -ENOMEM which is wrong. This patch > propagates -EBUSY from nvm_reserve_luns which is correct response. > > Fixes: ade69e243 ("lightnvm: merge gennvm with core") > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> > --- > > V2: return error code directly instead of using ret variable (Frans) > > drivers/lightnvm/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > index b8f82f5..c5d71c6 100644 > --- a/drivers/lightnvm/core.c > +++ b/drivers/lightnvm/core.c > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create) > mutex_unlock(&dev->mlock); > > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > - return -ENOMEM; > + return -EBUSY; Why aren't you propagating ret in this version?
On Tue, Jun 27, 2017 at 01:01:22PM +0200, Frans Klaver wrote: > On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: > > While creating new device with NVM_DEV_CREATE if LUNs are already > > allocated ioctl would return -ENOMEM which is wrong. This patch > > propagates -EBUSY from nvm_reserve_luns which is correct response. > > > > Fixes: ade69e243 ("lightnvm: merge gennvm with core") > > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> > > --- > > > > V2: return error code directly instead of using ret variable (Frans) > > > > drivers/lightnvm/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > > index b8f82f5..c5d71c6 100644 > > --- a/drivers/lightnvm/core.c > > +++ b/drivers/lightnvm/core.c > > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create) > > mutex_unlock(&dev->mlock); > > > > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > > - return -ENOMEM; > > + return -EBUSY; > > Why aren't you propagating ret in this version? Well nvm_reserve_luns either returns 0 or -EBUSY and it is unlikely that return value would change and even if it does this can be updated.
On Tue, Jun 27, 2017 at 1:23 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: > On Tue, Jun 27, 2017 at 01:01:22PM +0200, Frans Klaver wrote: >> On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: >> > While creating new device with NVM_DEV_CREATE if LUNs are already >> > allocated ioctl would return -ENOMEM which is wrong. This patch >> > propagates -EBUSY from nvm_reserve_luns which is correct response. >> > >> > Fixes: ade69e243 ("lightnvm: merge gennvm with core") >> > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> >> > --- >> > >> > V2: return error code directly instead of using ret variable (Frans) >> > >> > drivers/lightnvm/core.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c >> > index b8f82f5..c5d71c6 100644 >> > --- a/drivers/lightnvm/core.c >> > +++ b/drivers/lightnvm/core.c >> > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create) >> > mutex_unlock(&dev->mlock); >> > >> > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) >> > - return -ENOMEM; >> > + return -EBUSY; >> >> Why aren't you propagating ret in this version? > > Well nvm_reserve_luns either returns 0 or -EBUSY and it is unlikely > that return value would change and even if it does this can be > updated. If you propagate the result of nvm_reserve_luns(), the casual reader will immediately understand that any possible faulty result is returned. returning -EBUSY here might suggest you're overriding whatever this function returns.
On Tue, Jun 27, 2017 at 01:27:40PM +0200, Frans Klaver wrote: > On Tue, Jun 27, 2017 at 1:23 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: > > On Tue, Jun 27, 2017 at 01:01:22PM +0200, Frans Klaver wrote: > >> On Tue, Jun 27, 2017 at 12:43 PM, Rakesh Pandit <rakesh@tuxera.com> wrote: > >> > While creating new device with NVM_DEV_CREATE if LUNs are already > >> > allocated ioctl would return -ENOMEM which is wrong. This patch > >> > propagates -EBUSY from nvm_reserve_luns which is correct response. > >> > > >> > Fixes: ade69e243 ("lightnvm: merge gennvm with core") > >> > Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> > >> > --- > >> > > >> > V2: return error code directly instead of using ret variable (Frans) > >> > > >> > drivers/lightnvm/core.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > >> > index b8f82f5..c5d71c6 100644 > >> > --- a/drivers/lightnvm/core.c > >> > +++ b/drivers/lightnvm/core.c > >> > @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create) > >> > mutex_unlock(&dev->mlock); > >> > > >> > if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > >> > - return -ENOMEM; > >> > + return -EBUSY; > >> > >> Why aren't you propagating ret in this version? > > > > Well nvm_reserve_luns either returns 0 or -EBUSY and it is unlikely > > that return value would change and even if it does this can be > > updated. > > If you propagate the result of nvm_reserve_luns(), the casual reader > will immediately understand that any possible faulty result is > returned. returning -EBUSY here might suggest you're overriding > whatever this function returns. I don't have a strong opinion for or against. That was being done irrespective of this change already. But you have a valid argument. I would post V3. Thanks,
diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c index b8f82f5..c5d71c6 100644 --- a/drivers/lightnvm/core.c +++ b/drivers/lightnvm/core.c @@ -253,7 +253,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct nvm_ioctl_create *create) mutex_unlock(&dev->mlock); if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) - return -ENOMEM; + return -EBUSY; t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); if (!t) {
While creating new device with NVM_DEV_CREATE if LUNs are already allocated ioctl would return -ENOMEM which is wrong. This patch propagates -EBUSY from nvm_reserve_luns which is correct response. Fixes: ade69e243 ("lightnvm: merge gennvm with core") Signed-off-by: Rakesh Pandit <rakesh@tuxera.com> --- V2: return error code directly instead of using ret variable (Frans) drivers/lightnvm/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)