Re: [PATCH 07/18] lightnvm: pblk: wait for inflight IOs in recovery

[Igor Konopko <igor.j.konopko@intel.com>]

On 17.03.2019 20:33, Matias Bjørling wrote:
> On 3/14/19 9:04 AM, Igor Konopko wrote:
>> This patch changes the behaviour of recovery padding in order to
>> support a case, when some IOs were already submitted to the drive and
>> some next one are not submitted due to error returned.
>>
>> Currently in case of errors we simply exit the pad function without
>> waiting for inflight IOs, which leads to panic on inflight IOs
>> completion.
>>
>> After the changes we always wait for all the inflight IOs before
>> exiting the function.
>>
>> Also, since NVMe has an internal timeout per IO, there is no need to
>> introduce additonal one here.
>>
>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>> ---
>>   drivers/lightnvm/pblk-recovery.c | 32 +++++++++++++-------------------
>>   1 file changed, 13 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/lightnvm/pblk-recovery.c 
>> b/drivers/lightnvm/pblk-recovery.c
>> index ba1691d..73d5ead 100644
>> --- a/drivers/lightnvm/pblk-recovery.c
>> +++ b/drivers/lightnvm/pblk-recovery.c
>> @@ -200,7 +200,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>       rq_ppas = pblk_calc_secs(pblk, left_ppas, 0, false);
>>       if (rq_ppas < pblk->min_write_pgs) {
>>           pblk_err(pblk, "corrupted pad line %d\n", line->id);
>> -        goto fail_free_pad;
>> +        goto fail_complete;
>>       }
>>       rq_len = rq_ppas * geo->csecs;
>> @@ -209,7 +209,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>                           PBLK_VMALLOC_META, GFP_KERNEL);
>>       if (IS_ERR(bio)) {
>>           ret = PTR_ERR(bio);
>> -        goto fail_free_pad;
>> +        goto fail_complete;
>>       }
>>       bio->bi_iter.bi_sector = 0; /* internal bio */
>> @@ -218,8 +218,11 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>       rqd = pblk_alloc_rqd(pblk, PBLK_WRITE_INT);
>>       ret = pblk_alloc_rqd_meta(pblk, rqd);
>> -    if (ret)
>> -        goto fail_free_rqd;
>> +    if (ret) {
>> +        pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
>> +        bio_put(bio);
>> +        goto fail_complete;
>> +    }
>>       rqd->bio = bio;
>>       rqd->opcode = NVM_OP_PWRITE;
>> @@ -266,7 +269,10 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>       if (ret) {
>>           pblk_err(pblk, "I/O submission failed: %d\n", ret);
>>           pblk_up_chunk(pblk, rqd->ppa_list[0]);
>> -        goto fail_free_rqd;
>> +        kref_put(&pad_rq->ref, pblk_recov_complete);
>> +        pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
>> +        bio_put(bio);
>> +        goto fail_complete;
>>       }
>>       left_line_ppas -= rq_ppas;
>> @@ -274,13 +280,9 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>       if (left_ppas && left_line_ppas)
>>           goto next_pad_rq;
>> +fail_complete:
>>       kref_put(&pad_rq->ref, pblk_recov_complete);
>> -
>> -    if (!wait_for_completion_io_timeout(&pad_rq->wait,
>> -                msecs_to_jiffies(PBLK_COMMAND_TIMEOUT_MS))) {
>> -        pblk_err(pblk, "pad write timed out\n");
>> -        ret = -ETIME;
>> -    }
>> +    wait_for_completion(&pad_rq->wait);
>>       if (!pblk_line_is_full(line))
>>           pblk_err(pblk, "corrupted padded line: %d\n", line->id);
>> @@ -289,14 +291,6 @@ static int pblk_recov_pad_line(struct pblk *pblk, 
>> struct pblk_line *line,
>>   free_rq:
>>       kfree(pad_rq);
>>       return ret;
>> -
>> -fail_free_rqd:
>> -    pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
>> -    bio_put(bio);
>> -fail_free_pad:
>> -    kfree(pad_rq);
>> -    vfree(data);
>> -    return ret;
>>   }
>>   static int pblk_pad_distance(struct pblk *pblk, struct pblk_line *line)
>>
> 
> Hi Igor,
> 
> Can you split this patch in two. One that removes the 
> wait_for_completion_io_timeout (and constant), and another that makes 
> sure it waits until all inflight IOs are completed?
> 

Sure, will split this into two patches for v2.
>