On Thu, Feb 28, 2019 at 6:09 PM Javier González <javier@xxxxxxxxxxx> wrote: > > > > > On 27 Feb 2019, at 12.14, Igor Konopko <igor.j.konopko@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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@xxxxxxxxxxx>