| Message ID | 20190314160428.3559-8-igor.j.konopko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: next set of improvements for 5.2 | expand |
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?
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. >
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)
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(-)