Re: [PATCH 02/13] lightnvm: pblk: Gracefully handle GC data malloc fail

From: Hans Holmberg <hans.ml.holmberg@owltronix.com>
To: "Javier González" <javier@javigon.com>
Cc: "Konopko, Igor J" <igor.j.konopko@intel.com>,
	"Matias Bjørling" <mb@lightnvm.io>,
	"Hans Holmberg" <hans.holmberg@cnexlabs.com>,
	linux-block@vger.kernel.org
Subject: Re: [PATCH 02/13] lightnvm: pblk: Gracefully handle GC data malloc fail
Date: Fri, 1 Mar 2019 13:50:57 +0100	[thread overview]
Message-ID: <CANr-nt3w4UmOeRVeXLdq+yLGgLqcJ7Xa_Mvba=xqm1QoaUKMEw@mail.gmail.com> (raw)
In-Reply-To: <7EEAD803-201C-477E-8ACA-7F152BC3C455@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>