Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
Hi Frans, On Tue, Jun 27, 2017 at 11:06:44AM +0200, Frans Klaver wrote: > On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørling wrote: > > From: Rakesh Pandit> > > > 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 > > Signed-off-by: Matias Bjørling > > --- > > drivers/lightnvm/core.c | 11 ++- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > > index b8f82f5..9ff348f 100644 > > --- a/drivers/lightnvm/core.c > > +++ b/drivers/lightnvm/core.c > > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > struct nvm_target *t; > > struct nvm_tgt_dev *tgt_dev; > > void *targetdata; > > - int ret; > > + int ret = 0; > > Is there any way that you can reach a 'return ret' without having ret > set by some other assignment? > > No. I should have been more careful. > > tt = nvm_find_target_type(create->tgttype, 1); > > if (!tt) { > > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > } > > mutex_unlock(>mlock); > > > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > > - return -ENOMEM; > > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > > + if (ret) > > + goto err; > > Why don't you return err straight away here? Intent was to future-proofing if num_reserve_luns would return anything other than -EBUSY and 0 but yes returning -EBUSY directly would be fine. > > > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > > if (!t) { > > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > mutex_lock(>mlock); > > list_add_tail(>list, >targets); > > mutex_unlock(>mlock); > > - > > - return 0; > > +err: > > + return ret; > > This should not be necessary. In any case, the de-init order should > always be the reverse of the init order, so we don't end up confused. Only if we directly return -EBUSY. Good point about getting confused I would resend quickly by directly returning error. That would not confuse folks. I would send an alternate patch which returns -EBUSY directly and do same thing. Thanks, > > Frans
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
Hi Frans, On Tue, Jun 27, 2017 at 11:06:44AM +0200, Frans Klaver wrote: > On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørling wrote: > > From: Rakesh Pandit > > > > 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 > > Signed-off-by: Matias Bjørling > > --- > > drivers/lightnvm/core.c | 11 ++- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > > index b8f82f5..9ff348f 100644 > > --- a/drivers/lightnvm/core.c > > +++ b/drivers/lightnvm/core.c > > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > struct nvm_target *t; > > struct nvm_tgt_dev *tgt_dev; > > void *targetdata; > > - int ret; > > + int ret = 0; > > Is there any way that you can reach a 'return ret' without having ret > set by some other assignment? > > No. I should have been more careful. > > tt = nvm_find_target_type(create->tgttype, 1); > > if (!tt) { > > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > } > > mutex_unlock(>mlock); > > > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > > - return -ENOMEM; > > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > > + if (ret) > > + goto err; > > Why don't you return err straight away here? Intent was to future-proofing if num_reserve_luns would return anything other than -EBUSY and 0 but yes returning -EBUSY directly would be fine. > > > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > > if (!t) { > > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > mutex_lock(>mlock); > > list_add_tail(>list, >targets); > > mutex_unlock(>mlock); > > - > > - return 0; > > +err: > > + return ret; > > This should not be necessary. In any case, the de-init order should > always be the reverse of the init order, so we don't end up confused. Only if we directly return -EBUSY. Good point about getting confused I would resend quickly by directly returning error. That would not confuse folks. I would send an alternate patch which returns -EBUSY directly and do same thing. Thanks, > > Frans
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørlingwrote: > From: Rakesh Pandit > > 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 > Signed-off-by: Matias Bjørling > --- > drivers/lightnvm/core.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > index b8f82f5..9ff348f 100644 > --- a/drivers/lightnvm/core.c > +++ b/drivers/lightnvm/core.c > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > struct nvm_target *t; > struct nvm_tgt_dev *tgt_dev; > void *targetdata; > - int ret; > + int ret = 0; Is there any way that you can reach a 'return ret' without having ret set by some other assignment? > tt = nvm_find_target_type(create->tgttype, 1); > if (!tt) { > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > } > mutex_unlock(>mlock); > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > - return -ENOMEM; > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > + if (ret) > + goto err; Why don't you return err straight away here? > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > if (!t) { > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > mutex_lock(>mlock); > list_add_tail(>list, >targets); > mutex_unlock(>mlock); > - > - return 0; > +err: > + return ret; This should not be necessary. In any case, the de-init order should always be the reverse of the init order, so we don't end up confused. Frans
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
On Tue, Jun 27, 2017 at 10:39 AM, Matias Bjørling wrote: > From: Rakesh Pandit > > 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 > Signed-off-by: Matias Bjørling > --- > drivers/lightnvm/core.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > index b8f82f5..9ff348f 100644 > --- a/drivers/lightnvm/core.c > +++ b/drivers/lightnvm/core.c > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > struct nvm_target *t; > struct nvm_tgt_dev *tgt_dev; > void *targetdata; > - int ret; > + int ret = 0; Is there any way that you can reach a 'return ret' without having ret set by some other assignment? > tt = nvm_find_target_type(create->tgttype, 1); > if (!tt) { > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > } > mutex_unlock(>mlock); > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > - return -ENOMEM; > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > + if (ret) > + goto err; Why don't you return err straight away here? > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > if (!t) { > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > mutex_lock(>mlock); > list_add_tail(>list, >targets); > mutex_unlock(>mlock); > - > - return 0; > +err: > + return ret; This should not be necessary. In any case, the de-init order should always be the reverse of the init order, so we don't end up confused. Frans
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
On Mon, May 29, 2017 at 11:05 AM, Rakesh Panditwrote: > Hi Matias, > > On Mon, May 15, 2017 at 06:31:58AM +, Javier Gonzalez wrote: >> > >> > On 13 May 2017, at 21.50, Rakesh Pandit 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 >> > --- >> > drivers/lightnvm/core.c | 11 ++- >> > 1 file changed, 6 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c >> > index 6a4aa60..440deb5 100644 >> > --- a/drivers/lightnvm/core.c >> > +++ b/drivers/lightnvm/core.c >> > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > struct nvm_target *t; >> > struct nvm_tgt_dev *tgt_dev; >> > void *targetdata; >> > - int ret; >> > + int ret = 0; >> > >> > tt = nvm_find_target_type(create->tgttype, 1); >> > if (!tt) { >> > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > } >> > mutex_unlock(>mlock); >> > >> > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) >> > - return -ENOMEM; >> > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); >> > + if (ret) >> > + goto err; >> > >> > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); >> > if (!t) { >> > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > mutex_lock(>mlock); >> > list_add_tail(>list, >targets); >> > mutex_unlock(>mlock); >> > - >> > - return 0; >> > +err: >> > + return ret; >> > err_sysfs: >> > if (tt->exit) >> > tt->exit(targetdata); >> > -- >> > 2.9.3 >> >> Looks good. >> >> Reviewed-by: Javier González >> > > May you review/approve this so that it gets queued ? Thanks, Rakesh. Will do. Queued it up for 4.13. -Matias
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
On Mon, May 29, 2017 at 11:05 AM, Rakesh Pandit wrote: > Hi Matias, > > On Mon, May 15, 2017 at 06:31:58AM +, Javier Gonzalez wrote: >> > >> > On 13 May 2017, at 21.50, Rakesh Pandit 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 >> > --- >> > drivers/lightnvm/core.c | 11 ++- >> > 1 file changed, 6 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c >> > index 6a4aa60..440deb5 100644 >> > --- a/drivers/lightnvm/core.c >> > +++ b/drivers/lightnvm/core.c >> > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > struct nvm_target *t; >> > struct nvm_tgt_dev *tgt_dev; >> > void *targetdata; >> > - int ret; >> > + int ret = 0; >> > >> > tt = nvm_find_target_type(create->tgttype, 1); >> > if (!tt) { >> > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > } >> > mutex_unlock(>mlock); >> > >> > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) >> > - return -ENOMEM; >> > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); >> > + if (ret) >> > + goto err; >> > >> > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); >> > if (!t) { >> > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct >> > nvm_ioctl_create *create) >> > mutex_lock(>mlock); >> > list_add_tail(>list, >targets); >> > mutex_unlock(>mlock); >> > - >> > - return 0; >> > +err: >> > + return ret; >> > err_sysfs: >> > if (tt->exit) >> > tt->exit(targetdata); >> > -- >> > 2.9.3 >> >> Looks good. >> >> Reviewed-by: Javier González >> > > May you review/approve this so that it gets queued ? Thanks, Rakesh. Will do. Queued it up for 4.13. -Matias
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
Hi Matias, On Mon, May 15, 2017 at 06:31:58AM +, Javier Gonzalez wrote: > > > > On 13 May 2017, at 21.50, Rakesh Panditwrote: > > > > 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 > > --- > > drivers/lightnvm/core.c | 11 ++- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > > index 6a4aa60..440deb5 100644 > > --- a/drivers/lightnvm/core.c > > +++ b/drivers/lightnvm/core.c > > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > struct nvm_target *t; > > struct nvm_tgt_dev *tgt_dev; > > void *targetdata; > > - int ret; > > + int ret = 0; > > > > tt = nvm_find_target_type(create->tgttype, 1); > > if (!tt) { > > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > } > > mutex_unlock(>mlock); > > > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > > - return -ENOMEM; > > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > > + if (ret) > > + goto err; > > > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > > if (!t) { > > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > mutex_lock(>mlock); > > list_add_tail(>list, >targets); > > mutex_unlock(>mlock); > > - > > - return 0; > > +err: > > + return ret; > > err_sysfs: > > if (tt->exit) > > tt->exit(targetdata); > > -- > > 2.9.3 > > Looks good. > > Reviewed-by: Javier González > May you review/approve this so that it gets queued ?
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
Hi Matias, On Mon, May 15, 2017 at 06:31:58AM +, Javier Gonzalez wrote: > > > > On 13 May 2017, at 21.50, Rakesh Pandit 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 > > --- > > drivers/lightnvm/core.c | 11 ++- > > 1 file changed, 6 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > > index 6a4aa60..440deb5 100644 > > --- a/drivers/lightnvm/core.c > > +++ b/drivers/lightnvm/core.c > > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > struct nvm_target *t; > > struct nvm_tgt_dev *tgt_dev; > > void *targetdata; > > - int ret; > > + int ret = 0; > > > > tt = nvm_find_target_type(create->tgttype, 1); > > if (!tt) { > > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > } > > mutex_unlock(>mlock); > > > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > > - return -ENOMEM; > > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > > + if (ret) > > + goto err; > > > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > > if (!t) { > > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > > nvm_ioctl_create *create) > > mutex_lock(>mlock); > > list_add_tail(>list, >targets); > > mutex_unlock(>mlock); > > - > > - return 0; > > +err: > > + return ret; > > err_sysfs: > > if (tt->exit) > > tt->exit(targetdata); > > -- > > 2.9.3 > > Looks good. > > Reviewed-by: Javier González > May you review/approve this so that it gets queued ?
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
> > On 13 May 2017, at 21.50, Rakesh Panditwrote: > > 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 > --- > drivers/lightnvm/core.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > index 6a4aa60..440deb5 100644 > --- a/drivers/lightnvm/core.c > +++ b/drivers/lightnvm/core.c > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > struct nvm_target *t; > struct nvm_tgt_dev *tgt_dev; > void *targetdata; > - int ret; > + int ret = 0; > > tt = nvm_find_target_type(create->tgttype, 1); > if (!tt) { > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > } > mutex_unlock(>mlock); > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > - return -ENOMEM; > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > + if (ret) > + goto err; > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > if (!t) { > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > mutex_lock(>mlock); > list_add_tail(>list, >targets); > mutex_unlock(>mlock); > - > - return 0; > +err: > + return ret; > err_sysfs: > if (tt->exit) > tt->exit(targetdata); > -- > 2.9.3 Looks good. Reviewed-by: Javier González signature.asc Description: Message signed with OpenPGP
Re: [PATCH] ligtnvm: if LUNs are already allocated fix return
> > On 13 May 2017, at 21.50, Rakesh Pandit 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 > --- > drivers/lightnvm/core.c | 11 ++- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c > index 6a4aa60..440deb5 100644 > --- a/drivers/lightnvm/core.c > +++ b/drivers/lightnvm/core.c > @@ -235,7 +235,7 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > struct nvm_target *t; > struct nvm_tgt_dev *tgt_dev; > void *targetdata; > - int ret; > + int ret = 0; > > tt = nvm_find_target_type(create->tgttype, 1); > if (!tt) { > @@ -252,8 +252,9 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > } > mutex_unlock(>mlock); > > - if (nvm_reserve_luns(dev, s->lun_begin, s->lun_end)) > - return -ENOMEM; > + ret = nvm_reserve_luns(dev, s->lun_begin, s->lun_end); > + if (ret) > + goto err; > > t = kmalloc(sizeof(struct nvm_target), GFP_KERNEL); > if (!t) { > @@ -314,8 +315,8 @@ static int nvm_create_tgt(struct nvm_dev *dev, struct > nvm_ioctl_create *create) > mutex_lock(>mlock); > list_add_tail(>list, >targets); > mutex_unlock(>mlock); > - > - return 0; > +err: > + return ret; > err_sysfs: > if (tt->exit) > tt->exit(targetdata); > -- > 2.9.3 Looks good. Reviewed-by: Javier González signature.asc Description: Message signed with OpenPGP