| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 31 Jan 2018 19:24:19 +0100
From: Matias Bjørling <mb@...htnvm.io>
To: Javier Gonzalez <javier@...xlabs.com>
Cc: Javier González <jg@...htnvm.io>,
"linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/5] lightnvm: pblk: refactor bad block identification
On 01/31/2018 10:13 AM, Javier Gonzalez wrote:
>
>
>> On 31 Jan 2018, at 16.51, Matias Bjørling <mb@...htnvm.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@...xlabs.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.
>
Powered by blists - more mailing lists