| Message ID | 1517364411-22386-5-git-send-email-javier@cnexlabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/31/2018 03:06 AM, Javier González wrote: > In preparation for the OCSSD 2.0 spec. bad block identification, > refactor the current code to generalize bad block get/set functions and > structures. > > Signed-off-by: Javier González <javier@cnexlabs.com> > --- > drivers/lightnvm/pblk-init.c | 213 +++++++++++++++++++++++-------------------- > drivers/lightnvm/pblk.h | 6 -- > 2 files changed, 112 insertions(+), 107 deletions(-) > > diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c > index a5e3510c0f38..86a94a7faa96 100644 > --- a/drivers/lightnvm/pblk-init.c > +++ b/drivers/lightnvm/pblk-init.c > @@ -365,7 +365,25 @@ static void pblk_luns_free(struct pblk *pblk) > kfree(pblk->luns); > } > > -static void pblk_free_line_bitmaps(struct pblk_line *line) > +static void pblk_line_mg_free(struct pblk *pblk) > +{ > + struct pblk_line_mgmt *l_mg = &pblk->l_mg; > + int i; > + > + kfree(l_mg->bb_template); > + kfree(l_mg->bb_aux); > + kfree(l_mg->vsc_list); > + > + for (i = 0; i < PBLK_DATA_LINES; i++) { > + kfree(l_mg->sline_meta[i]); > + pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); > + kfree(l_mg->eline_meta[i]); > + } > + > + kfree(pblk->lines); > +} > + > +static void pblk_line_meta_free(struct pblk_line *line) > { > kfree(line->blk_bitmap); > kfree(line->erase_bitmap); > @@ -382,40 +400,16 @@ static void pblk_lines_free(struct pblk *pblk) > line = &pblk->lines[i]; > > pblk_line_free(pblk, line); > - pblk_free_line_bitmaps(line); > + pblk_line_meta_free(line); > } > spin_unlock(&l_mg->free_lock); > } > > -static void pblk_line_meta_free(struct pblk *pblk) > +static int pblk_bb_get_tbl(struct nvm_tgt_dev *dev, struct pblk_lun *rlun, > + u8 *blks, int nr_blks) > { > - struct pblk_line_mgmt *l_mg = &pblk->l_mg; > - int i; > - > - kfree(l_mg->bb_template); > - kfree(l_mg->bb_aux); > - kfree(l_mg->vsc_list); > - > - for (i = 0; i < PBLK_DATA_LINES; i++) { > - kfree(l_mg->sline_meta[i]); > - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); > - kfree(l_mg->eline_meta[i]); > - } > - > - kfree(pblk->lines); > -} > - > -static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) > -{ > - struct nvm_geo *geo = &dev->geo; > struct ppa_addr ppa; > - u8 *blks; > - int nr_blks, ret; > - > - nr_blks = geo->nr_chks * geo->plane_mode; > - blks = kmalloc(nr_blks, GFP_KERNEL); > - if (!blks) > - return -ENOMEM; > + int ret; > > ppa.ppa = 0; > ppa.g.ch = rlun->bppa.g.ch; > @@ -423,34 +417,56 @@ static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) > > ret = nvm_get_tgt_bb_tbl(dev, ppa, blks); > if (ret) > - goto out; > + return ret; > > nr_blks = nvm_bb_tbl_fold(dev->parent, blks, nr_blks); > - if (nr_blks < 0) { > - ret = nr_blks; > - goto out; > - } > - > - rlun->bb_list = blks; > + if (nr_blks < 0) > + return -EIO; > > return 0; > -out: > - kfree(blks); > - return ret; > +} > + > +static void *pblk_bb_get_log(struct pblk *pblk) > +{ > + struct nvm_tgt_dev *dev = pblk->dev; > + struct nvm_geo *geo = &dev->geo; > + u8 *log; > + int i, nr_blks, blk_per_lun; > + int ret; > + > + blk_per_lun = geo->nr_chks * geo->plane_mode; > + nr_blks = blk_per_lun * geo->all_luns; > + > + log = kmalloc(nr_blks, GFP_KERNEL); > + if (!log) > + return ERR_PTR(-ENOMEM); > + > + for (i = 0; i < geo->all_luns; i++) { > + struct pblk_lun *rlun = &pblk->luns[i]; > + u8 *log_pos = log + i * blk_per_lun; > + > + ret = pblk_bb_get_tbl(dev, rlun, log_pos, blk_per_lun); > + if (ret) { > + kfree(log); > + return ERR_PTR(-EIO); > + } > + } > + > + return log; > } > > static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, > - int blk_per_line) > + u8 *bb_log, int blk_per_line) > { > struct nvm_tgt_dev *dev = pblk->dev; > struct nvm_geo *geo = &dev->geo; > - struct pblk_lun *rlun; > - int bb_cnt = 0; > - int i; > + int i, bb_cnt = 0; > > for (i = 0; i < blk_per_line; i++) { > - rlun = &pblk->luns[i]; > - if (rlun->bb_list[line->id] == NVM_BLK_T_FREE) > + struct pblk_lun *rlun = &pblk->luns[i]; > + u8 *lun_bb_log = bb_log + i * blk_per_line; > + > + if (lun_bb_log[line->id] == NVM_BLK_T_FREE) > continue; > > set_bit(pblk_ppa_to_pos(geo, rlun->bppa), line->blk_bitmap); > @@ -460,29 +476,12 @@ static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, > return bb_cnt; > } > > -static int pblk_alloc_line_bitmaps(struct pblk *pblk, struct pblk_line *line) > -{ > - struct pblk_line_meta *lm = &pblk->lm; > - > - line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); > - if (!line->blk_bitmap) > - return -ENOMEM; > - > - line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); > - if (!line->erase_bitmap) { > - kfree(line->blk_bitmap); > - return -ENOMEM; > - } > - > - return 0; > -} > - > static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) > { > struct nvm_tgt_dev *dev = pblk->dev; > struct nvm_geo *geo = &dev->geo; > struct pblk_lun *rlun; > - int i, ret; > + int i; > > /* TODO: Implement unbalanced LUN support */ > if (geo->nr_luns < 0) { > @@ -505,13 +504,6 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) > rlun->bppa = luns[lunid]; > > sema_init(&rlun->wr_sem, 1); > - > - ret = pblk_bb_discovery(dev, rlun); > - if (ret) { > - while (--i >= 0) > - kfree(pblk->luns[i].bb_list); > - return ret; > - } > } > > return 0; > @@ -689,6 +681,26 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk) > return -ENOMEM; > } > > +static int pblk_setup_line_meta(struct pblk *pblk, struct pblk_line *line, > + void *chunk_log, long *nr_bad_blks) > +{ > + struct pblk_line_meta *lm = &pblk->lm; > + > + line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); > + if (!line->blk_bitmap) > + return -ENOMEM; > + > + line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); > + if (!line->erase_bitmap) { > + kfree(line->blk_bitmap); > + return -ENOMEM; > + } > + > + *nr_bad_blks = pblk_bb_line(pblk, line, chunk_log, lm->blk_per_line); > + > + return 0; > +} > + > static int pblk_lines_init(struct pblk *pblk) > { > struct nvm_tgt_dev *dev = pblk->dev; > @@ -696,8 +708,9 @@ static int pblk_lines_init(struct pblk *pblk) > struct pblk_line_mgmt *l_mg = &pblk->l_mg; > struct pblk_line_meta *lm = &pblk->lm; > struct pblk_line *line; > + void *chunk_log; > unsigned int smeta_len, emeta_len; > - long nr_bad_blks, nr_free_blks; > + long nr_bad_blks = 0, nr_free_blks = 0; > int bb_distance, max_write_ppas, mod; > int i, ret; > > @@ -771,13 +784,12 @@ static int pblk_lines_init(struct pblk *pblk) > if (lm->min_blk_line > lm->blk_per_line) { > pr_err("pblk: config. not supported. Min. LUN in line:%d\n", > lm->blk_per_line); > - ret = -EINVAL; > - goto fail; > + return -EINVAL; > } > > ret = pblk_lines_alloc_metadata(pblk); > if (ret) > - goto fail; > + return ret; > > l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL); > if (!l_mg->bb_template) { > @@ -821,9 +833,16 @@ static int pblk_lines_init(struct pblk *pblk) > goto fail_free_bb_aux; > } > > - nr_free_blks = 0; > + chunk_log = pblk_bb_get_log(pblk); > + if (IS_ERR(chunk_log)) { > + pr_err("pblk: could not get bad block log (%lu)\n", > + PTR_ERR(chunk_log)); > + ret = PTR_ERR(chunk_log); > + goto fail_free_lines; > + } > + > for (i = 0; i < l_mg->nr_lines; i++) { > - int blk_in_line; > + int chk_in_line; > > line = &pblk->lines[i]; > > @@ -835,26 +854,20 @@ static int pblk_lines_init(struct pblk *pblk) > line->vsc = &l_mg->vsc_list[i]; > spin_lock_init(&line->lock); > > - ret = pblk_alloc_line_bitmaps(pblk, line); > + ret = pblk_setup_line_meta(pblk, line, chunk_log, &nr_bad_blks); > if (ret) > - goto fail_free_lines; > + goto fail_free_chunk_log; > > - nr_bad_blks = pblk_bb_line(pblk, line, lm->blk_per_line); > - if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) { > - pblk_free_line_bitmaps(line); > - ret = -EINVAL; > - goto fail_free_lines; > - } > - > - blk_in_line = lm->blk_per_line - nr_bad_blks; > - if (blk_in_line < lm->min_blk_line) { > + chk_in_line = lm->blk_per_line - nr_bad_blks; > + if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line || > + chk_in_line < lm->min_blk_line) { > line->state = PBLK_LINESTATE_BAD; > list_add_tail(&line->list, &l_mg->bad_list); > continue; > } > > - nr_free_blks += blk_in_line; > - atomic_set(&line->blk_in_line, blk_in_line); > + nr_free_blks += chk_in_line; > + atomic_set(&line->blk_in_line, chk_in_line); > > l_mg->nr_free_lines++; > list_add_tail(&line->list, &l_mg->free_list); > @@ -862,23 +875,21 @@ static int pblk_lines_init(struct pblk *pblk) > > pblk_set_provision(pblk, nr_free_blks); > > - /* Cleanup per-LUN bad block lists - managed within lines on run-time */ > - for (i = 0; i < geo->all_luns; i++) > - kfree(pblk->luns[i].bb_list); > - > + kfree(chunk_log); > return 0; > -fail_free_lines: > + > +fail_free_chunk_log: > + kfree(chunk_log); > while (--i >= 0) > - pblk_free_line_bitmaps(&pblk->lines[i]); > + pblk_line_meta_free(&pblk->lines[i]); > +fail_free_lines: > + kfree(pblk->lines); > fail_free_bb_aux: > kfree(l_mg->bb_aux); > fail_free_bb_template: > kfree(l_mg->bb_template); > fail_free_meta: > - pblk_line_meta_free(pblk); > -fail: > - for (i = 0; i < geo->all_luns; i++) > - kfree(pblk->luns[i].bb_list); > + pblk_line_mg_free(pblk); > > return ret; > } > @@ -922,7 +933,7 @@ static void pblk_free(struct pblk *pblk) > pblk_luns_free(pblk); > pblk_lines_free(pblk); > kfree(pblk->pad_dist); > - pblk_line_meta_free(pblk); > + pblk_line_mg_free(pblk); > pblk_core_free(pblk); > pblk_l2p_free(pblk); > > @@ -1110,7 +1121,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk, > fail_free_pad_dist: > kfree(pblk->pad_dist); > fail_free_line_meta: > - pblk_line_meta_free(pblk); > + pblk_line_mg_free(pblk); > fail_free_luns: > pblk_luns_free(pblk); > fail: > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h > index 88720e2441c0..282dfc8780e8 100644 > --- a/drivers/lightnvm/pblk.h > +++ b/drivers/lightnvm/pblk.h > @@ -201,12 +201,6 @@ struct pblk_rb { > > struct pblk_lun { > struct ppa_addr bppa; > - > - u8 *bb_list; /* Bad block list for LUN. Only used on > - * bring up. Bad blocks are managed > - * within lines on run-time. > - */ > - > struct semaphore wr_sem; > }; > > Looks good to me. With respect to the 2.0 implementation. I am thinking that get_bb and set_bb should behave in the following way: For get_bb (in nvme/host/lightnvm.c) 1.2: Keep the implementation as is. 2.0: Use the report chunk command, and redo the get_bb format. For set_bb (in nvme/host/lightnvm.c) 1.2: Business as usual 2.0: Report error. For 2.0, there will be added a function pointer for get report chunk implementation, where the client can ask for full chunk information. Similarly here, client will fail the function call if the drive is 1.2. I hope to post the small 2.0 identify implementation Tomorrow or Friday, and then you can post the report chunk implementation that you have done if you like. We also need to take a look at the new error codes, which does not align with the 1.2 specification (now that they actually follow the nvme specification ;))
> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@lightnvm.io> wrote: > >> On 01/31/2018 03:06 AM, Javier González wrote: >> In preparation for the OCSSD 2.0 spec. bad block identification, >> refactor the current code to generalize bad block get/set functions and >> structures. >> Signed-off-by: Javier González <javier@cnexlabs.com> >> --- >> drivers/lightnvm/pblk-init.c | 213 +++++++++++++++++++++++-------------------- >> drivers/lightnvm/pblk.h | 6 -- >> 2 files changed, 112 insertions(+), 107 deletions(-) >> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >> index a5e3510c0f38..86a94a7faa96 100644 >> --- a/drivers/lightnvm/pblk-init.c >> +++ b/drivers/lightnvm/pblk-init.c >> @@ -365,7 +365,25 @@ static void pblk_luns_free(struct pblk *pblk) >> kfree(pblk->luns); >> } >> -static void pblk_free_line_bitmaps(struct pblk_line *line) >> +static void pblk_line_mg_free(struct pblk *pblk) >> +{ >> + struct pblk_line_mgmt *l_mg = &pblk->l_mg; >> + int i; >> + >> + kfree(l_mg->bb_template); >> + kfree(l_mg->bb_aux); >> + kfree(l_mg->vsc_list); >> + >> + for (i = 0; i < PBLK_DATA_LINES; i++) { >> + kfree(l_mg->sline_meta[i]); >> + pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); >> + kfree(l_mg->eline_meta[i]); >> + } >> + >> + kfree(pblk->lines); >> +} >> + >> +static void pblk_line_meta_free(struct pblk_line *line) >> { >> kfree(line->blk_bitmap); >> kfree(line->erase_bitmap); >> @@ -382,40 +400,16 @@ static void pblk_lines_free(struct pblk *pblk) >> line = &pblk->lines[i]; >> pblk_line_free(pblk, line); >> - pblk_free_line_bitmaps(line); >> + pblk_line_meta_free(line); >> } >> spin_unlock(&l_mg->free_lock); >> } >> -static void pblk_line_meta_free(struct pblk *pblk) >> +static int pblk_bb_get_tbl(struct nvm_tgt_dev *dev, struct pblk_lun *rlun, >> + u8 *blks, int nr_blks) >> { >> - struct pblk_line_mgmt *l_mg = &pblk->l_mg; >> - int i; >> - >> - kfree(l_mg->bb_template); >> - kfree(l_mg->bb_aux); >> - kfree(l_mg->vsc_list); >> - >> - for (i = 0; i < PBLK_DATA_LINES; i++) { >> - kfree(l_mg->sline_meta[i]); >> - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); >> - kfree(l_mg->eline_meta[i]); >> - } >> - >> - kfree(pblk->lines); >> -} >> - >> -static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) >> -{ >> - struct nvm_geo *geo = &dev->geo; >> struct ppa_addr ppa; >> - u8 *blks; >> - int nr_blks, ret; >> - >> - nr_blks = geo->nr_chks * geo->plane_mode; >> - blks = kmalloc(nr_blks, GFP_KERNEL); >> - if (!blks) >> - return -ENOMEM; >> + int ret; >> ppa.ppa = 0; >> ppa.g.ch = rlun->bppa.g.ch; >> @@ -423,34 +417,56 @@ static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) >> ret = nvm_get_tgt_bb_tbl(dev, ppa, blks); >> if (ret) >> - goto out; >> + return ret; >> nr_blks = nvm_bb_tbl_fold(dev->parent, blks, nr_blks); >> - if (nr_blks < 0) { >> - ret = nr_blks; >> - goto out; >> - } >> - >> - rlun->bb_list = blks; >> + if (nr_blks < 0) >> + return -EIO; >> return 0; >> -out: >> - kfree(blks); >> - return ret; >> +} >> + >> +static void *pblk_bb_get_log(struct pblk *pblk) >> +{ >> + struct nvm_tgt_dev *dev = pblk->dev; >> + struct nvm_geo *geo = &dev->geo; >> + u8 *log; >> + int i, nr_blks, blk_per_lun; >> + int ret; >> + >> + blk_per_lun = geo->nr_chks * geo->plane_mode; >> + nr_blks = blk_per_lun * geo->all_luns; >> + >> + log = kmalloc(nr_blks, GFP_KERNEL); >> + if (!log) >> + return ERR_PTR(-ENOMEM); >> + >> + for (i = 0; i < geo->all_luns; i++) { >> + struct pblk_lun *rlun = &pblk->luns[i]; >> + u8 *log_pos = log + i * blk_per_lun; >> + >> + ret = pblk_bb_get_tbl(dev, rlun, log_pos, blk_per_lun); >> + if (ret) { >> + kfree(log); >> + return ERR_PTR(-EIO); >> + } >> + } >> + >> + return log; >> } >> static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, >> - int blk_per_line) >> + u8 *bb_log, int blk_per_line) >> { >> struct nvm_tgt_dev *dev = pblk->dev; >> struct nvm_geo *geo = &dev->geo; >> - struct pblk_lun *rlun; >> - int bb_cnt = 0; >> - int i; >> + int i, bb_cnt = 0; >> for (i = 0; i < blk_per_line; i++) { >> - rlun = &pblk->luns[i]; >> - if (rlun->bb_list[line->id] == NVM_BLK_T_FREE) >> + struct pblk_lun *rlun = &pblk->luns[i]; >> + u8 *lun_bb_log = bb_log + i * blk_per_line; >> + >> + if (lun_bb_log[line->id] == NVM_BLK_T_FREE) >> continue; >> set_bit(pblk_ppa_to_pos(geo, rlun->bppa), line->blk_bitmap); >> @@ -460,29 +476,12 @@ static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, >> return bb_cnt; >> } >> -static int pblk_alloc_line_bitmaps(struct pblk *pblk, struct pblk_line *line) >> -{ >> - struct pblk_line_meta *lm = &pblk->lm; >> - >> - line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >> - if (!line->blk_bitmap) >> - return -ENOMEM; >> - >> - line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >> - if (!line->erase_bitmap) { >> - kfree(line->blk_bitmap); >> - return -ENOMEM; >> - } >> - >> - return 0; >> -} >> - >> static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) >> { >> struct nvm_tgt_dev *dev = pblk->dev; >> struct nvm_geo *geo = &dev->geo; >> struct pblk_lun *rlun; >> - int i, ret; >> + int i; >> /* TODO: Implement unbalanced LUN support */ >> if (geo->nr_luns < 0) { >> @@ -505,13 +504,6 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) >> rlun->bppa = luns[lunid]; >> sema_init(&rlun->wr_sem, 1); >> - >> - ret = pblk_bb_discovery(dev, rlun); >> - if (ret) { >> - while (--i >= 0) >> - kfree(pblk->luns[i].bb_list); >> - return ret; >> - } >> } >> return 0; >> @@ -689,6 +681,26 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk) >> return -ENOMEM; >> } >> +static int pblk_setup_line_meta(struct pblk *pblk, struct pblk_line *line, >> + void *chunk_log, long *nr_bad_blks) >> +{ >> + struct pblk_line_meta *lm = &pblk->lm; >> + >> + line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >> + if (!line->blk_bitmap) >> + return -ENOMEM; >> + >> + line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >> + if (!line->erase_bitmap) { >> + kfree(line->blk_bitmap); >> + return -ENOMEM; >> + } >> + >> + *nr_bad_blks = pblk_bb_line(pblk, line, chunk_log, lm->blk_per_line); >> + >> + return 0; >> +} >> + >> static int pblk_lines_init(struct pblk *pblk) >> { >> struct nvm_tgt_dev *dev = pblk->dev; >> @@ -696,8 +708,9 @@ static int pblk_lines_init(struct pblk *pblk) >> struct pblk_line_mgmt *l_mg = &pblk->l_mg; >> struct pblk_line_meta *lm = &pblk->lm; >> struct pblk_line *line; >> + void *chunk_log; >> unsigned int smeta_len, emeta_len; >> - long nr_bad_blks, nr_free_blks; >> + long nr_bad_blks = 0, nr_free_blks = 0; >> int bb_distance, max_write_ppas, mod; >> int i, ret; >> @@ -771,13 +784,12 @@ static int pblk_lines_init(struct pblk *pblk) >> if (lm->min_blk_line > lm->blk_per_line) { >> pr_err("pblk: config. not supported. Min. LUN in line:%d\n", >> lm->blk_per_line); >> - ret = -EINVAL; >> - goto fail; >> + return -EINVAL; >> } >> ret = pblk_lines_alloc_metadata(pblk); >> if (ret) >> - goto fail; >> + return ret; >> l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL); >> if (!l_mg->bb_template) { >> @@ -821,9 +833,16 @@ static int pblk_lines_init(struct pblk *pblk) >> goto fail_free_bb_aux; >> } >> - nr_free_blks = 0; >> + chunk_log = pblk_bb_get_log(pblk); >> + if (IS_ERR(chunk_log)) { >> + pr_err("pblk: could not get bad block log (%lu)\n", >> + PTR_ERR(chunk_log)); >> + ret = PTR_ERR(chunk_log); >> + goto fail_free_lines; >> + } >> + >> for (i = 0; i < l_mg->nr_lines; i++) { >> - int blk_in_line; >> + int chk_in_line; >> line = &pblk->lines[i]; >> @@ -835,26 +854,20 @@ static int pblk_lines_init(struct pblk *pblk) >> line->vsc = &l_mg->vsc_list[i]; >> spin_lock_init(&line->lock); >> - ret = pblk_alloc_line_bitmaps(pblk, line); >> + ret = pblk_setup_line_meta(pblk, line, chunk_log, &nr_bad_blks); >> if (ret) >> - goto fail_free_lines; >> + goto fail_free_chunk_log; >> - nr_bad_blks = pblk_bb_line(pblk, line, lm->blk_per_line); >> - if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) { >> - pblk_free_line_bitmaps(line); >> - ret = -EINVAL; >> - goto fail_free_lines; >> - } >> - >> - blk_in_line = lm->blk_per_line - nr_bad_blks; >> - if (blk_in_line < lm->min_blk_line) { >> + chk_in_line = lm->blk_per_line - nr_bad_blks; >> + if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line || >> + chk_in_line < lm->min_blk_line) { >> line->state = PBLK_LINESTATE_BAD; >> list_add_tail(&line->list, &l_mg->bad_list); >> continue; >> } >> - nr_free_blks += blk_in_line; >> - atomic_set(&line->blk_in_line, blk_in_line); >> + nr_free_blks += chk_in_line; >> + atomic_set(&line->blk_in_line, chk_in_line); >> l_mg->nr_free_lines++; >> list_add_tail(&line->list, &l_mg->free_list); >> @@ -862,23 +875,21 @@ static int pblk_lines_init(struct pblk *pblk) >> pblk_set_provision(pblk, nr_free_blks); >> - /* Cleanup per-LUN bad block lists - managed within lines on run-time */ >> - for (i = 0; i < geo->all_luns; i++) >> - kfree(pblk->luns[i].bb_list); >> - >> + kfree(chunk_log); >> return 0; >> -fail_free_lines: >> + >> +fail_free_chunk_log: >> + kfree(chunk_log); >> while (--i >= 0) >> - pblk_free_line_bitmaps(&pblk->lines[i]); >> + pblk_line_meta_free(&pblk->lines[i]); >> +fail_free_lines: >> + kfree(pblk->lines); >> fail_free_bb_aux: >> kfree(l_mg->bb_aux); >> fail_free_bb_template: >> kfree(l_mg->bb_template); >> fail_free_meta: >> - pblk_line_meta_free(pblk); >> -fail: >> - for (i = 0; i < geo->all_luns; i++) >> - kfree(pblk->luns[i].bb_list); >> + pblk_line_mg_free(pblk); >> return ret; >> } >> @@ -922,7 +933,7 @@ static void pblk_free(struct pblk *pblk) >> pblk_luns_free(pblk); >> pblk_lines_free(pblk); >> kfree(pblk->pad_dist); >> - pblk_line_meta_free(pblk); >> + pblk_line_mg_free(pblk); >> pblk_core_free(pblk); >> pblk_l2p_free(pblk); >> @@ -1110,7 +1121,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk, >> fail_free_pad_dist: >> kfree(pblk->pad_dist); >> fail_free_line_meta: >> - pblk_line_meta_free(pblk); >> + pblk_line_mg_free(pblk); >> fail_free_luns: >> pblk_luns_free(pblk); >> fail: >> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h >> index 88720e2441c0..282dfc8780e8 100644 >> --- a/drivers/lightnvm/pblk.h >> +++ b/drivers/lightnvm/pblk.h >> @@ -201,12 +201,6 @@ struct pblk_rb { >> struct pblk_lun { >> struct ppa_addr bppa; >> - >> - u8 *bb_list; /* Bad block list for LUN. Only used on >> - * bring up. Bad blocks are managed >> - * within lines on run-time. >> - */ >> - >> struct semaphore wr_sem; >> }; >> > > Looks good to me. > > With respect to the 2.0 implementation. I am thinking that get_bb and set_bb should behave in the following way: > > For get_bb (in nvme/host/lightnvm.c) > 1.2: Keep the implementation as is. > 2.0: Use the report chunk command, and redo the get_bb format. > > For set_bb (in nvme/host/lightnvm.c) > 1.2: Business as usual > 2.0: Report error. I have a patches abstracting this, which I think it makes it cleaner. I can push it next week for review. I’m traveling this week. (If you want to get a glimpse I can point you to the code). > > For 2.0, there will be added a function pointer for get report chunk implementation, where the client can ask for full chunk information. Similarly here, client will fail the function call if the drive is 1.2. > > I hope to post the small 2.0 identify implementation Tomorrow or Friday, and then you can post the report chunk implementation that you have done if you like Sure. I have patches implementing 2.0 too, but you can push your version. One thing I’d like to get in is a generic geometry structure that simplifies the identify command and puts the logic in the identify path. This makes the base group structure from 1.2 go away makes the 1.2 and 2.0 paths easier to maintain at the target level. I can point you to the patches tomorrow if you want to align with it. Otherwise, I can just rebase and send them next week. But please, consider this before making a new abstraction to make the 2 specs coexist - most of the work is done already... > We also need to take a look at the new error codes, which does not align with the 1.2 specification (now that they actually follow the nvme specification ;)) Cool. Let’s maintain the 1.2 errors for compatibility and make the 2.0 path clean - that is nvme compatible ;) Javier.
On 01/31/2018 10:13 AM, Javier Gonzalez wrote: > > >> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@lightnvm.io> wrote: >> >>> On 01/31/2018 03:06 AM, Javier González wrote: >>> In preparation for the OCSSD 2.0 spec. bad block identification, >>> refactor the current code to generalize bad block get/set functions and >>> structures. >>> Signed-off-by: Javier González <javier@cnexlabs.com> >>> --- >>> drivers/lightnvm/pblk-init.c | 213 +++++++++++++++++++++++-------------------- >>> drivers/lightnvm/pblk.h | 6 -- >>> 2 files changed, 112 insertions(+), 107 deletions(-) >>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c >>> index a5e3510c0f38..86a94a7faa96 100644 >>> --- a/drivers/lightnvm/pblk-init.c >>> +++ b/drivers/lightnvm/pblk-init.c >>> @@ -365,7 +365,25 @@ static void pblk_luns_free(struct pblk *pblk) >>> kfree(pblk->luns); >>> } >>> -static void pblk_free_line_bitmaps(struct pblk_line *line) >>> +static void pblk_line_mg_free(struct pblk *pblk) >>> +{ >>> + struct pblk_line_mgmt *l_mg = &pblk->l_mg; >>> + int i; >>> + >>> + kfree(l_mg->bb_template); >>> + kfree(l_mg->bb_aux); >>> + kfree(l_mg->vsc_list); >>> + >>> + for (i = 0; i < PBLK_DATA_LINES; i++) { >>> + kfree(l_mg->sline_meta[i]); >>> + pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); >>> + kfree(l_mg->eline_meta[i]); >>> + } >>> + >>> + kfree(pblk->lines); >>> +} >>> + >>> +static void pblk_line_meta_free(struct pblk_line *line) >>> { >>> kfree(line->blk_bitmap); >>> kfree(line->erase_bitmap); >>> @@ -382,40 +400,16 @@ static void pblk_lines_free(struct pblk *pblk) >>> line = &pblk->lines[i]; >>> pblk_line_free(pblk, line); >>> - pblk_free_line_bitmaps(line); >>> + pblk_line_meta_free(line); >>> } >>> spin_unlock(&l_mg->free_lock); >>> } >>> -static void pblk_line_meta_free(struct pblk *pblk) >>> +static int pblk_bb_get_tbl(struct nvm_tgt_dev *dev, struct pblk_lun *rlun, >>> + u8 *blks, int nr_blks) >>> { >>> - struct pblk_line_mgmt *l_mg = &pblk->l_mg; >>> - int i; >>> - >>> - kfree(l_mg->bb_template); >>> - kfree(l_mg->bb_aux); >>> - kfree(l_mg->vsc_list); >>> - >>> - for (i = 0; i < PBLK_DATA_LINES; i++) { >>> - kfree(l_mg->sline_meta[i]); >>> - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); >>> - kfree(l_mg->eline_meta[i]); >>> - } >>> - >>> - kfree(pblk->lines); >>> -} >>> - >>> -static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) >>> -{ >>> - struct nvm_geo *geo = &dev->geo; >>> struct ppa_addr ppa; >>> - u8 *blks; >>> - int nr_blks, ret; >>> - >>> - nr_blks = geo->nr_chks * geo->plane_mode; >>> - blks = kmalloc(nr_blks, GFP_KERNEL); >>> - if (!blks) >>> - return -ENOMEM; >>> + int ret; >>> ppa.ppa = 0; >>> ppa.g.ch = rlun->bppa.g.ch; >>> @@ -423,34 +417,56 @@ static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) >>> ret = nvm_get_tgt_bb_tbl(dev, ppa, blks); >>> if (ret) >>> - goto out; >>> + return ret; >>> nr_blks = nvm_bb_tbl_fold(dev->parent, blks, nr_blks); >>> - if (nr_blks < 0) { >>> - ret = nr_blks; >>> - goto out; >>> - } >>> - >>> - rlun->bb_list = blks; >>> + if (nr_blks < 0) >>> + return -EIO; >>> return 0; >>> -out: >>> - kfree(blks); >>> - return ret; >>> +} >>> + >>> +static void *pblk_bb_get_log(struct pblk *pblk) >>> +{ >>> + struct nvm_tgt_dev *dev = pblk->dev; >>> + struct nvm_geo *geo = &dev->geo; >>> + u8 *log; >>> + int i, nr_blks, blk_per_lun; >>> + int ret; >>> + >>> + blk_per_lun = geo->nr_chks * geo->plane_mode; >>> + nr_blks = blk_per_lun * geo->all_luns; >>> + >>> + log = kmalloc(nr_blks, GFP_KERNEL); >>> + if (!log) >>> + return ERR_PTR(-ENOMEM); >>> + >>> + for (i = 0; i < geo->all_luns; i++) { >>> + struct pblk_lun *rlun = &pblk->luns[i]; >>> + u8 *log_pos = log + i * blk_per_lun; >>> + >>> + ret = pblk_bb_get_tbl(dev, rlun, log_pos, blk_per_lun); >>> + if (ret) { >>> + kfree(log); >>> + return ERR_PTR(-EIO); >>> + } >>> + } >>> + >>> + return log; >>> } >>> static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, >>> - int blk_per_line) >>> + u8 *bb_log, int blk_per_line) >>> { >>> struct nvm_tgt_dev *dev = pblk->dev; >>> struct nvm_geo *geo = &dev->geo; >>> - struct pblk_lun *rlun; >>> - int bb_cnt = 0; >>> - int i; >>> + int i, bb_cnt = 0; >>> for (i = 0; i < blk_per_line; i++) { >>> - rlun = &pblk->luns[i]; >>> - if (rlun->bb_list[line->id] == NVM_BLK_T_FREE) >>> + struct pblk_lun *rlun = &pblk->luns[i]; >>> + u8 *lun_bb_log = bb_log + i * blk_per_line; >>> + >>> + if (lun_bb_log[line->id] == NVM_BLK_T_FREE) >>> continue; >>> set_bit(pblk_ppa_to_pos(geo, rlun->bppa), line->blk_bitmap); >>> @@ -460,29 +476,12 @@ static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, >>> return bb_cnt; >>> } >>> -static int pblk_alloc_line_bitmaps(struct pblk *pblk, struct pblk_line *line) >>> -{ >>> - struct pblk_line_meta *lm = &pblk->lm; >>> - >>> - line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >>> - if (!line->blk_bitmap) >>> - return -ENOMEM; >>> - >>> - line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >>> - if (!line->erase_bitmap) { >>> - kfree(line->blk_bitmap); >>> - return -ENOMEM; >>> - } >>> - >>> - return 0; >>> -} >>> - >>> static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) >>> { >>> struct nvm_tgt_dev *dev = pblk->dev; >>> struct nvm_geo *geo = &dev->geo; >>> struct pblk_lun *rlun; >>> - int i, ret; >>> + int i; >>> /* TODO: Implement unbalanced LUN support */ >>> if (geo->nr_luns < 0) { >>> @@ -505,13 +504,6 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) >>> rlun->bppa = luns[lunid]; >>> sema_init(&rlun->wr_sem, 1); >>> - >>> - ret = pblk_bb_discovery(dev, rlun); >>> - if (ret) { >>> - while (--i >= 0) >>> - kfree(pblk->luns[i].bb_list); >>> - return ret; >>> - } >>> } >>> return 0; >>> @@ -689,6 +681,26 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk) >>> return -ENOMEM; >>> } >>> +static int pblk_setup_line_meta(struct pblk *pblk, struct pblk_line *line, >>> + void *chunk_log, long *nr_bad_blks) >>> +{ >>> + struct pblk_line_meta *lm = &pblk->lm; >>> + >>> + line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >>> + if (!line->blk_bitmap) >>> + return -ENOMEM; >>> + >>> + line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); >>> + if (!line->erase_bitmap) { >>> + kfree(line->blk_bitmap); >>> + return -ENOMEM; >>> + } >>> + >>> + *nr_bad_blks = pblk_bb_line(pblk, line, chunk_log, lm->blk_per_line); >>> + >>> + return 0; >>> +} >>> + >>> static int pblk_lines_init(struct pblk *pblk) >>> { >>> struct nvm_tgt_dev *dev = pblk->dev; >>> @@ -696,8 +708,9 @@ static int pblk_lines_init(struct pblk *pblk) >>> struct pblk_line_mgmt *l_mg = &pblk->l_mg; >>> struct pblk_line_meta *lm = &pblk->lm; >>> struct pblk_line *line; >>> + void *chunk_log; >>> unsigned int smeta_len, emeta_len; >>> - long nr_bad_blks, nr_free_blks; >>> + long nr_bad_blks = 0, nr_free_blks = 0; >>> int bb_distance, max_write_ppas, mod; >>> int i, ret; >>> @@ -771,13 +784,12 @@ static int pblk_lines_init(struct pblk *pblk) >>> if (lm->min_blk_line > lm->blk_per_line) { >>> pr_err("pblk: config. not supported. Min. LUN in line:%d\n", >>> lm->blk_per_line); >>> - ret = -EINVAL; >>> - goto fail; >>> + return -EINVAL; >>> } >>> ret = pblk_lines_alloc_metadata(pblk); >>> if (ret) >>> - goto fail; >>> + return ret; >>> l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL); >>> if (!l_mg->bb_template) { >>> @@ -821,9 +833,16 @@ static int pblk_lines_init(struct pblk *pblk) >>> goto fail_free_bb_aux; >>> } >>> - nr_free_blks = 0; >>> + chunk_log = pblk_bb_get_log(pblk); >>> + if (IS_ERR(chunk_log)) { >>> + pr_err("pblk: could not get bad block log (%lu)\n", >>> + PTR_ERR(chunk_log)); >>> + ret = PTR_ERR(chunk_log); >>> + goto fail_free_lines; >>> + } >>> + >>> for (i = 0; i < l_mg->nr_lines; i++) { >>> - int blk_in_line; >>> + int chk_in_line; >>> line = &pblk->lines[i]; >>> @@ -835,26 +854,20 @@ static int pblk_lines_init(struct pblk *pblk) >>> line->vsc = &l_mg->vsc_list[i]; >>> spin_lock_init(&line->lock); >>> - ret = pblk_alloc_line_bitmaps(pblk, line); >>> + ret = pblk_setup_line_meta(pblk, line, chunk_log, &nr_bad_blks); >>> if (ret) >>> - goto fail_free_lines; >>> + goto fail_free_chunk_log; >>> - nr_bad_blks = pblk_bb_line(pblk, line, lm->blk_per_line); >>> - if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) { >>> - pblk_free_line_bitmaps(line); >>> - ret = -EINVAL; >>> - goto fail_free_lines; >>> - } >>> - >>> - blk_in_line = lm->blk_per_line - nr_bad_blks; >>> - if (blk_in_line < lm->min_blk_line) { >>> + chk_in_line = lm->blk_per_line - nr_bad_blks; >>> + if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line || >>> + chk_in_line < lm->min_blk_line) { >>> line->state = PBLK_LINESTATE_BAD; >>> list_add_tail(&line->list, &l_mg->bad_list); >>> continue; >>> } >>> - nr_free_blks += blk_in_line; >>> - atomic_set(&line->blk_in_line, blk_in_line); >>> + nr_free_blks += chk_in_line; >>> + atomic_set(&line->blk_in_line, chk_in_line); >>> l_mg->nr_free_lines++; >>> list_add_tail(&line->list, &l_mg->free_list); >>> @@ -862,23 +875,21 @@ static int pblk_lines_init(struct pblk *pblk) >>> pblk_set_provision(pblk, nr_free_blks); >>> - /* Cleanup per-LUN bad block lists - managed within lines on run-time */ >>> - for (i = 0; i < geo->all_luns; i++) >>> - kfree(pblk->luns[i].bb_list); >>> - >>> + kfree(chunk_log); >>> return 0; >>> -fail_free_lines: >>> + >>> +fail_free_chunk_log: >>> + kfree(chunk_log); >>> while (--i >= 0) >>> - pblk_free_line_bitmaps(&pblk->lines[i]); >>> + pblk_line_meta_free(&pblk->lines[i]); >>> +fail_free_lines: >>> + kfree(pblk->lines); >>> fail_free_bb_aux: >>> kfree(l_mg->bb_aux); >>> fail_free_bb_template: >>> kfree(l_mg->bb_template); >>> fail_free_meta: >>> - pblk_line_meta_free(pblk); >>> -fail: >>> - for (i = 0; i < geo->all_luns; i++) >>> - kfree(pblk->luns[i].bb_list); >>> + pblk_line_mg_free(pblk); >>> return ret; >>> } >>> @@ -922,7 +933,7 @@ static void pblk_free(struct pblk *pblk) >>> pblk_luns_free(pblk); >>> pblk_lines_free(pblk); >>> kfree(pblk->pad_dist); >>> - pblk_line_meta_free(pblk); >>> + pblk_line_mg_free(pblk); >>> pblk_core_free(pblk); >>> pblk_l2p_free(pblk); >>> @@ -1110,7 +1121,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk, >>> fail_free_pad_dist: >>> kfree(pblk->pad_dist); >>> fail_free_line_meta: >>> - pblk_line_meta_free(pblk); >>> + pblk_line_mg_free(pblk); >>> fail_free_luns: >>> pblk_luns_free(pblk); >>> fail: >>> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h >>> index 88720e2441c0..282dfc8780e8 100644 >>> --- a/drivers/lightnvm/pblk.h >>> +++ b/drivers/lightnvm/pblk.h >>> @@ -201,12 +201,6 @@ struct pblk_rb { >>> struct pblk_lun { >>> struct ppa_addr bppa; >>> - >>> - u8 *bb_list; /* Bad block list for LUN. Only used on >>> - * bring up. Bad blocks are managed >>> - * within lines on run-time. >>> - */ >>> - >>> struct semaphore wr_sem; >>> }; >>> >> >> Looks good to me. >> >> With respect to the 2.0 implementation. I am thinking that get_bb and set_bb should behave in the following way: >> >> For get_bb (in nvme/host/lightnvm.c) >> 1.2: Keep the implementation as is. >> 2.0: Use the report chunk command, and redo the get_bb format. >> >> For set_bb (in nvme/host/lightnvm.c) >> 1.2: Business as usual >> 2.0: Report error. > > I have a patches abstracting this, which I think it makes it cleaner. I can push it next week for review. I’m traveling this week. (If you want to get a glimpse I can point you to the code). > Yes, please do. Thanks >> >> For 2.0, there will be added a function pointer for get report chunk implementation, where the client can ask for full chunk information. Similarly here, client will fail the function call if the drive is 1.2. >> >> I hope to post the small 2.0 identify implementation Tomorrow or Friday, and then you can post the report chunk implementation that you have done if you like > > Sure. I have patches implementing 2.0 too, but you can push your version. One thing I’d like to get in is a generic geometry structure that simplifies the identify command and puts the logic in the identify path. Agree, that comes naturally when adding the support. > > This makes the base group structure from 1.2 go away makes the 1.2 and 2.0 paths easier to maintain at the target level. > > I can point you to the patches tomorrow if you want to align with it. Otherwise, I can just rebase and send them next week. But please, consider this before making a new abstraction to make the 2 specs coexist - most of the work is done already... Thanks. Yes, they should co-exist. > > >> We also need to take a look at the new error codes, which does not align with the 1.2 specification (now that they actually follow the nvme specification ;)) > > Cool. Let’s maintain the 1.2 errors for compatibility and make the 2.0 path clean - that is nvme compatible ;) > > Javier. >
> On 31 Jan 2018, at 19.24, Matias Bjørling <mb@lightnvm.io> wrote: > > On 01/31/2018 10:13 AM, Javier Gonzalez wrote: >>> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@lightnvm.io> wrote: >>> >> I have a patches abstracting this, which I think it makes it cleaner. I can push it next week for review. I’m traveling this week. (If you want to get a glimpse I can point you to the code). > > Yes, please do. Thanks This is the release candidate for 2.0 support based on 4.17. I'll rebase on top of you 2.0 support. We'll see if all changes make it to 4.17 then. https://github.com/OpenChannelSSD/linux/tree/for-4.17/spec20 Javier
On 02/04/2018 11:37 AM, Javier Gonzalez wrote: > >> On 31 Jan 2018, at 19.24, Matias Bjørling <mb@lightnvm.io> wrote: >> >> On 01/31/2018 10:13 AM, Javier Gonzalez wrote: >>>> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@lightnvm.io> wrote: >>>> >>> I have a patches abstracting this, which I think it makes it cleaner. I can push it next week for review. I’m traveling this week. (If you want to get a glimpse I can point you to the code). >> >> Yes, please do. Thanks > > This is the release candidate for 2.0 support based on 4.17. I'll rebase > on top of you 2.0 support. We'll see if all changes make it to 4.17 > then. > > https://github.com/OpenChannelSSD/linux/tree/for-4.17/spec20 > > Javier > Great. I look forward to be patches being cleaned up and posted. I do see some nitpicks here and there, which we properly can take a couple of stabs at. One think that generally stands out to me is the "if 1.2 support", else, ... statements. These could be structured better by having dedicated setup functions for 1.2 and 2.0.
> On 4 Feb 2018, at 13.55, Matias Bjørling <mb@lightnvm.io> wrote: > > On 02/04/2018 11:37 AM, Javier Gonzalez wrote: >>> On 31 Jan 2018, at 19.24, Matias Bjørling <mb@lightnvm.io> wrote: >>> >>> On 01/31/2018 10:13 AM, Javier Gonzalez wrote: >>>>> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@lightnvm.io> wrote: >>>>> >>>> I have a patches abstracting this, which I think it makes it cleaner. I can push it next week for review. I’m traveling this week. (If you want to get a glimpse I can point you to the code). >>> >>> Yes, please do. Thanks >> This is the release candidate for 2.0 support based on 4.17. I'll rebase >> on top of you 2.0 support. We'll see if all changes make it to 4.17 >> then. >> https://github.com/OpenChannelSSD/linux/tree/for-4.17/spec20 >> Javier > > Great. I look forward to be patches being cleaned up and posted. I do see some nitpicks here and there, which we properly can take a couple of stabs at. Sure. This is still in development; just wanted to point to the abstractions I’m thinking of so that we don’t do the same work twice. I’ll wait for posting until you do the 2.0 identify, since the old version is implemented on the first patch of this series. > One think that generally stands out to me is the "if 1.2 support", else, ... statements. These could be structured better by having dedicated setup functions for 1.2 and 2.0. We have this construction both in pblk and in core for address translation. Note that we need to have them separated to support multi instance and keep channels decoupled from each instance. I assume 2 if...then is cheaper than doing 2 de-references to function pointers. This is the way it is done on legacy paths in other places (e.g., non mq scsi), but I can look into how pointer functions would look like and measure the performance impact. Javier
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c index a5e3510c0f38..86a94a7faa96 100644 --- a/drivers/lightnvm/pblk-init.c +++ b/drivers/lightnvm/pblk-init.c @@ -365,7 +365,25 @@ static void pblk_luns_free(struct pblk *pblk) kfree(pblk->luns); } -static void pblk_free_line_bitmaps(struct pblk_line *line) +static void pblk_line_mg_free(struct pblk *pblk) +{ + struct pblk_line_mgmt *l_mg = &pblk->l_mg; + int i; + + kfree(l_mg->bb_template); + kfree(l_mg->bb_aux); + kfree(l_mg->vsc_list); + + for (i = 0; i < PBLK_DATA_LINES; i++) { + kfree(l_mg->sline_meta[i]); + pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); + kfree(l_mg->eline_meta[i]); + } + + kfree(pblk->lines); +} + +static void pblk_line_meta_free(struct pblk_line *line) { kfree(line->blk_bitmap); kfree(line->erase_bitmap); @@ -382,40 +400,16 @@ static void pblk_lines_free(struct pblk *pblk) line = &pblk->lines[i]; pblk_line_free(pblk, line); - pblk_free_line_bitmaps(line); + pblk_line_meta_free(line); } spin_unlock(&l_mg->free_lock); } -static void pblk_line_meta_free(struct pblk *pblk) +static int pblk_bb_get_tbl(struct nvm_tgt_dev *dev, struct pblk_lun *rlun, + u8 *blks, int nr_blks) { - struct pblk_line_mgmt *l_mg = &pblk->l_mg; - int i; - - kfree(l_mg->bb_template); - kfree(l_mg->bb_aux); - kfree(l_mg->vsc_list); - - for (i = 0; i < PBLK_DATA_LINES; i++) { - kfree(l_mg->sline_meta[i]); - pblk_mfree(l_mg->eline_meta[i]->buf, l_mg->emeta_alloc_type); - kfree(l_mg->eline_meta[i]); - } - - kfree(pblk->lines); -} - -static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) -{ - struct nvm_geo *geo = &dev->geo; struct ppa_addr ppa; - u8 *blks; - int nr_blks, ret; - - nr_blks = geo->nr_chks * geo->plane_mode; - blks = kmalloc(nr_blks, GFP_KERNEL); - if (!blks) - return -ENOMEM; + int ret; ppa.ppa = 0; ppa.g.ch = rlun->bppa.g.ch; @@ -423,34 +417,56 @@ static int pblk_bb_discovery(struct nvm_tgt_dev *dev, struct pblk_lun *rlun) ret = nvm_get_tgt_bb_tbl(dev, ppa, blks); if (ret) - goto out; + return ret; nr_blks = nvm_bb_tbl_fold(dev->parent, blks, nr_blks); - if (nr_blks < 0) { - ret = nr_blks; - goto out; - } - - rlun->bb_list = blks; + if (nr_blks < 0) + return -EIO; return 0; -out: - kfree(blks); - return ret; +} + +static void *pblk_bb_get_log(struct pblk *pblk) +{ + struct nvm_tgt_dev *dev = pblk->dev; + struct nvm_geo *geo = &dev->geo; + u8 *log; + int i, nr_blks, blk_per_lun; + int ret; + + blk_per_lun = geo->nr_chks * geo->plane_mode; + nr_blks = blk_per_lun * geo->all_luns; + + log = kmalloc(nr_blks, GFP_KERNEL); + if (!log) + return ERR_PTR(-ENOMEM); + + for (i = 0; i < geo->all_luns; i++) { + struct pblk_lun *rlun = &pblk->luns[i]; + u8 *log_pos = log + i * blk_per_lun; + + ret = pblk_bb_get_tbl(dev, rlun, log_pos, blk_per_lun); + if (ret) { + kfree(log); + return ERR_PTR(-EIO); + } + } + + return log; } static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, - int blk_per_line) + u8 *bb_log, int blk_per_line) { struct nvm_tgt_dev *dev = pblk->dev; struct nvm_geo *geo = &dev->geo; - struct pblk_lun *rlun; - int bb_cnt = 0; - int i; + int i, bb_cnt = 0; for (i = 0; i < blk_per_line; i++) { - rlun = &pblk->luns[i]; - if (rlun->bb_list[line->id] == NVM_BLK_T_FREE) + struct pblk_lun *rlun = &pblk->luns[i]; + u8 *lun_bb_log = bb_log + i * blk_per_line; + + if (lun_bb_log[line->id] == NVM_BLK_T_FREE) continue; set_bit(pblk_ppa_to_pos(geo, rlun->bppa), line->blk_bitmap); @@ -460,29 +476,12 @@ static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line, return bb_cnt; } -static int pblk_alloc_line_bitmaps(struct pblk *pblk, struct pblk_line *line) -{ - struct pblk_line_meta *lm = &pblk->lm; - - line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); - if (!line->blk_bitmap) - return -ENOMEM; - - line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); - if (!line->erase_bitmap) { - kfree(line->blk_bitmap); - return -ENOMEM; - } - - return 0; -} - static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) { struct nvm_tgt_dev *dev = pblk->dev; struct nvm_geo *geo = &dev->geo; struct pblk_lun *rlun; - int i, ret; + int i; /* TODO: Implement unbalanced LUN support */ if (geo->nr_luns < 0) { @@ -505,13 +504,6 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns) rlun->bppa = luns[lunid]; sema_init(&rlun->wr_sem, 1); - - ret = pblk_bb_discovery(dev, rlun); - if (ret) { - while (--i >= 0) - kfree(pblk->luns[i].bb_list); - return ret; - } } return 0; @@ -689,6 +681,26 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk) return -ENOMEM; } +static int pblk_setup_line_meta(struct pblk *pblk, struct pblk_line *line, + void *chunk_log, long *nr_bad_blks) +{ + struct pblk_line_meta *lm = &pblk->lm; + + line->blk_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); + if (!line->blk_bitmap) + return -ENOMEM; + + line->erase_bitmap = kzalloc(lm->blk_bitmap_len, GFP_KERNEL); + if (!line->erase_bitmap) { + kfree(line->blk_bitmap); + return -ENOMEM; + } + + *nr_bad_blks = pblk_bb_line(pblk, line, chunk_log, lm->blk_per_line); + + return 0; +} + static int pblk_lines_init(struct pblk *pblk) { struct nvm_tgt_dev *dev = pblk->dev; @@ -696,8 +708,9 @@ static int pblk_lines_init(struct pblk *pblk) struct pblk_line_mgmt *l_mg = &pblk->l_mg; struct pblk_line_meta *lm = &pblk->lm; struct pblk_line *line; + void *chunk_log; unsigned int smeta_len, emeta_len; - long nr_bad_blks, nr_free_blks; + long nr_bad_blks = 0, nr_free_blks = 0; int bb_distance, max_write_ppas, mod; int i, ret; @@ -771,13 +784,12 @@ static int pblk_lines_init(struct pblk *pblk) if (lm->min_blk_line > lm->blk_per_line) { pr_err("pblk: config. not supported. Min. LUN in line:%d\n", lm->blk_per_line); - ret = -EINVAL; - goto fail; + return -EINVAL; } ret = pblk_lines_alloc_metadata(pblk); if (ret) - goto fail; + return ret; l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL); if (!l_mg->bb_template) { @@ -821,9 +833,16 @@ static int pblk_lines_init(struct pblk *pblk) goto fail_free_bb_aux; } - nr_free_blks = 0; + chunk_log = pblk_bb_get_log(pblk); + if (IS_ERR(chunk_log)) { + pr_err("pblk: could not get bad block log (%lu)\n", + PTR_ERR(chunk_log)); + ret = PTR_ERR(chunk_log); + goto fail_free_lines; + } + for (i = 0; i < l_mg->nr_lines; i++) { - int blk_in_line; + int chk_in_line; line = &pblk->lines[i]; @@ -835,26 +854,20 @@ static int pblk_lines_init(struct pblk *pblk) line->vsc = &l_mg->vsc_list[i]; spin_lock_init(&line->lock); - ret = pblk_alloc_line_bitmaps(pblk, line); + ret = pblk_setup_line_meta(pblk, line, chunk_log, &nr_bad_blks); if (ret) - goto fail_free_lines; + goto fail_free_chunk_log; - nr_bad_blks = pblk_bb_line(pblk, line, lm->blk_per_line); - if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line) { - pblk_free_line_bitmaps(line); - ret = -EINVAL; - goto fail_free_lines; - } - - blk_in_line = lm->blk_per_line - nr_bad_blks; - if (blk_in_line < lm->min_blk_line) { + chk_in_line = lm->blk_per_line - nr_bad_blks; + if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line || + chk_in_line < lm->min_blk_line) { line->state = PBLK_LINESTATE_BAD; list_add_tail(&line->list, &l_mg->bad_list); continue; } - nr_free_blks += blk_in_line; - atomic_set(&line->blk_in_line, blk_in_line); + nr_free_blks += chk_in_line; + atomic_set(&line->blk_in_line, chk_in_line); l_mg->nr_free_lines++; list_add_tail(&line->list, &l_mg->free_list); @@ -862,23 +875,21 @@ static int pblk_lines_init(struct pblk *pblk) pblk_set_provision(pblk, nr_free_blks); - /* Cleanup per-LUN bad block lists - managed within lines on run-time */ - for (i = 0; i < geo->all_luns; i++) - kfree(pblk->luns[i].bb_list); - + kfree(chunk_log); return 0; -fail_free_lines: + +fail_free_chunk_log: + kfree(chunk_log); while (--i >= 0) - pblk_free_line_bitmaps(&pblk->lines[i]); + pblk_line_meta_free(&pblk->lines[i]); +fail_free_lines: + kfree(pblk->lines); fail_free_bb_aux: kfree(l_mg->bb_aux); fail_free_bb_template: kfree(l_mg->bb_template); fail_free_meta: - pblk_line_meta_free(pblk); -fail: - for (i = 0; i < geo->all_luns; i++) - kfree(pblk->luns[i].bb_list); + pblk_line_mg_free(pblk); return ret; } @@ -922,7 +933,7 @@ static void pblk_free(struct pblk *pblk) pblk_luns_free(pblk); pblk_lines_free(pblk); kfree(pblk->pad_dist); - pblk_line_meta_free(pblk); + pblk_line_mg_free(pblk); pblk_core_free(pblk); pblk_l2p_free(pblk); @@ -1110,7 +1121,7 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk, fail_free_pad_dist: kfree(pblk->pad_dist); fail_free_line_meta: - pblk_line_meta_free(pblk); + pblk_line_mg_free(pblk); fail_free_luns: pblk_luns_free(pblk); fail: diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h index 88720e2441c0..282dfc8780e8 100644 --- a/drivers/lightnvm/pblk.h +++ b/drivers/lightnvm/pblk.h @@ -201,12 +201,6 @@ struct pblk_rb { struct pblk_lun { struct ppa_addr bppa; - - u8 *bb_list; /* Bad block list for LUN. Only used on - * bring up. Bad blocks are managed - * within lines on run-time. - */ - struct semaphore wr_sem; };
In preparation for the OCSSD 2.0 spec. bad block identification, refactor the current code to generalize bad block get/set functions and structures. Signed-off-by: Javier González <javier@cnexlabs.com> --- drivers/lightnvm/pblk-init.c | 213 +++++++++++++++++++++++-------------------- drivers/lightnvm/pblk.h | 6 -- 2 files changed, 112 insertions(+), 107 deletions(-)