| Message ID | 20190227171442.11853-3-igor.j.konopko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: bugfixes and improvements | expand |
> On 27 Feb 2019, at 12.14, Igor Konopko <igor.j.konopko@intel.com> wrote: > > Currently when we fail on gc rq data allocation > we simply skip the data which we wanted to move > and finally move the line to free state and lose > that data due to that. This patch move the data > allocation to some earlier phase of GC, where we > can still fail gracefully by moving line back > to closed state. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-gc.c | 19 +++++++++---------- > 1 file changed, 9 insertions(+), 10 deletions(-) > > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > index 3feadfd9418d..31fc1339faa8 100644 > --- a/drivers/lightnvm/pblk-gc.c > +++ b/drivers/lightnvm/pblk-gc.c > @@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work) > struct pblk_line_ws *gc_rq_ws = container_of(work, > struct pblk_line_ws, ws); > struct pblk *pblk = gc_rq_ws->pblk; > - struct nvm_tgt_dev *dev = pblk->dev; > - struct nvm_geo *geo = &dev->geo; > struct pblk_gc *gc = &pblk->gc; > struct pblk_line *line = gc_rq_ws->line; > struct pblk_gc_rq *gc_rq = gc_rq_ws->priv; > @@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work) > > up(&gc->gc_sem); > > - gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs)); > - if (!gc_rq->data) { > - pblk_err(pblk, "could not GC line:%d (%d/%d)\n", > - line->id, *line->vsc, gc_rq->nr_secs); > - goto out; > - } > - > /* Read from GC victim block */ > ret = pblk_submit_read_gc(pblk, gc_rq); > if (ret) { > @@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > struct pblk_line *line = line_ws->line; > struct pblk_line_mgmt *l_mg = &pblk->l_mg; > struct pblk_line_meta *lm = &pblk->lm; > + struct nvm_tgt_dev *dev = pblk->dev; > + struct nvm_geo *geo = &dev->geo; > struct pblk_gc *gc = &pblk->gc; > struct pblk_line_ws *gc_rq_ws; > struct pblk_gc_rq *gc_rq; > @@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > gc_rq->nr_secs = nr_secs; > gc_rq->line = line; > > + gc_rq->data = vmalloc(gc_rq->nr_secs * geo->csecs); > + if (!gc_rq->data) > + goto fail_free_gc_rq; > + > gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); > if (!gc_rq_ws) > - goto fail_free_gc_rq; > + goto fail_free_gc_data; > > gc_rq_ws->pblk = pblk; > gc_rq_ws->line = line; > @@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > > return; > > +fail_free_gc_data: > + vfree(gc_rq->data); > fail_free_gc_rq: > kfree(gc_rq); > fail_free_lba_list: > -- > 2.17.1 Looks good to me. Reviewed-by: Javier González <javier@javigon.com>
On Thu, Feb 28, 2019 at 6:09 PM Javier González <javier@javigon.com> wrote: > > > > > On 27 Feb 2019, at 12.14, Igor Konopko <igor.j.konopko@intel.com> wrote: > > > > Currently when we fail on gc rq data allocation > > we simply skip the data which we wanted to move > > and finally move the line to free state and lose > > that data due to that. This patch move the data > > allocation to some earlier phase of GC, where we > > can still fail gracefully by moving line back > > to closed state. > > > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > > --- > > drivers/lightnvm/pblk-gc.c | 19 +++++++++---------- > > 1 file changed, 9 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c > > index 3feadfd9418d..31fc1339faa8 100644 > > --- a/drivers/lightnvm/pblk-gc.c > > +++ b/drivers/lightnvm/pblk-gc.c > > @@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work) > > struct pblk_line_ws *gc_rq_ws = container_of(work, > > struct pblk_line_ws, ws); > > struct pblk *pblk = gc_rq_ws->pblk; > > - struct nvm_tgt_dev *dev = pblk->dev; > > - struct nvm_geo *geo = &dev->geo; > > struct pblk_gc *gc = &pblk->gc; > > struct pblk_line *line = gc_rq_ws->line; > > struct pblk_gc_rq *gc_rq = gc_rq_ws->priv; > > @@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work) > > > > up(&gc->gc_sem); > > > > - gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs)); > > - if (!gc_rq->data) { > > - pblk_err(pblk, "could not GC line:%d (%d/%d)\n", > > - line->id, *line->vsc, gc_rq->nr_secs); > > - goto out; > > - } > > - > > /* Read from GC victim block */ > > ret = pblk_submit_read_gc(pblk, gc_rq); > > if (ret) { > > @@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > > struct pblk_line *line = line_ws->line; > > struct pblk_line_mgmt *l_mg = &pblk->l_mg; > > struct pblk_line_meta *lm = &pblk->lm; > > + struct nvm_tgt_dev *dev = pblk->dev; > > + struct nvm_geo *geo = &dev->geo; > > struct pblk_gc *gc = &pblk->gc; > > struct pblk_line_ws *gc_rq_ws; > > struct pblk_gc_rq *gc_rq; > > @@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > > gc_rq->nr_secs = nr_secs; > > gc_rq->line = line; > > > > + gc_rq->data = vmalloc(gc_rq->nr_secs * geo->csecs); Why not use array_size to do the size calculation as before? It checks for overflows. Apart from this, the patch looks good to me. > > + if (!gc_rq->data) > > + goto fail_free_gc_rq; > > + > > gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); > > if (!gc_rq_ws) > > - goto fail_free_gc_rq; > > + goto fail_free_gc_data; > > > > gc_rq_ws->pblk = pblk; > > gc_rq_ws->line = line; > > @@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) > > > > return; > > > > +fail_free_gc_data: > > + vfree(gc_rq->data); > > fail_free_gc_rq: > > kfree(gc_rq); > > fail_free_lba_list: > > -- > > 2.17.1 > > Looks good to me. > > Reviewed-by: Javier González <javier@javigon.com>
On 01.03.2019 13:50, Hans Holmberg wrote: > On Thu, Feb 28, 2019 at 6:09 PM Javier González <javier@javigon.com> wrote: >> >> >> >>> On 27 Feb 2019, at 12.14, Igor Konopko <igor.j.konopko@intel.com> wrote: >>> >>> Currently when we fail on gc rq data allocation >>> we simply skip the data which we wanted to move >>> and finally move the line to free state and lose >>> that data due to that. This patch move the data >>> allocation to some earlier phase of GC, where we >>> can still fail gracefully by moving line back >>> to closed state. >>> >>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> >>> --- >>> drivers/lightnvm/pblk-gc.c | 19 +++++++++---------- >>> 1 file changed, 9 insertions(+), 10 deletions(-) >>> >>> diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c >>> index 3feadfd9418d..31fc1339faa8 100644 >>> --- a/drivers/lightnvm/pblk-gc.c >>> +++ b/drivers/lightnvm/pblk-gc.c >>> @@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work) >>> struct pblk_line_ws *gc_rq_ws = container_of(work, >>> struct pblk_line_ws, ws); >>> struct pblk *pblk = gc_rq_ws->pblk; >>> - struct nvm_tgt_dev *dev = pblk->dev; >>> - struct nvm_geo *geo = &dev->geo; >>> struct pblk_gc *gc = &pblk->gc; >>> struct pblk_line *line = gc_rq_ws->line; >>> struct pblk_gc_rq *gc_rq = gc_rq_ws->priv; >>> @@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work) >>> >>> up(&gc->gc_sem); >>> >>> - gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs)); >>> - if (!gc_rq->data) { >>> - pblk_err(pblk, "could not GC line:%d (%d/%d)\n", >>> - line->id, *line->vsc, gc_rq->nr_secs); >>> - goto out; >>> - } >>> - >>> /* Read from GC victim block */ >>> ret = pblk_submit_read_gc(pblk, gc_rq); >>> if (ret) { >>> @@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) >>> struct pblk_line *line = line_ws->line; >>> struct pblk_line_mgmt *l_mg = &pblk->l_mg; >>> struct pblk_line_meta *lm = &pblk->lm; >>> + struct nvm_tgt_dev *dev = pblk->dev; >>> + struct nvm_geo *geo = &dev->geo; >>> struct pblk_gc *gc = &pblk->gc; >>> struct pblk_line_ws *gc_rq_ws; >>> struct pblk_gc_rq *gc_rq; >>> @@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) >>> gc_rq->nr_secs = nr_secs; >>> gc_rq->line = line; >>> >>> + gc_rq->data = vmalloc(gc_rq->nr_secs * geo->csecs); > > Why not use array_size to do the size calculation as before? It checks > for overflows. > Apart from this, the patch looks good to me. Sure, you are right array_size should be used here in the same way as it was before. Will fix that in v2. > >>> + if (!gc_rq->data) >>> + goto fail_free_gc_rq; >>> + >>> gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); >>> if (!gc_rq_ws) >>> - goto fail_free_gc_rq; >>> + goto fail_free_gc_data; >>> >>> gc_rq_ws->pblk = pblk; >>> gc_rq_ws->line = line; >>> @@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) >>> >>> return; >>> >>> +fail_free_gc_data: >>> + vfree(gc_rq->data); >>> fail_free_gc_rq: >>> kfree(gc_rq); >>> fail_free_lba_list: >>> -- >>> 2.17.1 >> >> Looks good to me. >> >> Reviewed-by: Javier González <javier@javigon.com>
diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c index 3feadfd9418d..31fc1339faa8 100644 --- a/drivers/lightnvm/pblk-gc.c +++ b/drivers/lightnvm/pblk-gc.c @@ -84,8 +84,6 @@ static void pblk_gc_line_ws(struct work_struct *work) struct pblk_line_ws *gc_rq_ws = container_of(work, struct pblk_line_ws, ws); struct pblk *pblk = gc_rq_ws->pblk; - struct nvm_tgt_dev *dev = pblk->dev; - struct nvm_geo *geo = &dev->geo; struct pblk_gc *gc = &pblk->gc; struct pblk_line *line = gc_rq_ws->line; struct pblk_gc_rq *gc_rq = gc_rq_ws->priv; @@ -93,13 +91,6 @@ static void pblk_gc_line_ws(struct work_struct *work) up(&gc->gc_sem); - gc_rq->data = vmalloc(array_size(gc_rq->nr_secs, geo->csecs)); - if (!gc_rq->data) { - pblk_err(pblk, "could not GC line:%d (%d/%d)\n", - line->id, *line->vsc, gc_rq->nr_secs); - goto out; - } - /* Read from GC victim block */ ret = pblk_submit_read_gc(pblk, gc_rq); if (ret) { @@ -189,6 +180,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) struct pblk_line *line = line_ws->line; struct pblk_line_mgmt *l_mg = &pblk->l_mg; struct pblk_line_meta *lm = &pblk->lm; + struct nvm_tgt_dev *dev = pblk->dev; + struct nvm_geo *geo = &dev->geo; struct pblk_gc *gc = &pblk->gc; struct pblk_line_ws *gc_rq_ws; struct pblk_gc_rq *gc_rq; @@ -247,9 +240,13 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) gc_rq->nr_secs = nr_secs; gc_rq->line = line; + gc_rq->data = vmalloc(gc_rq->nr_secs * geo->csecs); + if (!gc_rq->data) + goto fail_free_gc_rq; + gc_rq_ws = kmalloc(sizeof(struct pblk_line_ws), GFP_KERNEL); if (!gc_rq_ws) - goto fail_free_gc_rq; + goto fail_free_gc_data; gc_rq_ws->pblk = pblk; gc_rq_ws->line = line; @@ -281,6 +278,8 @@ static void pblk_gc_line_prepare_ws(struct work_struct *work) return; +fail_free_gc_data: + vfree(gc_rq->data); fail_free_gc_rq: kfree(gc_rq); fail_free_lba_list:
Currently when we fail on gc rq data allocation we simply skip the data which we wanted to move and finally move the line to free state and lose that data due to that. This patch move the data allocation to some earlier phase of GC, where we can still fail gracefully by moving line back to closed state. Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> --- drivers/lightnvm/pblk-gc.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-)