linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2] lightnvm: pblk: refactor init/exit sequences
@ 2018-03-01 15:59 Javier González
  2018-03-01 15:59 ` [PATCH] " Javier González
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-01 15:59 UTC (permalink / raw)
  To: mb; +Cc: linux-block, linux-kernel, dan.carpenter, Javier González

# Changes since V1
  - Remove double check for factory initialization

The init/exit sequences have grown in a very bad way. Refactor them to
eliminate dependencies across initialization modules.

One of these dependencies caused a bad double free when introducing a
preparation patch for 2.0 bad block identification. This was reported by
Dan Carpenter and 0-DAY.

Matias,

Since you have not send the PR to Jens, please squash this patch with:
    lightnvm: pblk: refactor bad block identification

You will notice that I had queued this patch on the 2.0 series. I did
the rebase now, which is probably what I should have done from the
beginning. Since I'll be sending a V5 for it, this should not be a
problem.

Thanks,
Javier

Javier González (1):
  lightnvm: pblk: refactor init/exit sequences

 drivers/lightnvm/pblk-init.c | 415 +++++++++++++++++++++----------------------
 1 file changed, 206 insertions(+), 209 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-01 15:59 [PATCH V2] lightnvm: pblk: refactor init/exit sequences Javier González
@ 2018-03-01 15:59 ` Javier González
  2018-03-01 18:49   ` Matias Bjørling
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-01 15:59 UTC (permalink / raw)
  To: mb; +Cc: linux-block, linux-kernel, dan.carpenter, Javier González

Refactor init and exit sequences to eliminate dependencies among init
modules and improve readability.

Signed-off-by: Javier González <javier@cnexlabs.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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-01 15:59 ` [PATCH] " Javier González
@ 2018-03-01 18:49   ` Matias Bjørling
  2018-03-01 19:29     ` Javier González
  0 siblings, 1 reply; 11+ messages in thread
From: Matias Bjørling @ 2018-03-01 18:49 UTC (permalink / raw)
  To: Javier González
  Cc: linux-block, linux-kernel, dan.carpenter, Javier González

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@cnexlabs.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?

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-01 18:49   ` Matias Bjørling
@ 2018-03-01 19:29     ` Javier González
  2018-03-05 13:38       ` Matias Bjørling
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-01 19:29 UTC (permalink / raw)
  To: Matias Bjørling; +Cc: linux-block, linux-kernel, Dan Carpenter

[-- Attachment #1: Type: text/plain, Size: 19990 bytes --]


> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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.

Javier


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-01 19:29     ` Javier González
@ 2018-03-05 13:38       ` Matias Bjørling
  2018-03-05 13:45         ` Javier González
  0 siblings, 1 reply; 11+ messages in thread
From: Matias Bjørling @ 2018-03-05 13:38 UTC (permalink / raw)
  To: Javier González; +Cc: linux-block, linux-kernel, Dan Carpenter

On 03/01/2018 08:29 PM, Javier González wrote:
> 
>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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)

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-05 13:38       ` Matias Bjørling
@ 2018-03-05 13:45         ` Javier González
  2018-03-05 14:16           ` Matias Bjørling
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-05 13:45 UTC (permalink / raw)
  To: Matias Bjørling; +Cc: linux-block, LKML, Dan Carpenter

[-- Attachment #1: Type: text/plain, Size: 22002 bytes --]

> On 5 Mar 2018, at 14.38, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 03/01/2018 08:29 PM, Javier González wrote:
>>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-05 13:45         ` Javier González
@ 2018-03-05 14:16           ` Matias Bjørling
  2018-03-05 14:18             ` Javier González
  0 siblings, 1 reply; 11+ messages in thread
From: Matias Bjørling @ 2018-03-05 14:16 UTC (permalink / raw)
  To: Javier González; +Cc: linux-block, LKML, Dan Carpenter

On 03/05/2018 02:45 PM, Javier González wrote:
>> On 5 Mar 2018, at 14.38, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 03/01/2018 08:29 PM, Javier González wrote:
>>>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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.
> 

No thanks. I fixed it up with this:

diff --git i/drivers/lightnvm/pblk-init.c w/drivers/lightnvm/pblk-init.c
index a2b54a88968b..c31e24aabc18 100644
--- i/drivers/lightnvm/pblk-init.c
+++ w/drivers/lightnvm/pblk-init.c
@@ -838,7 +838,7 @@ static int pblk_lines_init(struct pblk *pblk)
                 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_bb_aux;
         }

         for (i = 0; i < l_mg->nr_lines; i++) {
@@ -882,8 +882,6 @@ static int pblk_lines_init(struct pblk *pblk)
         kfree(chunk_log);
         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:

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-05 14:16           ` Matias Bjørling
@ 2018-03-05 14:18             ` Javier González
  2018-03-05 18:27               ` Matias Bjørling
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-05 14:18 UTC (permalink / raw)
  To: Matias Bjørling; +Cc: linux-block, LKML, Dan Carpenter

[-- Attachment #1: Type: text/plain, Size: 24401 bytes --]

> On 5 Mar 2018, at 15.16, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 03/05/2018 02:45 PM, Javier González wrote:
>>> On 5 Mar 2018, at 14.38, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>> On 03/01/2018 08:29 PM, Javier González wrote:
>>>>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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.
> 
> No thanks. I fixed it up with this:
> 
> diff --git i/drivers/lightnvm/pblk-init.c w/drivers/lightnvm/pblk-init.c
> index a2b54a88968b..c31e24aabc18 100644
> --- i/drivers/lightnvm/pblk-init.c
> +++ w/drivers/lightnvm/pblk-init.c
> @@ -838,7 +838,7 @@ static int pblk_lines_init(struct pblk *pblk)
>                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_bb_aux;
>        }
> 
>        for (i = 0; i < l_mg->nr_lines; i++) {
> @@ -882,8 +882,6 @@ static int pblk_lines_init(struct pblk *pblk)
>        kfree(chunk_log);
>        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:

It's not that simple... There are other double frees due to the way l_mg
is allocated, which is fixed on the other patch.

Javier


[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-05 14:18             ` Javier González
@ 2018-03-05 18:27               ` Matias Bjørling
  0 siblings, 0 replies; 11+ messages in thread
From: Matias Bjørling @ 2018-03-05 18:27 UTC (permalink / raw)
  To: Javier González; +Cc: linux-block, LKML, Dan Carpenter

On 03/05/2018 03:18 PM, Javier González wrote:
>> On 5 Mar 2018, at 15.16, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 03/05/2018 02:45 PM, Javier González wrote:
>>>> On 5 Mar 2018, at 14.38, Matias Bjørling <mb@lightnvm.io> wrote:
>>>>
>>>> On 03/01/2018 08:29 PM, Javier González wrote:
>>>>>> On 1 Mar 2018, at 19.49, Matias Bjørling <mb@lightnvm.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@cnexlabs.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.
>>
>> No thanks. I fixed it up with this:
>>
>> diff --git i/drivers/lightnvm/pblk-init.c w/drivers/lightnvm/pblk-init.c
>> index a2b54a88968b..c31e24aabc18 100644
>> --- i/drivers/lightnvm/pblk-init.c
>> +++ w/drivers/lightnvm/pblk-init.c
>> @@ -838,7 +838,7 @@ static int pblk_lines_init(struct pblk *pblk)
>>                 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_bb_aux;
>>         }
>>
>>         for (i = 0; i < l_mg->nr_lines; i++) {
>> @@ -882,8 +882,6 @@ static int pblk_lines_init(struct pblk *pblk)
>>         kfree(chunk_log);
>>         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:
> 
> It's not that simple... There are other double frees due to the way l_mg
> is allocated, which is fixed on the other patch.
> 

I know that. This was to fix up the original patch. I do not want to 
rebase ack'ed patches.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH] lightnvm: pblk: refactor init/exit sequences
  2018-03-01 15:08 Javier González
@ 2018-03-01 15:08 ` Javier González
  0 siblings, 0 replies; 11+ messages in thread
From: Javier González @ 2018-03-01 15:08 UTC (permalink / raw)
  To: mb; +Cc: linux-block, linux-kernel, dan.carpenter, Javier González

Refactor init and exit sequences to eliminate dependencies among init
modules and improve readability.

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/lightnvm/pblk-init.c | 412 +++++++++++++++++++++----------------------
 1 file changed, 206 insertions(+), 206 deletions(-)

diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 25fc70ca07f7..785020e95a73 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);
 }
@@ -1003,16 +1030,6 @@ static void *pblk_init(struct nvm_tgt_dev *dev, struct gendisk *tdisk,
 	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 +1053,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 +1116,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);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 11+ messages in thread
* [PATCH] lightnvm: pblk: refactor init/exit sequences
@ 2018-03-01 15:08 Javier González
  2018-03-01 15:08 ` Javier González
  0 siblings, 1 reply; 11+ messages in thread
From: Javier González @ 2018-03-01 15:08 UTC (permalink / raw)
  To: mb; +Cc: linux-block, linux-kernel, dan.carpenter, Javier González

The init/exit sequences have grown in a very bad way. Refactor them to
eliminate dependencies across initialization modules.

One of these dependencies caused a bad double free when introducing a
preparation patch for 2.0 bad block identification. This was reported by
Dan Carpenter and 0-DAY.

Matias,

Since you have not send the PR to Jens, please squash this patch with:
    lightnvm: pblk: refactor bad block identification

You will notice that I had queued this patch on the 2.0 series. I did
the rebase now, which is probably what I should have done from the
beginning. Since I'll be sending a V5 for it, this should not be a
problem.

Thanks,
Javier

Javier González (1):
  lightnvm: pblk: refactor init/exit sequences

 drivers/lightnvm/pblk-init.c | 412 +++++++++++++++++++++----------------------
 1 file changed, 206 insertions(+), 206 deletions(-)

-- 
2.7.4

^ permalink raw reply	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-03-05 18:27 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-01 15:59 [PATCH V2] lightnvm: pblk: refactor init/exit sequences Javier González
2018-03-01 15:59 ` [PATCH] " Javier González
2018-03-01 18:49   ` Matias Bjørling
2018-03-01 19:29     ` Javier González
2018-03-05 13:38       ` Matias Bjørling
2018-03-05 13:45         ` Javier González
2018-03-05 14:16           ` Matias Bjørling
2018-03-05 14:18             ` Javier González
2018-03-05 18:27               ` Matias Bjørling
  -- strict thread matches above, loose matches on Subject: below --
2018-03-01 15:08 Javier González
2018-03-01 15:08 ` Javier González

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).