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

[Igor Konopko <igor.j.konopko@intel.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>