Message-Id: <12DCD7A9-01CB-4CCE-8F6A-6B892056F5F0@javigon.com>
Date:   Mon, 5 Mar 2018 14:45:29 +0100
From:   Javier González <javier@...igon.com>
To:     Matias Bjørling <mb@...htnvm.io>
Cc:     linux-block@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Dan Carpenter <dan.carpenter@...cle.com>
Subject: Re: [PATCH] lightnvm: pblk: refactor init/exit sequences

> On 5 Mar 2018, at 14.38, Matias Bjørling <mb@...htnvm.io> wrote:
> 
> On 03/01/2018 08:29 PM, Javier González wrote:
>>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@...htnvm.io> wrote:
>>> 
>>> On 03/01/2018 04:59 PM, Javier González wrote:
>>>> Refactor init and exit sequences to eliminate dependencies among init
>>>> modules and improve readability.
>>>> Signed-off-by: Javier González <javier@...xlabs.com>
>>>> ---
>>>>  drivers/lightnvm/pblk-init.c | 415 +++++++++++++++++++++----------------------
>>>>  1 file changed, 206 insertions(+), 209 deletions(-)
>>>> diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
>>>> index 25fc70ca07f7..87c390667dd6 100644
>>>> --- a/drivers/lightnvm/pblk-init.c
>>>> +++ b/drivers/lightnvm/pblk-init.c
>>>> @@ -103,7 +103,40 @@ static void pblk_l2p_free(struct pblk *pblk)
>>>>  	vfree(pblk->trans_map);
>>>>  }
>>>>  -static int pblk_l2p_init(struct pblk *pblk)
>>>> +static int pblk_l2p_recover(struct pblk *pblk, bool factory_init)
>>>> +{
>>>> +	struct pblk_line *line = NULL;
>>>> +
>>>> +	if (factory_init) {
>>>> +		pblk_setup_uuid(pblk);
>>>> +	} else {
>>>> +		line = pblk_recov_l2p(pblk);
>>>> +		if (IS_ERR(line)) {
>>>> +			pr_err("pblk: could not recover l2p table\n");
>>>> +			return -EFAULT;
>>>> +		}
>>>> +	}
>>>> +
>>>> +#ifdef CONFIG_NVM_DEBUG
>>>> +	pr_info("pblk init: L2P CRC: %x\n", pblk_l2p_crc(pblk));
>>>> +#endif
>>>> +
>>>> +	/* Free full lines directly as GC has not been started yet */
>>>> +	pblk_gc_free_full_lines(pblk);
>>>> +
>>>> +	if (!line) {
>>>> +		/* Configure next line for user data */
>>>> +		line = pblk_line_get_first_data(pblk);
>>>> +		if (!line) {
>>>> +			pr_err("pblk: line list corrupted\n");
>>>> +			return -EFAULT;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +static int pblk_l2p_init(struct pblk *pblk, bool factory_init)
>>>>  {
>>>>  	sector_t i;
>>>>  	struct ppa_addr ppa;
>>>> @@ -119,7 +152,7 @@ static int pblk_l2p_init(struct pblk *pblk)
>>>>  	for (i = 0; i < pblk->rl.nr_secs; i++)
>>>>  		pblk_trans_map_set(pblk, i, ppa);
>>>>  -	return 0;
>>>> +	return pblk_l2p_recover(pblk, factory_init);
>>>>  }
>>>>    static void pblk_rwb_free(struct pblk *pblk)
>>>> @@ -159,7 +192,13 @@ static int pblk_set_ppaf(struct pblk *pblk)
>>>>  	struct nvm_tgt_dev *dev = pblk->dev;
>>>>  	struct nvm_geo *geo = &dev->geo;
>>>>  	struct nvm_addr_format ppaf = geo->ppaf;
>>>> -	int power_len;
>>>> +	int mod, power_len;
>>>> +
>>>> +	div_u64_rem(geo->sec_per_chk, pblk->min_write_pgs, &mod);
>>>> +	if (mod) {
>>>> +		pr_err("pblk: bad configuration of sectors/pages\n");
>>>> +		return -EINVAL;
>>>> +	}
>>>>    	/* Re-calculate channel and lun format to adapt to configuration */
>>>>  	power_len = get_count_order(geo->nr_chnls);
>>>> @@ -252,12 +291,39 @@ static int pblk_core_init(struct pblk *pblk)
>>>>  {
>>>>  	struct nvm_tgt_dev *dev = pblk->dev;
>>>>  	struct nvm_geo *geo = &dev->geo;
>>>> +	int max_write_ppas;
>>>> +
>>>> +	atomic64_set(&pblk->user_wa, 0);
>>>> +	atomic64_set(&pblk->pad_wa, 0);
>>>> +	atomic64_set(&pblk->gc_wa, 0);
>>>> +	pblk->user_rst_wa = 0;
>>>> +	pblk->pad_rst_wa = 0;
>>>> +	pblk->gc_rst_wa = 0;
>>>> +
>>>> +	atomic64_set(&pblk->nr_flush, 0);
>>>> +	pblk->nr_flush_rst = 0;
>>>>    	pblk->pgs_in_buffer = NVM_MEM_PAGE_WRITE * geo->sec_per_pg *
>>>>  						geo->nr_planes * geo->all_luns;
>>>>  +	pblk->min_write_pgs = geo->sec_per_pl * (geo->sec_size / PAGE_SIZE);
>>>> +	max_write_ppas = pblk->min_write_pgs * geo->all_luns;
>>>> +	pblk->max_write_pgs = min_t(int, max_write_ppas, NVM_MAX_VLBA);
>>>> +	pblk_set_sec_per_write(pblk, pblk->min_write_pgs);
>>>> +
>>>> +	if (pblk->max_write_pgs > PBLK_MAX_REQ_ADDRS) {
>>>> +		pr_err("pblk: vector list too big(%u > %u)\n",
>>>> +				pblk->max_write_pgs, PBLK_MAX_REQ_ADDRS);
>>>> +		return -EINVAL;
>>>> +	}
>>>> +
>>>> +	pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) * sizeof(atomic64_t),
>>>> +								GFP_KERNEL);
>>>> +	if (!pblk->pad_dist)
>>>> +		return -ENOMEM;
>>>> +
>>>>  	if (pblk_init_global_caches(pblk))
>>>> -		return -ENOMEM;
>>>> +		goto fail_free_pad_dist;
>>>>    	/* Internal bios can be at most the sectors signaled by the device. */
>>>>  	pblk->page_bio_pool = mempool_create_page_pool(NVM_MAX_VLBA, 0);
>>>> @@ -307,10 +373,8 @@ static int pblk_core_init(struct pblk *pblk)
>>>>  	if (pblk_set_ppaf(pblk))
>>>>  		goto free_r_end_wq;
>>>>  -	if (pblk_rwb_init(pblk))
>>>> -		goto free_r_end_wq;
>>>> -
>>>>  	INIT_LIST_HEAD(&pblk->compl_list);
>>>> +
>>>>  	return 0;
>>>>    free_r_end_wq:
>>>> @@ -333,6 +397,8 @@ static int pblk_core_init(struct pblk *pblk)
>>>>  	mempool_destroy(pblk->page_bio_pool);
>>>>  free_global_caches:
>>>>  	pblk_free_global_caches(pblk);
>>>> +fail_free_pad_dist:
>>>> +	kfree(pblk->pad_dist);
>>>>  	return -ENOMEM;
>>>>  }
>>>>  @@ -354,14 +420,8 @@ static void pblk_core_free(struct pblk *pblk)
>>>>  	mempool_destroy(pblk->e_rq_pool);
>>>>  	mempool_destroy(pblk->w_rq_pool);
>>>>  -	pblk_rwb_free(pblk);
>>>> -
>>>>  	pblk_free_global_caches(pblk);
>>>> -}
>>>> -
>>>> -static void pblk_luns_free(struct pblk *pblk)
>>>> -{
>>>> -	kfree(pblk->luns);
>>>> +	kfree(pblk->pad_dist);
>>>>  }
>>>>    static void pblk_line_mg_free(struct pblk *pblk)
>>>> @@ -378,8 +438,6 @@ static void pblk_line_mg_free(struct pblk *pblk)
>>>>  		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)
>>>> @@ -402,6 +460,11 @@ static void pblk_lines_free(struct pblk *pblk)
>>>>  		pblk_line_meta_free(line);
>>>>  	}
>>>>  	spin_unlock(&l_mg->free_lock);
>>>> +
>>>> +	pblk_line_mg_free(pblk);
>>>> +
>>>> +	kfree(pblk->luns);
>>>> +	kfree(pblk->lines);
>>>>  }
>>>>    static int pblk_bb_get_tbl(struct nvm_tgt_dev *dev, struct pblk_lun *rlun,
>>>> @@ -476,7 +539,7 @@ static int pblk_bb_line(struct pblk *pblk, struct pblk_line *line,
>>>>  	return bb_cnt;
>>>>  }
>>>>  -static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns)
>>>> +static int pblk_luns_init(struct pblk *pblk)
>>>>  {
>>>>  	struct nvm_tgt_dev *dev = pblk->dev;
>>>>  	struct nvm_geo *geo = &dev->geo;
>>>> @@ -501,7 +564,7 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns)
>>>>  		int lunid = lun_raw + ch * geo->nr_luns;
>>>>    		rlun = &pblk->luns[i];
>>>> -		rlun->bppa = luns[lunid];
>>>> +		rlun->bppa = dev->luns[lunid];
>>>>    		sema_init(&rlun->wr_sem, 1);
>>>>  	}
>>>> @@ -509,38 +572,6 @@ static int pblk_luns_init(struct pblk *pblk, struct ppa_addr *luns)
>>>>  	return 0;
>>>>  }
>>>>  -static int pblk_lines_configure(struct pblk *pblk, int flags)
>>>> -{
>>>> -	struct pblk_line *line = NULL;
>>>> -	int ret = 0;
>>>> -
>>>> -	if (!(flags & NVM_TARGET_FACTORY)) {
>>>> -		line = pblk_recov_l2p(pblk);
>>>> -		if (IS_ERR(line)) {
>>>> -			pr_err("pblk: could not recover l2p table\n");
>>>> -			ret = -EFAULT;
>>>> -		}
>>>> -	}
>>>> -
>>>> -#ifdef CONFIG_NVM_DEBUG
>>>> -	pr_info("pblk init: L2P CRC: %x\n", pblk_l2p_crc(pblk));
>>>> -#endif
>>>> -
>>>> -	/* Free full lines directly as GC has not been started yet */
>>>> -	pblk_gc_free_full_lines(pblk);
>>>> -
>>>> -	if (!line) {
>>>> -		/* Configure next line for user data */
>>>> -		line = pblk_line_get_first_data(pblk);
>>>> -		if (!line) {
>>>> -			pr_err("pblk: line list corrupted\n");
>>>> -			ret = -EFAULT;
>>>> -		}
>>>> -	}
>>>> -
>>>> -	return ret;
>>>> -}
>>>> -
>>>>  /* See comment over struct line_emeta definition */
>>>>  static unsigned int calc_emeta_len(struct pblk *pblk)
>>>>  {
>>>> @@ -606,11 +637,70 @@ static void pblk_set_provision(struct pblk *pblk, long nr_free_blks)
>>>>  	atomic_set(&pblk->rl.free_user_blocks, nr_free_blks);
>>>>  }
>>>>  -static int pblk_lines_alloc_metadata(struct pblk *pblk)
>>>> +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_line_mg_init(struct pblk *pblk)
>>>> +{
>>>> +	struct nvm_tgt_dev *dev = pblk->dev;
>>>> +	struct nvm_geo *geo = &dev->geo;
>>>>  	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
>>>>  	struct pblk_line_meta *lm = &pblk->lm;
>>>> -	int i;
>>>> +	int i, bb_distance;
>>>> +
>>>> +	l_mg->nr_lines = geo->nr_chks;
>>>> +	l_mg->log_line = l_mg->data_line = NULL;
>>>> +	l_mg->l_seq_nr = l_mg->d_seq_nr = 0;
>>>> +	l_mg->nr_free_lines = 0;
>>>> +	bitmap_zero(&l_mg->meta_bitmap, PBLK_DATA_LINES);
>>>> +
>>>> +	INIT_LIST_HEAD(&l_mg->free_list);
>>>> +	INIT_LIST_HEAD(&l_mg->corrupt_list);
>>>> +	INIT_LIST_HEAD(&l_mg->bad_list);
>>>> +	INIT_LIST_HEAD(&l_mg->gc_full_list);
>>>> +	INIT_LIST_HEAD(&l_mg->gc_high_list);
>>>> +	INIT_LIST_HEAD(&l_mg->gc_mid_list);
>>>> +	INIT_LIST_HEAD(&l_mg->gc_low_list);
>>>> +	INIT_LIST_HEAD(&l_mg->gc_empty_list);
>>>> +
>>>> +	INIT_LIST_HEAD(&l_mg->emeta_list);
>>>> +
>>>> +	l_mg->gc_lists[0] = &l_mg->gc_high_list;
>>>> +	l_mg->gc_lists[1] = &l_mg->gc_mid_list;
>>>> +	l_mg->gc_lists[2] = &l_mg->gc_low_list;
>>>> +
>>>> +	spin_lock_init(&l_mg->free_lock);
>>>> +	spin_lock_init(&l_mg->close_lock);
>>>> +	spin_lock_init(&l_mg->gc_lock);
>>>> +
>>>> +	l_mg->vsc_list = kcalloc(l_mg->nr_lines, sizeof(__le32), GFP_KERNEL);
>>>> +	if (!l_mg->vsc_list)
>>>> +		goto fail;
>>>> +
>>>> +	l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
>>>> +	if (!l_mg->bb_template)
>>>> +		goto fail_free_vsc_list;
>>>> +
>>>> +	l_mg->bb_aux = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
>>>> +	if (!l_mg->bb_aux)
>>>> +		goto fail_free_bb_template;
>>>>    	/* smeta is always small enough to fit on a kmalloc memory allocation,
>>>>  	 * emeta depends on the number of LUNs allocated to the pblk instance
>>>> @@ -656,13 +746,13 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk)
>>>>  		}
>>>>  	}
>>>>  -	l_mg->vsc_list = kcalloc(l_mg->nr_lines, sizeof(__le32), GFP_KERNEL);
>>>> -	if (!l_mg->vsc_list)
>>>> -		goto fail_free_emeta;
>>>> -
>>>>  	for (i = 0; i < l_mg->nr_lines; i++)
>>>>  		l_mg->vsc_list[i] = cpu_to_le32(EMPTY_ENTRY);
>>>>  +	bb_distance = (geo->all_luns) * geo->ws_opt;
>>>> +	for (i = 0; i < lm->sec_per_line; i += bb_distance)
>>>> +		bitmap_set(l_mg->bb_template, i, geo->ws_opt);
>>>> +
>>>>  	return 0;
>>>>    fail_free_emeta:
>>>> @@ -673,69 +763,25 @@ static int pblk_lines_alloc_metadata(struct pblk *pblk)
>>>>  			kfree(l_mg->eline_meta[i]->buf);
>>>>  		kfree(l_mg->eline_meta[i]);
>>>>  	}
>>>> -
>>>>  fail_free_smeta:
>>>>  	for (i = 0; i < PBLK_DATA_LINES; i++)
>>>>  		kfree(l_mg->sline_meta[i]);
>>>> -
>>>> +	kfree(l_mg->bb_aux);
>>>> +fail_free_bb_template:
>>>> +	kfree(l_mg->bb_template);
>>>> +fail_free_vsc_list:
>>>> +	kfree(l_mg->vsc_list);
>>>> +fail:
>>>>  	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)
>>>> +static int pblk_line_meta_init(struct pblk *pblk)
>>>>  {
>>>>  	struct nvm_tgt_dev *dev = pblk->dev;
>>>>  	struct nvm_geo *geo = &dev->geo;
>>>> -	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 = 0, nr_free_blks = 0;
>>>> -	int bb_distance, max_write_ppas, mod;
>>>> -	int i, ret;
>>>> -
>>>> -	pblk->min_write_pgs = geo->sec_per_pl * (geo->sec_size / PAGE_SIZE);
>>>> -	max_write_ppas = pblk->min_write_pgs * geo->all_luns;
>>>> -	pblk->max_write_pgs = min_t(int, max_write_ppas, NVM_MAX_VLBA);
>>>> -	pblk_set_sec_per_write(pblk, pblk->min_write_pgs);
>>>> -
>>>> -	if (pblk->max_write_pgs > PBLK_MAX_REQ_ADDRS) {
>>>> -		pr_err("pblk: vector list too big(%u > %u)\n",
>>>> -				pblk->max_write_pgs, PBLK_MAX_REQ_ADDRS);
>>>> -		return -EINVAL;
>>>> -	}
>>>> -
>>>> -	div_u64_rem(geo->sec_per_chk, pblk->min_write_pgs, &mod);
>>>> -	if (mod) {
>>>> -		pr_err("pblk: bad configuration of sectors/pages\n");
>>>> -		return -EINVAL;
>>>> -	}
>>>> -
>>>> -	l_mg->nr_lines = geo->nr_chks;
>>>> -	l_mg->log_line = l_mg->data_line = NULL;
>>>> -	l_mg->l_seq_nr = l_mg->d_seq_nr = 0;
>>>> -	l_mg->nr_free_lines = 0;
>>>> -	bitmap_zero(&l_mg->meta_bitmap, PBLK_DATA_LINES);
>>>> +	int i;
>>>>    	lm->sec_per_line = geo->sec_per_chk * geo->all_luns;
>>>>  	lm->blk_per_line = geo->all_luns;
>>>> @@ -787,58 +833,43 @@ static int pblk_lines_init(struct pblk *pblk)
>>>>  		return -EINVAL;
>>>>  	}
>>>>  -	ret = pblk_lines_alloc_metadata(pblk);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +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;
>>>> +	long nr_bad_blks = 0, nr_free_blks = 0;
>>>> +	int i, ret;
>>>> +
>>>> +	ret = pblk_line_meta_init(pblk);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	ret = pblk_line_mg_init(pblk);
>>>>  	if (ret)
>>>>  		return ret;
>>>>  -	l_mg->bb_template = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
>>>> -	if (!l_mg->bb_template) {
>>>> -		ret = -ENOMEM;
>>>> +	ret = pblk_luns_init(pblk);
>>>> +	if (ret)
>>>>  		goto fail_free_meta;
>>>> -	}
>>>> -
>>>> -	l_mg->bb_aux = kzalloc(lm->sec_bitmap_len, GFP_KERNEL);
>>>> -	if (!l_mg->bb_aux) {
>>>> -		ret = -ENOMEM;
>>>> -		goto fail_free_bb_template;
>>>> -	}
>>>> -
>>>> -	bb_distance = (geo->all_luns) * geo->sec_per_pl;
>>>> -	for (i = 0; i < lm->sec_per_line; i += bb_distance)
>>>> -		bitmap_set(l_mg->bb_template, i, geo->sec_per_pl);
>>>> -
>>>> -	INIT_LIST_HEAD(&l_mg->free_list);
>>>> -	INIT_LIST_HEAD(&l_mg->corrupt_list);
>>>> -	INIT_LIST_HEAD(&l_mg->bad_list);
>>>> -	INIT_LIST_HEAD(&l_mg->gc_full_list);
>>>> -	INIT_LIST_HEAD(&l_mg->gc_high_list);
>>>> -	INIT_LIST_HEAD(&l_mg->gc_mid_list);
>>>> -	INIT_LIST_HEAD(&l_mg->gc_low_list);
>>>> -	INIT_LIST_HEAD(&l_mg->gc_empty_list);
>>>> -
>>>> -	INIT_LIST_HEAD(&l_mg->emeta_list);
>>>> -
>>>> -	l_mg->gc_lists[0] = &l_mg->gc_high_list;
>>>> -	l_mg->gc_lists[1] = &l_mg->gc_mid_list;
>>>> -	l_mg->gc_lists[2] = &l_mg->gc_low_list;
>>>> -
>>>> -	spin_lock_init(&l_mg->free_lock);
>>>> -	spin_lock_init(&l_mg->close_lock);
>>>> -	spin_lock_init(&l_mg->gc_lock);
>>>> -
>>>> -	pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
>>>> -								GFP_KERNEL);
>>>> -	if (!pblk->lines) {
>>>> -		ret = -ENOMEM;
>>>> -		goto fail_free_bb_aux;
>>>> -	}
>>>>    	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;
>>>> +		goto fail_free_luns;
>>>> +	}
>>>> +
>>>> +	pblk->lines = kcalloc(l_mg->nr_lines, sizeof(struct pblk_line),
>>>> +								GFP_KERNEL);
>>>> +	if (!pblk->lines) {
>>>> +		ret = -ENOMEM;
>>>> +		goto fail_free_chunk_log;
>>>>  	}
>>>>    	for (i = 0; i < l_mg->nr_lines; i++) {
>>>> @@ -856,7 +887,7 @@ static int pblk_lines_init(struct pblk *pblk)
>>>>    		ret = pblk_setup_line_meta(pblk, line, chunk_log, &nr_bad_blks);
>>>>  		if (ret)
>>>> -			goto fail_free_chunk_log;
>>>> +			goto fail_free_lines;
>>>>    		chk_in_line = lm->blk_per_line - nr_bad_blks;
>>>>  		if (nr_bad_blks < 0 || nr_bad_blks > lm->blk_per_line ||
>>>> @@ -878,16 +909,14 @@ static int pblk_lines_init(struct pblk *pblk)
>>>>  	kfree(chunk_log);
>>>>  	return 0;
>>>>  -fail_free_chunk_log:
>>>> -	kfree(chunk_log);
>>>> +fail_free_lines:
>>>>  	while (--i >= 0)
>>>>  		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_chunk_log:
>>>> +	kfree(chunk_log);
>>>> +fail_free_luns:
>>>> +	kfree(pblk->luns);
>>>>  fail_free_meta:
>>>>  	pblk_line_mg_free(pblk);
>>>>  @@ -930,12 +959,10 @@ static void pblk_writer_stop(struct pblk *pblk)
>>>>    static void pblk_free(struct pblk *pblk)
>>>>  {
>>>> -	pblk_luns_free(pblk);
>>>>  	pblk_lines_free(pblk);
>>>> -	kfree(pblk->pad_dist);
>>>> -	pblk_line_mg_free(pblk);
>>>> -	pblk_core_free(pblk);
>>>>  	pblk_l2p_free(pblk);
>>>> +	pblk_rwb_free(pblk);
>>>> +	pblk_core_free(pblk);
>>>>    	kfree(pblk);
>>>>  }
>>>> @@ -1000,19 +1027,6 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>>>>  	spin_lock_init(&pblk->trans_lock);
>>>>  	spin_lock_init(&pblk->lock);
>>>>  -	if (flags & NVM_TARGET_FACTORY)
>>>> -		pblk_setup_uuid(pblk);
>>>> -
>>>> -	atomic64_set(&pblk->user_wa, 0);
>>>> -	atomic64_set(&pblk->pad_wa, 0);
>>>> -	atomic64_set(&pblk->gc_wa, 0);
>>>> -	pblk->user_rst_wa = 0;
>>>> -	pblk->pad_rst_wa = 0;
>>>> -	pblk->gc_rst_wa = 0;
>>>> -
>>>> -	atomic64_set(&pblk->nr_flush, 0);
>>>> -	pblk->nr_flush_rst = 0;
>>>> -
>>>>  #ifdef CONFIG_NVM_DEBUG
>>>>  	atomic_long_set(&pblk->inflight_writes, 0);
>>>>  	atomic_long_set(&pblk->padded_writes, 0);
>>>> @@ -1036,48 +1050,35 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>>>>  	atomic_long_set(&pblk->write_failed, 0);
>>>>  	atomic_long_set(&pblk->erase_failed, 0);
>>>>  -	ret = pblk_luns_init(pblk, dev->luns);
>>>> -	if (ret) {
>>>> -		pr_err("pblk: could not initialize luns\n");
>>>> -		goto fail;
>>>> -	}
>>>> -
>>>> -	ret = pblk_lines_init(pblk);
>>>> -	if (ret) {
>>>> -		pr_err("pblk: could not initialize lines\n");
>>>> -		goto fail_free_luns;
>>>> -	}
>>>> -
>>>> -	pblk->pad_dist = kzalloc((pblk->min_write_pgs - 1) * sizeof(atomic64_t),
>>>> -				 GFP_KERNEL);
>>>> -	if (!pblk->pad_dist) {
>>>> -		ret = -ENOMEM;
>>>> -		goto fail_free_line_meta;
>>>> -	}
>>>> -
>>>>  	ret = pblk_core_init(pblk);
>>>>  	if (ret) {
>>>>  		pr_err("pblk: could not initialize core\n");
>>>> -		goto fail_free_pad_dist;
>>>> +		goto fail;
>>>>  	}
>>>>  -	ret = pblk_l2p_init(pblk);
>>>> +	ret = pblk_lines_init(pblk);
>>>>  	if (ret) {
>>>> -		pr_err("pblk: could not initialize maps\n");
>>>> +		pr_err("pblk: could not initialize lines\n");
>>>>  		goto fail_free_core;
>>>>  	}
>>>>  -	ret = pblk_lines_configure(pblk, flags);
>>>> +	ret = pblk_rwb_init(pblk);
>>>>  	if (ret) {
>>>> -		pr_err("pblk: could not configure lines\n");
>>>> -		goto fail_free_l2p;
>>>> +		pr_err("pblk: could not initialize write buffer\n");
>>>> +		goto fail_free_lines;
>>>> +	}
>>>> +
>>>> +	ret = pblk_l2p_init(pblk, flags & NVM_TARGET_FACTORY);
>>>> +	if (ret) {
>>>> +		pr_err("pblk: could not initialize maps\n");
>>>> +		goto fail_free_rwb;
>>>>  	}
>>>>    	ret = pblk_writer_init(pblk);
>>>>  	if (ret) {
>>>>  		if (ret != -EINTR)
>>>>  			pr_err("pblk: could not initialize write thread\n");
>>>> -		goto fail_free_lines;
>>>> +		goto fail_free_l2p;
>>>>  	}
>>>>    	ret = pblk_gc_init(pblk);
>>>> @@ -1112,18 +1113,14 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
>>>>    fail_stop_writer:
>>>>  	pblk_writer_stop(pblk);
>>>> -fail_free_lines:
>>>> -	pblk_lines_free(pblk);
>>>>  fail_free_l2p:
>>>>  	pblk_l2p_free(pblk);
>>>> +fail_free_rwb:
>>>> +	pblk_rwb_free(pblk);
>>>> +fail_free_lines:
>>>> +	pblk_lines_free(pblk);
>>>>  fail_free_core:
>>>>  	pblk_core_free(pblk);
>>>> -fail_free_pad_dist:
>>>> -	kfree(pblk->pad_dist);
>>>> -fail_free_line_meta:
>>>> -	pblk_line_mg_free(pblk);
>>>> -fail_free_luns:
>>>> -	pblk_luns_free(pblk);
>>>>  fail:
>>>>  	kfree(pblk);
>>>>  	return ERR_PTR(ret);
>>> 
>>> Thanks. I'm not able to squash it without conflict. Is it based on for-4.17/core?
>> Yes, it's based on for-4.17/core, but I can see a small conflict due to
>> the patches applied in the middle of these two. How do you want to do
>> it? We can keep them separately, or do a rebase on the current patches.
> 
> This is all a bit of a mess- Can you send a fix I can apply that fixes
> up the 0-day, and then this larger patch can be applied afterwards?
> (e.g., before at after your other patches)

Not really. The problem is that the original refactoring for the bad
block table was a bandage on the init/exit sequence problem. The
init/ext patch is the one that does it properly.

What we can do, since this is only in your tree, is rebase and pull the
bad block patch up and then merge with init/exit. This way we fix the
original problem and the introduced double free is non-existing.

I can do the rebasing this evening and put it in a different branch you
can review.

Javier

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists