[PATCH v3 00/10] lightnvm: next set of improvements for 5.2

[PATCH v3 00/10] lightnvm: next set of improvements for 5.2

[Igor Konopko]

This is another set of fixes and improvements to both pblk and lightnvm
core. 

First & second patches are the most crutial, since they changes the approach
to the partial read path, so detailed review is needed especially here.

Other patches are my other findings related to some bugs or potential
improvements, mostly related to some corner cases.

Changes v2 -> v3;
-dropped some not needed patches
-dropped patches which were already pulled into for-5.2/core branch
-commit messages cleanup

Changes v1 -> v2:
-dropped some not needed patches
-review feedback incorporated for some of the patches
-partial read path changes patch splitted into two patches

Igor Konopko (10):
  lightnvm: pblk: IO path reorganization
  lightnvm: pblk: simplify partial read path
  lightnvm: pblk: propagate errors when reading meta
  lightnvm: pblk: recover only written metadata
  lightnvm: pblk: wait for inflight IOs in recovery
  lightnvm: pblk: remove internal IO timeout
  lightnvm: pblk: store multiple copies of smeta
  lightnvm: pblk: GC error handling
  lightnvm: pblk: use nvm_rq_to_ppa_list()
  lightnvm: track inflight target creations

 drivers/lightnvm/core.c          |  19 ++-
 drivers/lightnvm/pblk-cache.c    |   8 +-
 drivers/lightnvm/pblk-core.c     | 179 +++++++++++++++++----
 drivers/lightnvm/pblk-gc.c       |   5 +-
 drivers/lightnvm/pblk-init.c     |  37 +++--
 drivers/lightnvm/pblk-read.c     | 336 +++++++++++----------------------------
 drivers/lightnvm/pblk-recovery.c |  69 ++++----
 drivers/lightnvm/pblk-rl.c       |   3 +-
 drivers/lightnvm/pblk.h          |  31 ++--
 include/linux/lightnvm.h         |   2 +
 10 files changed, 343 insertions(+), 346 deletions(-)

-- 
2.9.5

[PATCH v3 01/10] lightnvm: pblk: IO path reorganization

[Igor Konopko]

This patch is made in order to prepare read path for new approach to
partial read handling (which is needed for multi-page bvec handling)
The most important change is to move the handling of completed and
failed bio from the pblk_make_rq() to particular read and write
functions. This is needed, since after partial read path changes,
sometimes completed/failed bio will be different from original one, so
we cannot do this any longer in pblk_make_rq(). Other changes are
small read path refactor in order to reduce the size of another patch
with partial read changes. Generally the goal of this patch is not to
change the functionality, but just to prepare the code for the
following changes.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/pblk-cache.c |  8 +++--
 drivers/lightnvm/pblk-init.c  | 14 ++------
 drivers/lightnvm/pblk-read.c  | 83 ++++++++++++++++++++-----------------------
 drivers/lightnvm/pblk.h       |  4 +--
 4 files changed, 48 insertions(+), 61 deletions(-)

diff --git a/drivers/lightnvm/pblk-cache.c b/drivers/lightnvm/pblk-cache.c
index c9fa26f..5c1034c 100644
--- a/drivers/lightnvm/pblk-cache.c
+++ b/drivers/lightnvm/pblk-cache.c
@@ -18,7 +18,8 @@
 
 #include "pblk.h"
 
-int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
+void pblk_write_to_cache(struct pblk *pblk, struct bio *bio,
+				unsigned long flags)
 {
 	struct request_queue *q = pblk->dev->q;
 	struct pblk_w_ctx w_ctx;
@@ -43,6 +44,7 @@ int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
 		goto retry;
 	case NVM_IO_ERR:
 		pblk_pipeline_stop(pblk);
+		bio_io_error(bio);
 		goto out;
 	}
 
@@ -79,7 +81,9 @@ int pblk_write_to_cache(struct pblk *pblk, struct bio *bio, unsigned long flags)
 out:
 	generic_end_io_acct(q, REQ_OP_WRITE, &pblk->disk->part0, start_time);
 	pblk_write_should_kick(pblk);
-	return ret;
+
+	if (ret == NVM_IO_DONE)
+		bio_endio(bio);
 }
 
 /*
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index 1e227a0..b351c7f 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -50,7 +50,6 @@ struct bio_set pblk_bio_set;
 static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
 {
 	struct pblk *pblk = q->queuedata;
-	int ret;
 
 	if (bio_op(bio) == REQ_OP_DISCARD) {
 		pblk_discard(pblk, bio);
@@ -65,7 +64,7 @@ static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
 	 */
 	if (bio_data_dir(bio) == READ) {
 		blk_queue_split(q, &bio);
-		ret = pblk_submit_read(pblk, bio);
+		pblk_submit_read(pblk, bio);
 	} else {
 		/* Prevent deadlock in the case of a modest LUN configuration
 		 * and large user I/Os. Unless stalled, the rate limiter
@@ -74,16 +73,7 @@ static blk_qc_t pblk_make_rq(struct request_queue *q, struct bio *bio)
 		if (pblk_get_secs(bio) > pblk_rl_max_io(&pblk->rl))
 			blk_queue_split(q, &bio);
 
-		ret = pblk_write_to_cache(pblk, bio, PBLK_IOTYPE_USER);
-	}
-
-	switch (ret) {
-	case NVM_IO_ERR:
-		bio_io_error(bio);
-		break;
-	case NVM_IO_DONE:
-		bio_endio(bio);
-		break;
+		pblk_write_to_cache(pblk, bio, PBLK_IOTYPE_USER);
 	}
 
 	return BLK_QC_T_NONE;
diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 6569746..597fe6d 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -179,7 +179,8 @@ static void pblk_end_user_read(struct bio *bio, int error)
 {
 	if (error && error != NVM_RSP_WARN_HIGHECC)
 		bio_io_error(bio);
-	bio_endio(bio);
+	else
+		bio_endio(bio);
 }
 
 static void __pblk_end_io_read(struct pblk *pblk, struct nvm_rq *rqd,
@@ -383,7 +384,6 @@ static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
 
 	/* Free allocated pages in new bio */
 	pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
-	__pblk_end_io_read(pblk, rqd, false);
 	return NVM_IO_ERR;
 }
 
@@ -428,7 +428,7 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
 	}
 }
 
-int pblk_submit_read(struct pblk *pblk, struct bio *bio)
+void pblk_submit_read(struct pblk *pblk, struct bio *bio)
 {
 	struct nvm_tgt_dev *dev = pblk->dev;
 	struct request_queue *q = dev->q;
@@ -436,9 +436,9 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
 	unsigned int nr_secs = pblk_get_secs(bio);
 	struct pblk_g_ctx *r_ctx;
 	struct nvm_rq *rqd;
+	struct bio *int_bio;
 	unsigned int bio_init_idx;
 	DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
-	int ret = NVM_IO_ERR;
 
 	generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
 			      &pblk->disk->part0);
@@ -449,74 +449,67 @@ int pblk_submit_read(struct pblk *pblk, struct bio *bio)
 
 	rqd->opcode = NVM_OP_PREAD;
 	rqd->nr_ppas = nr_secs;
-	rqd->bio = NULL; /* cloned bio if needed */
 	rqd->private = pblk;
 	rqd->end_io = pblk_end_io_read;
 
 	r_ctx = nvm_rq_to_pdu(rqd);
 	r_ctx->start_time = jiffies;
 	r_ctx->lba = blba;
-	r_ctx->private = bio; /* original bio */
 
 	/* Save the index for this bio's start. This is needed in case
 	 * we need to fill a partial read.
 	 */
 	bio_init_idx = pblk_get_bi_idx(bio);
 
-	if (pblk_alloc_rqd_meta(pblk, rqd))
-		goto fail_rqd_free;
+	if (pblk_alloc_rqd_meta(pblk, rqd)) {
+		bio_io_error(bio);
+		pblk_free_rqd(pblk, rqd, PBLK_READ);
+		return;
+	}
+
+	/* Clone read bio to deal internally with:
+	 * -read errors when reading from drive
+	 * -bio_advance() calls during l2p lookup and cache reads
+	 */
+	int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
 
 	if (nr_secs > 1)
 		pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
 	else
 		pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
 
+	r_ctx->private = bio; /* original bio */
+	rqd->bio = int_bio; /* internal bio */
+
 	if (bitmap_full(read_bitmap, nr_secs)) {
+		pblk_end_user_read(bio, 0);
 		atomic_inc(&pblk->inflight_io);
 		__pblk_end_io_read(pblk, rqd, false);
-		return NVM_IO_DONE;
+		return;
 	}
 
-	/* All sectors are to be read from the device */
-	if (bitmap_empty(read_bitmap, rqd->nr_ppas)) {
-		struct bio *int_bio = NULL;
-
-		/* Clone read bio to deal with read errors internally */
-		int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
-		if (!int_bio) {
-			pblk_err(pblk, "could not clone read bio\n");
-			goto fail_end_io;
-		}
-
-		rqd->bio = int_bio;
-
-		if (pblk_submit_io(pblk, rqd)) {
+	if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
+		/* The read bio request could be partially filled by the write
+		 * buffer, but there are some holes that need to be read from
+		 * the drive.
+		 */
+		bio_put(int_bio);
+		rqd->bio = NULL;
+		if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
+					    nr_secs)) {
 			pblk_err(pblk, "read IO submission failed\n");
-			ret = NVM_IO_ERR;
-			goto fail_end_io;
+			bio_io_error(bio);
+			__pblk_end_io_read(pblk, rqd, false);
 		}
-
-		return NVM_IO_OK;
+		return;
 	}
 
-	/* The read bio request could be partially filled by the write buffer,
-	 * but there are some holes that need to be read from the drive.
-	 */
-	ret = pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
-				    nr_secs);
-	if (ret)
-		goto fail_meta_free;
-
-	return NVM_IO_OK;
-
-fail_meta_free:
-	nvm_dev_dma_free(dev->parent, rqd->meta_list, rqd->dma_meta_list);
-fail_rqd_free:
-	pblk_free_rqd(pblk, rqd, PBLK_READ);
-	return ret;
-fail_end_io:
-	__pblk_end_io_read(pblk, rqd, false);
-	return ret;
+	/* All sectors are to be read from the device */
+	if (pblk_submit_io(pblk, rqd)) {
+		pblk_err(pblk, "read IO submission failed\n");
+		bio_io_error(bio);
+		__pblk_end_io_read(pblk, rqd, false);
+	}
 }
 
 static int read_ppalist_rq_gc(struct pblk *pblk, struct nvm_rq *rqd,
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 381f074..a3c925d 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -868,7 +868,7 @@ void pblk_get_packed_meta(struct pblk *pblk, struct nvm_rq *rqd);
 /*
  * pblk user I/O write path
  */
-int pblk_write_to_cache(struct pblk *pblk, struct bio *bio,
+void pblk_write_to_cache(struct pblk *pblk, struct bio *bio,
 			unsigned long flags);
 int pblk_write_gc_to_cache(struct pblk *pblk, struct pblk_gc_rq *gc_rq);
 
@@ -894,7 +894,7 @@ void pblk_write_kick(struct pblk *pblk);
  * pblk read path
  */
 extern struct bio_set pblk_bio_set;
-int pblk_submit_read(struct pblk *pblk, struct bio *bio);
+void pblk_submit_read(struct pblk *pblk, struct bio *bio);
 int pblk_submit_read_gc(struct pblk *pblk, struct pblk_gc_rq *gc_rq);
 /*
  * pblk recovery
-- 
2.9.5

[PATCH v3 02/10] lightnvm: pblk: simplify partial read path

[Igor Konopko]

This patch changes the approach to handling partial read path, what is
needed for supporting multi-page bvec, which is currently broken.

In old approach merging of data from round buffer and drive was fully
made by drive. This had some disadvantages - code was complex and
relies on bio internals, so it was hard to maintain and was strongly
dependent on bio changes.

In new approach most of the handling is done mostly by block layer
functions such as bio_split(), bio_chain() and generic_make request()
and generally is less complex and easier to maintain. Below some more
details of the new approach.

When read bio arrives, it is cloned for pblk internal purposes. All
the L2P mapping, which includes copying data from round buffer to bio
and thus bio_advance() calls is done on the cloned bio, so the original
bio is untouched. Later if we found that we have partial read case, we
still have original bio untouched, so we can split it and continue to
process only first 4K of it in current context, when the rest will be
called as separate bio request which is passed to generic_make_request()
for further processing.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-core.c |  16 +++
 drivers/lightnvm/pblk-read.c | 276 +++++++++++--------------------------------
 drivers/lightnvm/pblk.h      |  22 ++--
 3 files changed, 97 insertions(+), 217 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 39280c1..f2edec6 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -1529,6 +1529,22 @@ void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd)
 		pblk_ppa_to_line_put(pblk, ppa_list[i]);
 }
 
+void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
+					int offset)
+{
+	struct ppa_addr ppa;
+	struct pblk_line *line;
+	int i = offset;
+
+	for (; i < rqd->nr_ppas; i++) {
+		ppa = rqd->ppa_list[i];
+		if (!pblk_ppa_empty(ppa) && !pblk_addr_in_cache(ppa)) {
+			line = pblk_ppa_to_line(pblk, ppa);
+			kref_put(&line->ref, pblk_line_put);
+		}
+	}
+}
+
 static void pblk_stop_writes(struct pblk *pblk, struct pblk_line *line)
 {
 	lockdep_assert_held(&pblk->l_mg.free_lock);
diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 597fe6d..264d1d7 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -41,28 +41,30 @@ static int pblk_read_from_cache(struct pblk *pblk, struct bio *bio,
 
 static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
 				 struct bio *bio, sector_t blba,
-				 unsigned long *read_bitmap)
+				 struct pblk_read_info *info)
 {
 	void *meta_list = rqd->meta_list;
-	struct ppa_addr ppas[NVM_MAX_VLBA];
 	int nr_secs = rqd->nr_ppas;
 	bool advanced_bio = false;
-	int i, j = 0;
+	int i;
 
-	pblk_lookup_l2p_seq(pblk, ppas, blba, nr_secs);
+	pblk_lookup_l2p_seq(pblk, rqd->ppa_list, blba, nr_secs);
+	info->first_sec_from_cache = nr_secs;
+	info->first_sec_from_drive = nr_secs;
 
 	for (i = 0; i < nr_secs; i++) {
-		struct ppa_addr p = ppas[i];
 		struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
 		sector_t lba = blba + i;
 
 retry:
-		if (pblk_ppa_empty(p)) {
+		if (pblk_ppa_empty(rqd->ppa_list[i])) {
 			__le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
 
-			WARN_ON(test_and_set_bit(i, read_bitmap));
-			meta->lba = addr_empty;
+			info->secs_from_cache = true;
+			info->first_sec_from_cache = min_t(int, i,
+						info->first_sec_from_cache);
 
+			meta->lba = addr_empty;
 			if (unlikely(!advanced_bio)) {
 				bio_advance(bio, (i) * PBLK_EXPOSED_PAGE_SIZE);
 				advanced_bio = true;
@@ -74,13 +76,18 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
 		/* Try to read from write buffer. The address is later checked
 		 * on the write buffer to prevent retrieving overwritten data.
 		 */
-		if (pblk_addr_in_cache(p)) {
-			if (!pblk_read_from_cache(pblk, bio, lba, p, i,
-								advanced_bio)) {
-				pblk_lookup_l2p_seq(pblk, &p, lba, 1);
+		if (pblk_addr_in_cache(rqd->ppa_list[i])) {
+			if (!pblk_read_from_cache(pblk, bio, lba,
+					rqd->ppa_list[i], i, advanced_bio)) {
+				pblk_lookup_l2p_seq(pblk, &rqd->ppa_list[i],
+							lba, 1);
 				goto retry;
 			}
-			WARN_ON(test_and_set_bit(i, read_bitmap));
+
+			info->secs_from_cache = true;
+			info->first_sec_from_cache = min_t(int, i,
+						info->first_sec_from_cache);
+
 			meta->lba = cpu_to_le64(lba);
 			advanced_bio = true;
 #ifdef CONFIG_NVM_PBLK_DEBUG
@@ -88,7 +95,9 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
 #endif
 		} else {
 			/* Read from media non-cached sectors */
-			rqd->ppa_list[j++] = p;
+			info->secs_from_drive = true;
+			info->first_sec_from_drive = min_t(int, i,
+						info->first_sec_from_drive);
 		}
 
 next:
@@ -223,172 +232,8 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
 	__pblk_end_io_read(pblk, rqd, true);
 }
 
-static void pblk_end_partial_read(struct nvm_rq *rqd)
-{
-	struct pblk *pblk = rqd->private;
-	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
-	struct pblk_pr_ctx *pr_ctx = r_ctx->private;
-	struct pblk_sec_meta *meta;
-	struct bio *new_bio = rqd->bio;
-	struct bio *bio = pr_ctx->orig_bio;
-	struct bio_vec src_bv, dst_bv;
-	void *meta_list = rqd->meta_list;
-	int bio_init_idx = pr_ctx->bio_init_idx;
-	unsigned long *read_bitmap = pr_ctx->bitmap;
-	int nr_secs = pr_ctx->orig_nr_secs;
-	int nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
-	void *src_p, *dst_p;
-	int hole, i;
-
-	if (unlikely(nr_holes == 1)) {
-		struct ppa_addr ppa;
-
-		ppa = rqd->ppa_addr;
-		rqd->ppa_list = pr_ctx->ppa_ptr;
-		rqd->dma_ppa_list = pr_ctx->dma_ppa_list;
-		rqd->ppa_list[0] = ppa;
-	}
-
-	for (i = 0; i < nr_secs; i++) {
-		meta = pblk_get_meta(pblk, meta_list, i);
-		pr_ctx->lba_list_media[i] = le64_to_cpu(meta->lba);
-		meta->lba = cpu_to_le64(pr_ctx->lba_list_mem[i]);
-	}
-
-	/* Fill the holes in the original bio */
-	i = 0;
-	hole = find_first_zero_bit(read_bitmap, nr_secs);
-	do {
-		struct pblk_line *line;
-
-		line = pblk_ppa_to_line(pblk, rqd->ppa_list[i]);
-		kref_put(&line->ref, pblk_line_put);
-
-		meta = pblk_get_meta(pblk, meta_list, hole);
-		meta->lba = cpu_to_le64(pr_ctx->lba_list_media[i]);
-
-		src_bv = new_bio->bi_io_vec[i++];
-		dst_bv = bio->bi_io_vec[bio_init_idx + hole];
-
-		src_p = kmap_atomic(src_bv.bv_page);
-		dst_p = kmap_atomic(dst_bv.bv_page);
-
-		memcpy(dst_p + dst_bv.bv_offset,
-			src_p + src_bv.bv_offset,
-			PBLK_EXPOSED_PAGE_SIZE);
-
-		kunmap_atomic(src_p);
-		kunmap_atomic(dst_p);
-
-		mempool_free(src_bv.bv_page, &pblk->page_bio_pool);
-
-		hole = find_next_zero_bit(read_bitmap, nr_secs, hole + 1);
-	} while (hole < nr_secs);
-
-	bio_put(new_bio);
-	kfree(pr_ctx);
-
-	/* restore original request */
-	rqd->bio = NULL;
-	rqd->nr_ppas = nr_secs;
-
-	pblk_end_user_read(bio, rqd->error);
-	__pblk_end_io_read(pblk, rqd, false);
-}
-
-static int pblk_setup_partial_read(struct pblk *pblk, struct nvm_rq *rqd,
-			    unsigned int bio_init_idx,
-			    unsigned long *read_bitmap,
-			    int nr_holes)
-{
-	void *meta_list = rqd->meta_list;
-	struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
-	struct pblk_pr_ctx *pr_ctx;
-	struct bio *new_bio, *bio = r_ctx->private;
-	int nr_secs = rqd->nr_ppas;
-	int i;
-
-	new_bio = bio_alloc(GFP_KERNEL, nr_holes);
-
-	if (pblk_bio_add_pages(pblk, new_bio, GFP_KERNEL, nr_holes))
-		goto fail_bio_put;
-
-	if (nr_holes != new_bio->bi_vcnt) {
-		WARN_ONCE(1, "pblk: malformed bio\n");
-		goto fail_free_pages;
-	}
-
-	pr_ctx = kzalloc(sizeof(struct pblk_pr_ctx), GFP_KERNEL);
-	if (!pr_ctx)
-		goto fail_free_pages;
-
-	for (i = 0; i < nr_secs; i++) {
-		struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
-
-		pr_ctx->lba_list_mem[i] = le64_to_cpu(meta->lba);
-	}
-
-	new_bio->bi_iter.bi_sector = 0; /* internal bio */
-	bio_set_op_attrs(new_bio, REQ_OP_READ, 0);
-
-	rqd->bio = new_bio;
-	rqd->nr_ppas = nr_holes;
-
-	pr_ctx->orig_bio = bio;
-	bitmap_copy(pr_ctx->bitmap, read_bitmap, NVM_MAX_VLBA);
-	pr_ctx->bio_init_idx = bio_init_idx;
-	pr_ctx->orig_nr_secs = nr_secs;
-	r_ctx->private = pr_ctx;
-
-	if (unlikely(nr_holes == 1)) {
-		pr_ctx->ppa_ptr = rqd->ppa_list;
-		pr_ctx->dma_ppa_list = rqd->dma_ppa_list;
-		rqd->ppa_addr = rqd->ppa_list[0];
-	}
-	return 0;
-
-fail_free_pages:
-	pblk_bio_free_pages(pblk, new_bio, 0, new_bio->bi_vcnt);
-fail_bio_put:
-	bio_put(new_bio);
-
-	return -ENOMEM;
-}
-
-static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
-				 unsigned int bio_init_idx,
-				 unsigned long *read_bitmap, int nr_secs)
-{
-	int nr_holes;
-	int ret;
-
-	nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
-
-	if (pblk_setup_partial_read(pblk, rqd, bio_init_idx, read_bitmap,
-				    nr_holes))
-		return NVM_IO_ERR;
-
-	rqd->end_io = pblk_end_partial_read;
-
-	ret = pblk_submit_io(pblk, rqd);
-	if (ret) {
-		bio_put(rqd->bio);
-		pblk_err(pblk, "partial read IO submission failed\n");
-		goto err;
-	}
-
-	return NVM_IO_OK;
-
-err:
-	pblk_err(pblk, "failed to perform partial read\n");
-
-	/* Free allocated pages in new bio */
-	pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
-	return NVM_IO_ERR;
-}
-
 static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
-			 sector_t lba, unsigned long *read_bitmap)
+			 sector_t lba, struct pblk_read_info *info)
 {
 	struct pblk_sec_meta *meta = pblk_get_meta(pblk, rqd->meta_list, 0);
 	struct ppa_addr ppa;
@@ -403,8 +248,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
 	if (pblk_ppa_empty(ppa)) {
 		__le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
 
-		WARN_ON(test_and_set_bit(0, read_bitmap));
 		meta->lba = addr_empty;
+		info->secs_from_cache = true;
 		return;
 	}
 
@@ -417,14 +262,14 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
 			goto retry;
 		}
 
-		WARN_ON(test_and_set_bit(0, read_bitmap));
 		meta->lba = cpu_to_le64(lba);
-
+		info->secs_from_cache = true;
 #ifdef CONFIG_NVM_PBLK_DEBUG
 		atomic_long_inc(&pblk->cache_reads);
 #endif
 	} else {
 		rqd->ppa_addr = ppa;
+		info->secs_from_drive = true;
 	}
 }
 
@@ -436,15 +281,12 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
 	unsigned int nr_secs = pblk_get_secs(bio);
 	struct pblk_g_ctx *r_ctx;
 	struct nvm_rq *rqd;
-	struct bio *int_bio;
-	unsigned int bio_init_idx;
-	DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
+	struct bio *int_bio, *split_bio;
+	struct pblk_read_info info = {0};
 
 	generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
 			      &pblk->disk->part0);
 
-	bitmap_zero(read_bitmap, nr_secs);
-
 	rqd = pblk_alloc_rqd(pblk, PBLK_READ);
 
 	rqd->opcode = NVM_OP_PREAD;
@@ -456,11 +298,6 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
 	r_ctx->start_time = jiffies;
 	r_ctx->lba = blba;
 
-	/* Save the index for this bio's start. This is needed in case
-	 * we need to fill a partial read.
-	 */
-	bio_init_idx = pblk_get_bi_idx(bio);
-
 	if (pblk_alloc_rqd_meta(pblk, rqd)) {
 		bio_io_error(bio);
 		pblk_free_rqd(pblk, rqd, PBLK_READ);
@@ -474,34 +311,63 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
 	int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
 
 	if (nr_secs > 1)
-		pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
+		pblk_read_ppalist_rq(pblk, rqd, int_bio, blba, &info);
 	else
-		pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
+		pblk_read_rq(pblk, rqd, int_bio, blba, &info);
 
+split_retry:
 	r_ctx->private = bio; /* original bio */
 	rqd->bio = int_bio; /* internal bio */
 
-	if (bitmap_full(read_bitmap, nr_secs)) {
+	if (info.secs_from_cache && !info.secs_from_drive) {
 		pblk_end_user_read(bio, 0);
 		atomic_inc(&pblk->inflight_io);
 		__pblk_end_io_read(pblk, rqd, false);
 		return;
 	}
 
-	if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
+	if (info.secs_from_cache && info.secs_from_drive) {
 		/* The read bio request could be partially filled by the write
 		 * buffer, but there are some holes that need to be read from
-		 * the drive.
+		 * the drive. In order to handle this, we will use block layer
+		 * mechanism to split this request in to smaller ones and make
+		 * a chain of it.
+		 */
+		int split = max_t(int, info.first_sec_from_cache,
+					info.first_sec_from_drive);
+
+		split_bio = bio_split(bio, split * NR_PHY_IN_LOG, GFP_KERNEL,
+					&pblk_bio_set);
+		bio_chain(split_bio, bio);
+		generic_make_request(bio);
+
+		/* New bio contains first N sectors of the previous one, so
+		 * we can continue to use existing rqd, but we need to shrink
+		 * the number of PPAs in it. We also need to release line
+		 * references on the rest of the PPAs. Finally we also need
+		 * to update pblk_read_info struct properly in order to avoid
+		 * another interation of l2p lookup.
+		 */
+		bio = split_bio;
+		pblk_rq_to_line_partial_put(pblk, rqd, split);
+#ifdef CONFIG_NVM_PBLK_DEBUG
+		atomic_long_sub((rqd->nr_ppas - split), &pblk->inflight_reads);
+#endif
+		rqd->nr_ppas = split;
+		if (rqd->nr_ppas == 1)
+			rqd->ppa_addr = rqd->ppa_list[0];
+
+		if (info.first_sec_from_cache > info.first_sec_from_drive)
+			info.secs_from_cache = false;
+		else
+			info.secs_from_drive = false;
+
+		/* Recreate int_bio - existing might have some needed internal
+		 * fields modified already.
 		 */
 		bio_put(int_bio);
-		rqd->bio = NULL;
-		if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
-					    nr_secs)) {
-			pblk_err(pblk, "read IO submission failed\n");
-			bio_io_error(bio);
-			__pblk_end_io_read(pblk, rqd, false);
-		}
-		return;
+		int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
+		goto split_retry;
 	}
 
 	/* All sectors are to be read from the device */
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index a3c925d..e284a93 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -123,18 +123,6 @@ struct pblk_g_ctx {
 	u64 lba;
 };
 
-/* partial read context */
-struct pblk_pr_ctx {
-	struct bio *orig_bio;
-	DECLARE_BITMAP(bitmap, NVM_MAX_VLBA);
-	unsigned int orig_nr_secs;
-	unsigned int bio_init_idx;
-	void *ppa_ptr;
-	dma_addr_t dma_ppa_list;
-	u64 lba_list_mem[NVM_MAX_VLBA];
-	u64 lba_list_media[NVM_MAX_VLBA];
-};
-
 /* Pad context */
 struct pblk_pad_rq {
 	struct pblk *pblk;
@@ -726,6 +714,14 @@ struct pblk_line_ws {
 	struct work_struct ws;
 };
 
+/* L2P lookup on read path info */
+struct pblk_read_info {
+	int first_sec_from_cache;
+	int first_sec_from_drive;
+	bool secs_from_cache;
+	bool secs_from_drive;
+};
+
 #define pblk_g_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_g_ctx))
 #define pblk_w_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_c_ctx))
 
@@ -809,6 +805,8 @@ struct pblk_line *pblk_line_get_first_data(struct pblk *pblk);
 struct pblk_line *pblk_line_replace_data(struct pblk *pblk);
 void pblk_ppa_to_line_put(struct pblk *pblk, struct ppa_addr ppa);
 void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd);
+void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
+					int offset);
 int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line);
 void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line);
 struct pblk_line *pblk_line_get_data(struct pblk *pblk);
-- 
2.9.5

Re: [PATCH v3 02/10] lightnvm: pblk: simplify partial read path

[Hans Holmberg]

On Wed, Mar 27, 2019 at 1:41 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> This patch changes the approach to handling partial read path, what is
> needed for supporting multi-page bvec, which is currently broken.
>
> In old approach merging of data from round buffer and drive was fully
> made by drive. This had some disadvantages - code was complex and
> relies on bio internals, so it was hard to maintain and was strongly
> dependent on bio changes.
>
> In new approach most of the handling is done mostly by block layer
> functions such as bio_split(), bio_chain() and generic_make request()
> and generally is less complex and easier to maintain. Below some more
> details of the new approach.
>
> When read bio arrives, it is cloned for pblk internal purposes. All
> the L2P mapping, which includes copying data from round buffer to bio
> and thus bio_advance() calls is done on the cloned bio, so the original
> bio is untouched. Later if we found that we have partial read case, we
> still have original bio untouched, so we can split it and continue to
> process only first 4K of it in current context, when the rest will be
> called as separate bio request which is passed to generic_make_request()
> for further processing.
>
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>

I've played around with fio to figure out if there is a read latency
performance hit, and I have not found any diff that can't be written
off as noise.
(i did seq write + random 4k reads with different sizes and checked
the latency stats)

> ---
>  drivers/lightnvm/pblk-core.c |  16 +++
>  drivers/lightnvm/pblk-read.c | 276 +++++++++++--------------------------------
>  drivers/lightnvm/pblk.h      |  22 ++--
>  3 files changed, 97 insertions(+), 217 deletions(-)

Nice cleanup :)
I hope to find time early next week to go through the patch in depth.

Thanks,
Hans

>
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index 39280c1..f2edec6 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -1529,6 +1529,22 @@ void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd)
>                 pblk_ppa_to_line_put(pblk, ppa_list[i]);
>  }
>
> +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
> +                                       int offset)
> +{
> +       struct ppa_addr ppa;
> +       struct pblk_line *line;
> +       int i = offset;
> +
> +       for (; i < rqd->nr_ppas; i++) {
> +               ppa = rqd->ppa_list[i];
> +               if (!pblk_ppa_empty(ppa) && !pblk_addr_in_cache(ppa)) {
> +                       line = pblk_ppa_to_line(pblk, ppa);
> +                       kref_put(&line->ref, pblk_line_put);
> +               }
> +       }
> +}
> +
>  static void pblk_stop_writes(struct pblk *pblk, struct pblk_line *line)
>  {
>         lockdep_assert_held(&pblk->l_mg.free_lock);
> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> index 597fe6d..264d1d7 100644
> --- a/drivers/lightnvm/pblk-read.c
> +++ b/drivers/lightnvm/pblk-read.c
> @@ -41,28 +41,30 @@ static int pblk_read_from_cache(struct pblk *pblk, struct bio *bio,
>
>  static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>                                  struct bio *bio, sector_t blba,
> -                                unsigned long *read_bitmap)
> +                                struct pblk_read_info *info)
>  {
>         void *meta_list = rqd->meta_list;
> -       struct ppa_addr ppas[NVM_MAX_VLBA];
>         int nr_secs = rqd->nr_ppas;
>         bool advanced_bio = false;
> -       int i, j = 0;
> +       int i;
>
> -       pblk_lookup_l2p_seq(pblk, ppas, blba, nr_secs);
> +       pblk_lookup_l2p_seq(pblk, rqd->ppa_list, blba, nr_secs);
> +       info->first_sec_from_cache = nr_secs;
> +       info->first_sec_from_drive = nr_secs;
>
>         for (i = 0; i < nr_secs; i++) {
> -               struct ppa_addr p = ppas[i];
>                 struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
>                 sector_t lba = blba + i;
>
>  retry:
> -               if (pblk_ppa_empty(p)) {
> +               if (pblk_ppa_empty(rqd->ppa_list[i])) {
>                         __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
>
> -                       WARN_ON(test_and_set_bit(i, read_bitmap));
> -                       meta->lba = addr_empty;
> +                       info->secs_from_cache = true;
> +                       info->first_sec_from_cache = min_t(int, i,
> +                                               info->first_sec_from_cache);
>
> +                       meta->lba = addr_empty;
>                         if (unlikely(!advanced_bio)) {
>                                 bio_advance(bio, (i) * PBLK_EXPOSED_PAGE_SIZE);
>                                 advanced_bio = true;
> @@ -74,13 +76,18 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>                 /* Try to read from write buffer. The address is later checked
>                  * on the write buffer to prevent retrieving overwritten data.
>                  */
> -               if (pblk_addr_in_cache(p)) {
> -                       if (!pblk_read_from_cache(pblk, bio, lba, p, i,
> -                                                               advanced_bio)) {
> -                               pblk_lookup_l2p_seq(pblk, &p, lba, 1);
> +               if (pblk_addr_in_cache(rqd->ppa_list[i])) {
> +                       if (!pblk_read_from_cache(pblk, bio, lba,
> +                                       rqd->ppa_list[i], i, advanced_bio)) {
> +                               pblk_lookup_l2p_seq(pblk, &rqd->ppa_list[i],
> +                                                       lba, 1);
>                                 goto retry;
>                         }
> -                       WARN_ON(test_and_set_bit(i, read_bitmap));
> +
> +                       info->secs_from_cache = true;
> +                       info->first_sec_from_cache = min_t(int, i,
> +                                               info->first_sec_from_cache);
> +
>                         meta->lba = cpu_to_le64(lba);
>                         advanced_bio = true;
>  #ifdef CONFIG_NVM_PBLK_DEBUG
> @@ -88,7 +95,9 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>  #endif
>                 } else {
>                         /* Read from media non-cached sectors */
> -                       rqd->ppa_list[j++] = p;
> +                       info->secs_from_drive = true;
> +                       info->first_sec_from_drive = min_t(int, i,
> +                                               info->first_sec_from_drive);
>                 }
>
>  next:
> @@ -223,172 +232,8 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
>         __pblk_end_io_read(pblk, rqd, true);
>  }
>
> -static void pblk_end_partial_read(struct nvm_rq *rqd)
> -{
> -       struct pblk *pblk = rqd->private;
> -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
> -       struct pblk_pr_ctx *pr_ctx = r_ctx->private;
> -       struct pblk_sec_meta *meta;
> -       struct bio *new_bio = rqd->bio;
> -       struct bio *bio = pr_ctx->orig_bio;
> -       struct bio_vec src_bv, dst_bv;
> -       void *meta_list = rqd->meta_list;
> -       int bio_init_idx = pr_ctx->bio_init_idx;
> -       unsigned long *read_bitmap = pr_ctx->bitmap;
> -       int nr_secs = pr_ctx->orig_nr_secs;
> -       int nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
> -       void *src_p, *dst_p;
> -       int hole, i;
> -
> -       if (unlikely(nr_holes == 1)) {
> -               struct ppa_addr ppa;
> -
> -               ppa = rqd->ppa_addr;
> -               rqd->ppa_list = pr_ctx->ppa_ptr;
> -               rqd->dma_ppa_list = pr_ctx->dma_ppa_list;
> -               rqd->ppa_list[0] = ppa;
> -       }
> -
> -       for (i = 0; i < nr_secs; i++) {
> -               meta = pblk_get_meta(pblk, meta_list, i);
> -               pr_ctx->lba_list_media[i] = le64_to_cpu(meta->lba);
> -               meta->lba = cpu_to_le64(pr_ctx->lba_list_mem[i]);
> -       }
> -
> -       /* Fill the holes in the original bio */
> -       i = 0;
> -       hole = find_first_zero_bit(read_bitmap, nr_secs);
> -       do {
> -               struct pblk_line *line;
> -
> -               line = pblk_ppa_to_line(pblk, rqd->ppa_list[i]);
> -               kref_put(&line->ref, pblk_line_put);
> -
> -               meta = pblk_get_meta(pblk, meta_list, hole);
> -               meta->lba = cpu_to_le64(pr_ctx->lba_list_media[i]);
> -
> -               src_bv = new_bio->bi_io_vec[i++];
> -               dst_bv = bio->bi_io_vec[bio_init_idx + hole];
> -
> -               src_p = kmap_atomic(src_bv.bv_page);
> -               dst_p = kmap_atomic(dst_bv.bv_page);
> -
> -               memcpy(dst_p + dst_bv.bv_offset,
> -                       src_p + src_bv.bv_offset,
> -                       PBLK_EXPOSED_PAGE_SIZE);
> -
> -               kunmap_atomic(src_p);
> -               kunmap_atomic(dst_p);
> -
> -               mempool_free(src_bv.bv_page, &pblk->page_bio_pool);
> -
> -               hole = find_next_zero_bit(read_bitmap, nr_secs, hole + 1);
> -       } while (hole < nr_secs);
> -
> -       bio_put(new_bio);
> -       kfree(pr_ctx);
> -
> -       /* restore original request */
> -       rqd->bio = NULL;
> -       rqd->nr_ppas = nr_secs;
> -
> -       pblk_end_user_read(bio, rqd->error);
> -       __pblk_end_io_read(pblk, rqd, false);
> -}
> -
> -static int pblk_setup_partial_read(struct pblk *pblk, struct nvm_rq *rqd,
> -                           unsigned int bio_init_idx,
> -                           unsigned long *read_bitmap,
> -                           int nr_holes)
> -{
> -       void *meta_list = rqd->meta_list;
> -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
> -       struct pblk_pr_ctx *pr_ctx;
> -       struct bio *new_bio, *bio = r_ctx->private;
> -       int nr_secs = rqd->nr_ppas;
> -       int i;
> -
> -       new_bio = bio_alloc(GFP_KERNEL, nr_holes);
> -
> -       if (pblk_bio_add_pages(pblk, new_bio, GFP_KERNEL, nr_holes))
> -               goto fail_bio_put;
> -
> -       if (nr_holes != new_bio->bi_vcnt) {
> -               WARN_ONCE(1, "pblk: malformed bio\n");
> -               goto fail_free_pages;
> -       }
> -
> -       pr_ctx = kzalloc(sizeof(struct pblk_pr_ctx), GFP_KERNEL);
> -       if (!pr_ctx)
> -               goto fail_free_pages;
> -
> -       for (i = 0; i < nr_secs; i++) {
> -               struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
> -
> -               pr_ctx->lba_list_mem[i] = le64_to_cpu(meta->lba);
> -       }
> -
> -       new_bio->bi_iter.bi_sector = 0; /* internal bio */
> -       bio_set_op_attrs(new_bio, REQ_OP_READ, 0);
> -
> -       rqd->bio = new_bio;
> -       rqd->nr_ppas = nr_holes;
> -
> -       pr_ctx->orig_bio = bio;
> -       bitmap_copy(pr_ctx->bitmap, read_bitmap, NVM_MAX_VLBA);
> -       pr_ctx->bio_init_idx = bio_init_idx;
> -       pr_ctx->orig_nr_secs = nr_secs;
> -       r_ctx->private = pr_ctx;
> -
> -       if (unlikely(nr_holes == 1)) {
> -               pr_ctx->ppa_ptr = rqd->ppa_list;
> -               pr_ctx->dma_ppa_list = rqd->dma_ppa_list;
> -               rqd->ppa_addr = rqd->ppa_list[0];
> -       }
> -       return 0;
> -
> -fail_free_pages:
> -       pblk_bio_free_pages(pblk, new_bio, 0, new_bio->bi_vcnt);
> -fail_bio_put:
> -       bio_put(new_bio);
> -
> -       return -ENOMEM;
> -}
> -
> -static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
> -                                unsigned int bio_init_idx,
> -                                unsigned long *read_bitmap, int nr_secs)
> -{
> -       int nr_holes;
> -       int ret;
> -
> -       nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
> -
> -       if (pblk_setup_partial_read(pblk, rqd, bio_init_idx, read_bitmap,
> -                                   nr_holes))
> -               return NVM_IO_ERR;
> -
> -       rqd->end_io = pblk_end_partial_read;
> -
> -       ret = pblk_submit_io(pblk, rqd);
> -       if (ret) {
> -               bio_put(rqd->bio);
> -               pblk_err(pblk, "partial read IO submission failed\n");
> -               goto err;
> -       }
> -
> -       return NVM_IO_OK;
> -
> -err:
> -       pblk_err(pblk, "failed to perform partial read\n");
> -
> -       /* Free allocated pages in new bio */
> -       pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
> -       return NVM_IO_ERR;
> -}
> -
>  static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
> -                        sector_t lba, unsigned long *read_bitmap)
> +                        sector_t lba, struct pblk_read_info *info)
>  {
>         struct pblk_sec_meta *meta = pblk_get_meta(pblk, rqd->meta_list, 0);
>         struct ppa_addr ppa;
> @@ -403,8 +248,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>         if (pblk_ppa_empty(ppa)) {
>                 __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
>
> -               WARN_ON(test_and_set_bit(0, read_bitmap));
>                 meta->lba = addr_empty;
> +               info->secs_from_cache = true;
>                 return;
>         }
>
> @@ -417,14 +262,14 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>                         goto retry;
>                 }
>
> -               WARN_ON(test_and_set_bit(0, read_bitmap));
>                 meta->lba = cpu_to_le64(lba);
> -
> +               info->secs_from_cache = true;
>  #ifdef CONFIG_NVM_PBLK_DEBUG
>                 atomic_long_inc(&pblk->cache_reads);
>  #endif
>         } else {
>                 rqd->ppa_addr = ppa;
> +               info->secs_from_drive = true;
>         }
>  }
>
> @@ -436,15 +281,12 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>         unsigned int nr_secs = pblk_get_secs(bio);
>         struct pblk_g_ctx *r_ctx;
>         struct nvm_rq *rqd;
> -       struct bio *int_bio;
> -       unsigned int bio_init_idx;
> -       DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
> +       struct bio *int_bio, *split_bio;
> +       struct pblk_read_info info = {0};
>
>         generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
>                               &pblk->disk->part0);
>
> -       bitmap_zero(read_bitmap, nr_secs);
> -
>         rqd = pblk_alloc_rqd(pblk, PBLK_READ);
>
>         rqd->opcode = NVM_OP_PREAD;
> @@ -456,11 +298,6 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>         r_ctx->start_time = jiffies;
>         r_ctx->lba = blba;
>
> -       /* Save the index for this bio's start. This is needed in case
> -        * we need to fill a partial read.
> -        */
> -       bio_init_idx = pblk_get_bi_idx(bio);
> -
>         if (pblk_alloc_rqd_meta(pblk, rqd)) {
>                 bio_io_error(bio);
>                 pblk_free_rqd(pblk, rqd, PBLK_READ);
> @@ -474,34 +311,63 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>         int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
>
>         if (nr_secs > 1)
> -               pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
> +               pblk_read_ppalist_rq(pblk, rqd, int_bio, blba, &info);
>         else
> -               pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
> +               pblk_read_rq(pblk, rqd, int_bio, blba, &info);
>
> +split_retry:
>         r_ctx->private = bio; /* original bio */
>         rqd->bio = int_bio; /* internal bio */
>
> -       if (bitmap_full(read_bitmap, nr_secs)) {
> +       if (info.secs_from_cache && !info.secs_from_drive) {
>                 pblk_end_user_read(bio, 0);
>                 atomic_inc(&pblk->inflight_io);
>                 __pblk_end_io_read(pblk, rqd, false);
>                 return;
>         }
>
> -       if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
> +       if (info.secs_from_cache && info.secs_from_drive) {
>                 /* The read bio request could be partially filled by the write
>                  * buffer, but there are some holes that need to be read from
> -                * the drive.
> +                * the drive. In order to handle this, we will use block layer
> +                * mechanism to split this request in to smaller ones and make
> +                * a chain of it.
> +                */
> +               int split = max_t(int, info.first_sec_from_cache,
> +                                       info.first_sec_from_drive);
> +
> +               split_bio = bio_split(bio, split * NR_PHY_IN_LOG, GFP_KERNEL,
> +                                       &pblk_bio_set);
> +               bio_chain(split_bio, bio);
> +               generic_make_request(bio);
> +
> +               /* New bio contains first N sectors of the previous one, so
> +                * we can continue to use existing rqd, but we need to shrink
> +                * the number of PPAs in it. We also need to release line
> +                * references on the rest of the PPAs. Finally we also need
> +                * to update pblk_read_info struct properly in order to avoid
> +                * another interation of l2p lookup.
> +                */
> +               bio = split_bio;
> +               pblk_rq_to_line_partial_put(pblk, rqd, split);
> +#ifdef CONFIG_NVM_PBLK_DEBUG
> +               atomic_long_sub((rqd->nr_ppas - split), &pblk->inflight_reads);
> +#endif
> +               rqd->nr_ppas = split;
> +               if (rqd->nr_ppas == 1)
> +                       rqd->ppa_addr = rqd->ppa_list[0];
> +
> +               if (info.first_sec_from_cache > info.first_sec_from_drive)
> +                       info.secs_from_cache = false;
> +               else
> +                       info.secs_from_drive = false;
> +
> +               /* Recreate int_bio - existing might have some needed internal
> +                * fields modified already.
>                  */
>                 bio_put(int_bio);
> -               rqd->bio = NULL;
> -               if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
> -                                           nr_secs)) {
> -                       pblk_err(pblk, "read IO submission failed\n");
> -                       bio_io_error(bio);
> -                       __pblk_end_io_read(pblk, rqd, false);
> -               }
> -               return;
> +               int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
> +               goto split_retry;
>         }
>
>         /* All sectors are to be read from the device */
> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> index a3c925d..e284a93 100644
> --- a/drivers/lightnvm/pblk.h
> +++ b/drivers/lightnvm/pblk.h
> @@ -123,18 +123,6 @@ struct pblk_g_ctx {
>         u64 lba;
>  };
>
> -/* partial read context */
> -struct pblk_pr_ctx {
> -       struct bio *orig_bio;
> -       DECLARE_BITMAP(bitmap, NVM_MAX_VLBA);
> -       unsigned int orig_nr_secs;
> -       unsigned int bio_init_idx;
> -       void *ppa_ptr;
> -       dma_addr_t dma_ppa_list;
> -       u64 lba_list_mem[NVM_MAX_VLBA];
> -       u64 lba_list_media[NVM_MAX_VLBA];
> -};
> -
>  /* Pad context */
>  struct pblk_pad_rq {
>         struct pblk *pblk;
> @@ -726,6 +714,14 @@ struct pblk_line_ws {
>         struct work_struct ws;
>  };
>
> +/* L2P lookup on read path info */
> +struct pblk_read_info {
> +       int first_sec_from_cache;
> +       int first_sec_from_drive;
> +       bool secs_from_cache;
> +       bool secs_from_drive;
> +};
> +
>  #define pblk_g_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_g_ctx))
>  #define pblk_w_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_c_ctx))
>
> @@ -809,6 +805,8 @@ struct pblk_line *pblk_line_get_first_data(struct pblk *pblk);
>  struct pblk_line *pblk_line_replace_data(struct pblk *pblk);
>  void pblk_ppa_to_line_put(struct pblk *pblk, struct ppa_addr ppa);
>  void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd);
> +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
> +                                       int offset);
>  int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line);
>  void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line);
>  struct pblk_line *pblk_line_get_data(struct pblk *pblk);
> --
> 2.9.5
>

Re: [PATCH v3 02/10] lightnvm: pblk: simplify partial read path

[Heiner Litz]

Hi Igor,
thanks for sending the patch. There are a couple things I don't like too much:
- the fact that we have to carry around state in form of
pblk_read_info which somewhat replaces the read_bitmap
- the fact that sectors are looked up multiple times and hence krefs
are incremented and decremented unnecessarily
- the fact that we still bio_advance all over the place

I think we can avoid all above with a new lookup_l2p function which
does the following (pseudo code):

bool did_read_from_device = false;
bool did_read_from_cache = false;
lookup_l2p_segment() {
  for (i = 0; i < nr_secs; i++) {
    ppa = trans_map_get();
    if (!did_read_from_device && ppa_is_cached(ppa)) {
      read_from_cache(ppa);
      did_read_from_cache = true;
    }
    if (!did_read_from_cache && !ppa_is_cached(ppa)) {
      ppa_list[p++] = ppa;
      kref_get(line);
      did_read_from_device = true;
    }
    else
      return i;
  }
  return i;
}

This function achieves the following.
1) It returns the length (i) of a segment of consecutive cached or
device sectors. We can use i to split the bio
2) Whenever the function above returns we know that the split bio
either only contains device or cached sectors. This means we can
finish the split bio without needing read_info
3) The function returns after a split, so sectors are never looked up
twice (no multi-kref inc/dec)
4) don't need to bio_advance all over the place, as in a cached bio we
know that all sectors are cached and we can complete it at once
5) because we read from cache within lookup_l2p, we don't have to
retry externally if secs don't exist in the cache anymore

Note: If we don't want to read_from_cache while holding the trans_lock
we can release and re-acquire within lookup_l2p.


On Fri, Mar 29, 2019 at 2:12 AM Hans Holmberg <hans@owltronix.com> wrote:
>
> On Wed, Mar 27, 2019 at 1:41 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
> >
> > This patch changes the approach to handling partial read path, what is
> > needed for supporting multi-page bvec, which is currently broken.
> >
> > In old approach merging of data from round buffer and drive was fully
> > made by drive. This had some disadvantages - code was complex and
> > relies on bio internals, so it was hard to maintain and was strongly
> > dependent on bio changes.
> >
> > In new approach most of the handling is done mostly by block layer
> > functions such as bio_split(), bio_chain() and generic_make request()
> > and generally is less complex and easier to maintain. Below some more
> > details of the new approach.
> >
> > When read bio arrives, it is cloned for pblk internal purposes. All
> > the L2P mapping, which includes copying data from round buffer to bio
> > and thus bio_advance() calls is done on the cloned bio, so the original
> > bio is untouched. Later if we found that we have partial read case, we
> > still have original bio untouched, so we can split it and continue to
> > process only first 4K of it in current context, when the rest will be
> > called as separate bio request which is passed to generic_make_request()
> > for further processing.
> >
> > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>
> I've played around with fio to figure out if there is a read latency
> performance hit, and I have not found any diff that can't be written
> off as noise.
> (i did seq write + random 4k reads with different sizes and checked
> the latency stats)
>
> > ---
> >  drivers/lightnvm/pblk-core.c |  16 +++
> >  drivers/lightnvm/pblk-read.c | 276 +++++++++++--------------------------------
> >  drivers/lightnvm/pblk.h      |  22 ++--
> >  3 files changed, 97 insertions(+), 217 deletions(-)
>
> Nice cleanup :)
> I hope to find time early next week to go through the patch in depth.
>
> Thanks,
> Hans
>
> >
> > diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> > index 39280c1..f2edec6 100644
> > --- a/drivers/lightnvm/pblk-core.c
> > +++ b/drivers/lightnvm/pblk-core.c
> > @@ -1529,6 +1529,22 @@ void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd)
> >                 pblk_ppa_to_line_put(pblk, ppa_list[i]);
> >  }
> >
> > +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
> > +                                       int offset)
> > +{
> > +       struct ppa_addr ppa;
> > +       struct pblk_line *line;
> > +       int i = offset;
> > +
> > +       for (; i < rqd->nr_ppas; i++) {
> > +               ppa = rqd->ppa_list[i];
> > +               if (!pblk_ppa_empty(ppa) && !pblk_addr_in_cache(ppa)) {
> > +                       line = pblk_ppa_to_line(pblk, ppa);
> > +                       kref_put(&line->ref, pblk_line_put);
> > +               }
> > +       }
> > +}
> > +
> >  static void pblk_stop_writes(struct pblk *pblk, struct pblk_line *line)
> >  {
> >         lockdep_assert_held(&pblk->l_mg.free_lock);
> > diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
> > index 597fe6d..264d1d7 100644
> > --- a/drivers/lightnvm/pblk-read.c
> > +++ b/drivers/lightnvm/pblk-read.c
> > @@ -41,28 +41,30 @@ static int pblk_read_from_cache(struct pblk *pblk, struct bio *bio,
> >
> >  static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
> >                                  struct bio *bio, sector_t blba,
> > -                                unsigned long *read_bitmap)
> > +                                struct pblk_read_info *info)
> >  {
> >         void *meta_list = rqd->meta_list;
> > -       struct ppa_addr ppas[NVM_MAX_VLBA];
> >         int nr_secs = rqd->nr_ppas;
> >         bool advanced_bio = false;
> > -       int i, j = 0;
> > +       int i;
> >
> > -       pblk_lookup_l2p_seq(pblk, ppas, blba, nr_secs);
> > +       pblk_lookup_l2p_seq(pblk, rqd->ppa_list, blba, nr_secs);
> > +       info->first_sec_from_cache = nr_secs;
> > +       info->first_sec_from_drive = nr_secs;
> >
> >         for (i = 0; i < nr_secs; i++) {
> > -               struct ppa_addr p = ppas[i];
> >                 struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
> >                 sector_t lba = blba + i;
> >
> >  retry:
> > -               if (pblk_ppa_empty(p)) {
> > +               if (pblk_ppa_empty(rqd->ppa_list[i])) {
> >                         __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
> >
> > -                       WARN_ON(test_and_set_bit(i, read_bitmap));
> > -                       meta->lba = addr_empty;
> > +                       info->secs_from_cache = true;
> > +                       info->first_sec_from_cache = min_t(int, i,
> > +                                               info->first_sec_from_cache);
> >
> > +                       meta->lba = addr_empty;
> >                         if (unlikely(!advanced_bio)) {
> >                                 bio_advance(bio, (i) * PBLK_EXPOSED_PAGE_SIZE);
> >                                 advanced_bio = true;
> > @@ -74,13 +76,18 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
> >                 /* Try to read from write buffer. The address is later checked
> >                  * on the write buffer to prevent retrieving overwritten data.
> >                  */
> > -               if (pblk_addr_in_cache(p)) {
> > -                       if (!pblk_read_from_cache(pblk, bio, lba, p, i,
> > -                                                               advanced_bio)) {
> > -                               pblk_lookup_l2p_seq(pblk, &p, lba, 1);
> > +               if (pblk_addr_in_cache(rqd->ppa_list[i])) {
> > +                       if (!pblk_read_from_cache(pblk, bio, lba,
> > +                                       rqd->ppa_list[i], i, advanced_bio)) {
> > +                               pblk_lookup_l2p_seq(pblk, &rqd->ppa_list[i],
> > +                                                       lba, 1);
> >                                 goto retry;
> >                         }
> > -                       WARN_ON(test_and_set_bit(i, read_bitmap));
> > +
> > +                       info->secs_from_cache = true;
> > +                       info->first_sec_from_cache = min_t(int, i,
> > +                                               info->first_sec_from_cache);
> > +
> >                         meta->lba = cpu_to_le64(lba);
> >                         advanced_bio = true;
> >  #ifdef CONFIG_NVM_PBLK_DEBUG
> > @@ -88,7 +95,9 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
> >  #endif
> >                 } else {
> >                         /* Read from media non-cached sectors */
> > -                       rqd->ppa_list[j++] = p;
> > +                       info->secs_from_drive = true;
> > +                       info->first_sec_from_drive = min_t(int, i,
> > +                                               info->first_sec_from_drive);
> >                 }
> >
> >  next:
> > @@ -223,172 +232,8 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
> >         __pblk_end_io_read(pblk, rqd, true);
> >  }
> >
> > -static void pblk_end_partial_read(struct nvm_rq *rqd)
> > -{
> > -       struct pblk *pblk = rqd->private;
> > -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
> > -       struct pblk_pr_ctx *pr_ctx = r_ctx->private;
> > -       struct pblk_sec_meta *meta;
> > -       struct bio *new_bio = rqd->bio;
> > -       struct bio *bio = pr_ctx->orig_bio;
> > -       struct bio_vec src_bv, dst_bv;
> > -       void *meta_list = rqd->meta_list;
> > -       int bio_init_idx = pr_ctx->bio_init_idx;
> > -       unsigned long *read_bitmap = pr_ctx->bitmap;
> > -       int nr_secs = pr_ctx->orig_nr_secs;
> > -       int nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
> > -       void *src_p, *dst_p;
> > -       int hole, i;
> > -
> > -       if (unlikely(nr_holes == 1)) {
> > -               struct ppa_addr ppa;
> > -
> > -               ppa = rqd->ppa_addr;
> > -               rqd->ppa_list = pr_ctx->ppa_ptr;
> > -               rqd->dma_ppa_list = pr_ctx->dma_ppa_list;
> > -               rqd->ppa_list[0] = ppa;
> > -       }
> > -
> > -       for (i = 0; i < nr_secs; i++) {
> > -               meta = pblk_get_meta(pblk, meta_list, i);
> > -               pr_ctx->lba_list_media[i] = le64_to_cpu(meta->lba);
> > -               meta->lba = cpu_to_le64(pr_ctx->lba_list_mem[i]);
> > -       }
> > -
> > -       /* Fill the holes in the original bio */
> > -       i = 0;
> > -       hole = find_first_zero_bit(read_bitmap, nr_secs);
> > -       do {
> > -               struct pblk_line *line;
> > -
> > -               line = pblk_ppa_to_line(pblk, rqd->ppa_list[i]);
> > -               kref_put(&line->ref, pblk_line_put);
> > -
> > -               meta = pblk_get_meta(pblk, meta_list, hole);
> > -               meta->lba = cpu_to_le64(pr_ctx->lba_list_media[i]);
> > -
> > -               src_bv = new_bio->bi_io_vec[i++];
> > -               dst_bv = bio->bi_io_vec[bio_init_idx + hole];
> > -
> > -               src_p = kmap_atomic(src_bv.bv_page);
> > -               dst_p = kmap_atomic(dst_bv.bv_page);
> > -
> > -               memcpy(dst_p + dst_bv.bv_offset,
> > -                       src_p + src_bv.bv_offset,
> > -                       PBLK_EXPOSED_PAGE_SIZE);
> > -
> > -               kunmap_atomic(src_p);
> > -               kunmap_atomic(dst_p);
> > -
> > -               mempool_free(src_bv.bv_page, &pblk->page_bio_pool);
> > -
> > -               hole = find_next_zero_bit(read_bitmap, nr_secs, hole + 1);
> > -       } while (hole < nr_secs);
> > -
> > -       bio_put(new_bio);
> > -       kfree(pr_ctx);
> > -
> > -       /* restore original request */
> > -       rqd->bio = NULL;
> > -       rqd->nr_ppas = nr_secs;
> > -
> > -       pblk_end_user_read(bio, rqd->error);
> > -       __pblk_end_io_read(pblk, rqd, false);
> > -}
> > -
> > -static int pblk_setup_partial_read(struct pblk *pblk, struct nvm_rq *rqd,
> > -                           unsigned int bio_init_idx,
> > -                           unsigned long *read_bitmap,
> > -                           int nr_holes)
> > -{
> > -       void *meta_list = rqd->meta_list;
> > -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
> > -       struct pblk_pr_ctx *pr_ctx;
> > -       struct bio *new_bio, *bio = r_ctx->private;
> > -       int nr_secs = rqd->nr_ppas;
> > -       int i;
> > -
> > -       new_bio = bio_alloc(GFP_KERNEL, nr_holes);
> > -
> > -       if (pblk_bio_add_pages(pblk, new_bio, GFP_KERNEL, nr_holes))
> > -               goto fail_bio_put;
> > -
> > -       if (nr_holes != new_bio->bi_vcnt) {
> > -               WARN_ONCE(1, "pblk: malformed bio\n");
> > -               goto fail_free_pages;
> > -       }
> > -
> > -       pr_ctx = kzalloc(sizeof(struct pblk_pr_ctx), GFP_KERNEL);
> > -       if (!pr_ctx)
> > -               goto fail_free_pages;
> > -
> > -       for (i = 0; i < nr_secs; i++) {
> > -               struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
> > -
> > -               pr_ctx->lba_list_mem[i] = le64_to_cpu(meta->lba);
> > -       }
> > -
> > -       new_bio->bi_iter.bi_sector = 0; /* internal bio */
> > -       bio_set_op_attrs(new_bio, REQ_OP_READ, 0);
> > -
> > -       rqd->bio = new_bio;
> > -       rqd->nr_ppas = nr_holes;
> > -
> > -       pr_ctx->orig_bio = bio;
> > -       bitmap_copy(pr_ctx->bitmap, read_bitmap, NVM_MAX_VLBA);
> > -       pr_ctx->bio_init_idx = bio_init_idx;
> > -       pr_ctx->orig_nr_secs = nr_secs;
> > -       r_ctx->private = pr_ctx;
> > -
> > -       if (unlikely(nr_holes == 1)) {
> > -               pr_ctx->ppa_ptr = rqd->ppa_list;
> > -               pr_ctx->dma_ppa_list = rqd->dma_ppa_list;
> > -               rqd->ppa_addr = rqd->ppa_list[0];
> > -       }
> > -       return 0;
> > -
> > -fail_free_pages:
> > -       pblk_bio_free_pages(pblk, new_bio, 0, new_bio->bi_vcnt);
> > -fail_bio_put:
> > -       bio_put(new_bio);
> > -
> > -       return -ENOMEM;
> > -}
> > -
> > -static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
> > -                                unsigned int bio_init_idx,
> > -                                unsigned long *read_bitmap, int nr_secs)
> > -{
> > -       int nr_holes;
> > -       int ret;
> > -
> > -       nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
> > -
> > -       if (pblk_setup_partial_read(pblk, rqd, bio_init_idx, read_bitmap,
> > -                                   nr_holes))
> > -               return NVM_IO_ERR;
> > -
> > -       rqd->end_io = pblk_end_partial_read;
> > -
> > -       ret = pblk_submit_io(pblk, rqd);
> > -       if (ret) {
> > -               bio_put(rqd->bio);
> > -               pblk_err(pblk, "partial read IO submission failed\n");
> > -               goto err;
> > -       }
> > -
> > -       return NVM_IO_OK;
> > -
> > -err:
> > -       pblk_err(pblk, "failed to perform partial read\n");
> > -
> > -       /* Free allocated pages in new bio */
> > -       pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
> > -       return NVM_IO_ERR;
> > -}
> > -
> >  static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
> > -                        sector_t lba, unsigned long *read_bitmap)
> > +                        sector_t lba, struct pblk_read_info *info)
> >  {
> >         struct pblk_sec_meta *meta = pblk_get_meta(pblk, rqd->meta_list, 0);
> >         struct ppa_addr ppa;
> > @@ -403,8 +248,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
> >         if (pblk_ppa_empty(ppa)) {
> >                 __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
> >
> > -               WARN_ON(test_and_set_bit(0, read_bitmap));
> >                 meta->lba = addr_empty;
> > +               info->secs_from_cache = true;
> >                 return;
> >         }
> >
> > @@ -417,14 +262,14 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
> >                         goto retry;
> >                 }
> >
> > -               WARN_ON(test_and_set_bit(0, read_bitmap));
> >                 meta->lba = cpu_to_le64(lba);
> > -
> > +               info->secs_from_cache = true;
> >  #ifdef CONFIG_NVM_PBLK_DEBUG
> >                 atomic_long_inc(&pblk->cache_reads);
> >  #endif
> >         } else {
> >                 rqd->ppa_addr = ppa;
> > +               info->secs_from_drive = true;
> >         }
> >  }
> >
> > @@ -436,15 +281,12 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
> >         unsigned int nr_secs = pblk_get_secs(bio);
> >         struct pblk_g_ctx *r_ctx;
> >         struct nvm_rq *rqd;
> > -       struct bio *int_bio;
> > -       unsigned int bio_init_idx;
> > -       DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
> > +       struct bio *int_bio, *split_bio;
> > +       struct pblk_read_info info = {0};
> >
> >         generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
> >                               &pblk->disk->part0);
> >
> > -       bitmap_zero(read_bitmap, nr_secs);
> > -
> >         rqd = pblk_alloc_rqd(pblk, PBLK_READ);
> >
> >         rqd->opcode = NVM_OP_PREAD;
> > @@ -456,11 +298,6 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
> >         r_ctx->start_time = jiffies;
> >         r_ctx->lba = blba;
> >
> > -       /* Save the index for this bio's start. This is needed in case
> > -        * we need to fill a partial read.
> > -        */
> > -       bio_init_idx = pblk_get_bi_idx(bio);
> > -
> >         if (pblk_alloc_rqd_meta(pblk, rqd)) {
> >                 bio_io_error(bio);
> >                 pblk_free_rqd(pblk, rqd, PBLK_READ);
> > @@ -474,34 +311,63 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
> >         int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
> >
> >         if (nr_secs > 1)
> > -               pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
> > +               pblk_read_ppalist_rq(pblk, rqd, int_bio, blba, &info);
> >         else
> > -               pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
> > +               pblk_read_rq(pblk, rqd, int_bio, blba, &info);
> >
> > +split_retry:
> >         r_ctx->private = bio; /* original bio */
> >         rqd->bio = int_bio; /* internal bio */
> >
> > -       if (bitmap_full(read_bitmap, nr_secs)) {
> > +       if (info.secs_from_cache && !info.secs_from_drive) {
> >                 pblk_end_user_read(bio, 0);
> >                 atomic_inc(&pblk->inflight_io);
> >                 __pblk_end_io_read(pblk, rqd, false);
> >                 return;
> >         }
> >
> > -       if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
> > +       if (info.secs_from_cache && info.secs_from_drive) {
> >                 /* The read bio request could be partially filled by the write
> >                  * buffer, but there are some holes that need to be read from
> > -                * the drive.
> > +                * the drive. In order to handle this, we will use block layer
> > +                * mechanism to split this request in to smaller ones and make
> > +                * a chain of it.
> > +                */
> > +               int split = max_t(int, info.first_sec_from_cache,
> > +                                       info.first_sec_from_drive);
> > +
> > +               split_bio = bio_split(bio, split * NR_PHY_IN_LOG, GFP_KERNEL,
> > +                                       &pblk_bio_set);
> > +               bio_chain(split_bio, bio);
> > +               generic_make_request(bio);
> > +
> > +               /* New bio contains first N sectors of the previous one, so
> > +                * we can continue to use existing rqd, but we need to shrink
> > +                * the number of PPAs in it. We also need to release line
> > +                * references on the rest of the PPAs. Finally we also need
> > +                * to update pblk_read_info struct properly in order to avoid
> > +                * another interation of l2p lookup.
> > +                */
> > +               bio = split_bio;
> > +               pblk_rq_to_line_partial_put(pblk, rqd, split);
> > +#ifdef CONFIG_NVM_PBLK_DEBUG
> > +               atomic_long_sub((rqd->nr_ppas - split), &pblk->inflight_reads);
> > +#endif
> > +               rqd->nr_ppas = split;
> > +               if (rqd->nr_ppas == 1)
> > +                       rqd->ppa_addr = rqd->ppa_list[0];
> > +
> > +               if (info.first_sec_from_cache > info.first_sec_from_drive)
> > +                       info.secs_from_cache = false;
> > +               else
> > +                       info.secs_from_drive = false;
> > +
> > +               /* Recreate int_bio - existing might have some needed internal
> > +                * fields modified already.
> >                  */
> >                 bio_put(int_bio);
> > -               rqd->bio = NULL;
> > -               if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
> > -                                           nr_secs)) {
> > -                       pblk_err(pblk, "read IO submission failed\n");
> > -                       bio_io_error(bio);
> > -                       __pblk_end_io_read(pblk, rqd, false);
> > -               }
> > -               return;
> > +               int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
> > +               goto split_retry;
> >         }
> >
> >         /* All sectors are to be read from the device */
> > diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
> > index a3c925d..e284a93 100644
> > --- a/drivers/lightnvm/pblk.h
> > +++ b/drivers/lightnvm/pblk.h
> > @@ -123,18 +123,6 @@ struct pblk_g_ctx {
> >         u64 lba;
> >  };
> >
> > -/* partial read context */
> > -struct pblk_pr_ctx {
> > -       struct bio *orig_bio;
> > -       DECLARE_BITMAP(bitmap, NVM_MAX_VLBA);
> > -       unsigned int orig_nr_secs;
> > -       unsigned int bio_init_idx;
> > -       void *ppa_ptr;
> > -       dma_addr_t dma_ppa_list;
> > -       u64 lba_list_mem[NVM_MAX_VLBA];
> > -       u64 lba_list_media[NVM_MAX_VLBA];
> > -};
> > -
> >  /* Pad context */
> >  struct pblk_pad_rq {
> >         struct pblk *pblk;
> > @@ -726,6 +714,14 @@ struct pblk_line_ws {
> >         struct work_struct ws;
> >  };
> >
> > +/* L2P lookup on read path info */
> > +struct pblk_read_info {
> > +       int first_sec_from_cache;
> > +       int first_sec_from_drive;
> > +       bool secs_from_cache;
> > +       bool secs_from_drive;
> > +};
> > +
> >  #define pblk_g_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_g_ctx))
> >  #define pblk_w_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_c_ctx))
> >
> > @@ -809,6 +805,8 @@ struct pblk_line *pblk_line_get_first_data(struct pblk *pblk);
> >  struct pblk_line *pblk_line_replace_data(struct pblk *pblk);
> >  void pblk_ppa_to_line_put(struct pblk *pblk, struct ppa_addr ppa);
> >  void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd);
> > +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
> > +                                       int offset);
> >  int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line);
> >  void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line);
> >  struct pblk_line *pblk_line_get_data(struct pblk *pblk);
> > --
> > 2.9.5
> >

Re: [PATCH v3 02/10] lightnvm: pblk: simplify partial read path

[Igor Konopko]

On 30.03.2019 06:02, Heiner Litz wrote:
> Hi Igor,
> thanks for sending the patch. There are a couple things I don't like too much:
> - the fact that we have to carry around state in form of
> pblk_read_info which somewhat replaces the read_bitmap
> - the fact that sectors are looked up multiple times and hence krefs
> are incremented and decremented unnecessarily
> - the fact that we still bio_advance all over the place
> 
> I think we can avoid all above with a new lookup_l2p function which
> does the following (pseudo code):
> 

Thanks for your feedback.
Generally your ideas sounds right and I would try to incorporate it into 
v4 of that patch. I did some basic implementation, but I need to do some 
testing with it. It is slightly different from what you proposed in 
pseudo-code, but I think that my version would also fix the three issues 
which you mention above.

Igor

> bool did_read_from_device = false;
> bool did_read_from_cache = false;
> lookup_l2p_segment() {
>    for (i = 0; i < nr_secs; i++) {
>      ppa = trans_map_get();
>      if (!did_read_from_device && ppa_is_cached(ppa)) {
>        read_from_cache(ppa);
>        did_read_from_cache = true;
>      }
>      if (!did_read_from_cache && !ppa_is_cached(ppa)) {
>        ppa_list[p++] = ppa;
>        kref_get(line);
>        did_read_from_device = true;
>      }
>      else
>        return i;
>    }
>    return i;
> }
> 
> This function achieves the following.
> 1) It returns the length (i) of a segment of consecutive cached or
> device sectors. We can use i to split the bio
> 2) Whenever the function above returns we know that the split bio
> either only contains device or cached sectors. This means we can
> finish the split bio without needing read_info
> 3) The function returns after a split, so sectors are never looked up
> twice (no multi-kref inc/dec)
> 4) don't need to bio_advance all over the place, as in a cached bio we
> know that all sectors are cached and we can complete it at once
> 5) because we read from cache within lookup_l2p, we don't have to
> retry externally if secs don't exist in the cache anymore
> 
> Note: If we don't want to read_from_cache while holding the trans_lock
> we can release and re-acquire within lookup_l2p.
> 
> 
> On Fri, Mar 29, 2019 at 2:12 AM Hans Holmberg <hans@owltronix.com> wrote:
>>
>> On Wed, Mar 27, 2019 at 1:41 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>>>
>>> This patch changes the approach to handling partial read path, what is
>>> needed for supporting multi-page bvec, which is currently broken.
>>>
>>> In old approach merging of data from round buffer and drive was fully
>>> made by drive. This had some disadvantages - code was complex and
>>> relies on bio internals, so it was hard to maintain and was strongly
>>> dependent on bio changes.
>>>
>>> In new approach most of the handling is done mostly by block layer
>>> functions such as bio_split(), bio_chain() and generic_make request()
>>> and generally is less complex and easier to maintain. Below some more
>>> details of the new approach.
>>>
>>> When read bio arrives, it is cloned for pblk internal purposes. All
>>> the L2P mapping, which includes copying data from round buffer to bio
>>> and thus bio_advance() calls is done on the cloned bio, so the original
>>> bio is untouched. Later if we found that we have partial read case, we
>>> still have original bio untouched, so we can split it and continue to
>>> process only first 4K of it in current context, when the rest will be
>>> called as separate bio request which is passed to generic_make_request()
>>> for further processing.
>>>
>>> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
>>
>> I've played around with fio to figure out if there is a read latency
>> performance hit, and I have not found any diff that can't be written
>> off as noise.
>> (i did seq write + random 4k reads with different sizes and checked
>> the latency stats)
>>
>>> ---
>>>   drivers/lightnvm/pblk-core.c |  16 +++
>>>   drivers/lightnvm/pblk-read.c | 276 +++++++++++--------------------------------
>>>   drivers/lightnvm/pblk.h      |  22 ++--
>>>   3 files changed, 97 insertions(+), 217 deletions(-)
>>
>> Nice cleanup :)
>> I hope to find time early next week to go through the patch in depth.
>>
>> Thanks,
>> Hans
>>
>>>
>>> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
>>> index 39280c1..f2edec6 100644
>>> --- a/drivers/lightnvm/pblk-core.c
>>> +++ b/drivers/lightnvm/pblk-core.c
>>> @@ -1529,6 +1529,22 @@ void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd)
>>>                  pblk_ppa_to_line_put(pblk, ppa_list[i]);
>>>   }
>>>
>>> +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
>>> +                                       int offset)
>>> +{
>>> +       struct ppa_addr ppa;
>>> +       struct pblk_line *line;
>>> +       int i = offset;
>>> +
>>> +       for (; i < rqd->nr_ppas; i++) {
>>> +               ppa = rqd->ppa_list[i];
>>> +               if (!pblk_ppa_empty(ppa) && !pblk_addr_in_cache(ppa)) {
>>> +                       line = pblk_ppa_to_line(pblk, ppa);
>>> +                       kref_put(&line->ref, pblk_line_put);
>>> +               }
>>> +       }
>>> +}
>>> +
>>>   static void pblk_stop_writes(struct pblk *pblk, struct pblk_line *line)
>>>   {
>>>          lockdep_assert_held(&pblk->l_mg.free_lock);
>>> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
>>> index 597fe6d..264d1d7 100644
>>> --- a/drivers/lightnvm/pblk-read.c
>>> +++ b/drivers/lightnvm/pblk-read.c
>>> @@ -41,28 +41,30 @@ static int pblk_read_from_cache(struct pblk *pblk, struct bio *bio,
>>>
>>>   static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>>                                   struct bio *bio, sector_t blba,
>>> -                                unsigned long *read_bitmap)
>>> +                                struct pblk_read_info *info)
>>>   {
>>>          void *meta_list = rqd->meta_list;
>>> -       struct ppa_addr ppas[NVM_MAX_VLBA];
>>>          int nr_secs = rqd->nr_ppas;
>>>          bool advanced_bio = false;
>>> -       int i, j = 0;
>>> +       int i;
>>>
>>> -       pblk_lookup_l2p_seq(pblk, ppas, blba, nr_secs);
>>> +       pblk_lookup_l2p_seq(pblk, rqd->ppa_list, blba, nr_secs);
>>> +       info->first_sec_from_cache = nr_secs;
>>> +       info->first_sec_from_drive = nr_secs;
>>>
>>>          for (i = 0; i < nr_secs; i++) {
>>> -               struct ppa_addr p = ppas[i];
>>>                  struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
>>>                  sector_t lba = blba + i;
>>>
>>>   retry:
>>> -               if (pblk_ppa_empty(p)) {
>>> +               if (pblk_ppa_empty(rqd->ppa_list[i])) {
>>>                          __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
>>>
>>> -                       WARN_ON(test_and_set_bit(i, read_bitmap));
>>> -                       meta->lba = addr_empty;
>>> +                       info->secs_from_cache = true;
>>> +                       info->first_sec_from_cache = min_t(int, i,
>>> +                                               info->first_sec_from_cache);
>>>
>>> +                       meta->lba = addr_empty;
>>>                          if (unlikely(!advanced_bio)) {
>>>                                  bio_advance(bio, (i) * PBLK_EXPOSED_PAGE_SIZE);
>>>                                  advanced_bio = true;
>>> @@ -74,13 +76,18 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>>                  /* Try to read from write buffer. The address is later checked
>>>                   * on the write buffer to prevent retrieving overwritten data.
>>>                   */
>>> -               if (pblk_addr_in_cache(p)) {
>>> -                       if (!pblk_read_from_cache(pblk, bio, lba, p, i,
>>> -                                                               advanced_bio)) {
>>> -                               pblk_lookup_l2p_seq(pblk, &p, lba, 1);
>>> +               if (pblk_addr_in_cache(rqd->ppa_list[i])) {
>>> +                       if (!pblk_read_from_cache(pblk, bio, lba,
>>> +                                       rqd->ppa_list[i], i, advanced_bio)) {
>>> +                               pblk_lookup_l2p_seq(pblk, &rqd->ppa_list[i],
>>> +                                                       lba, 1);
>>>                                  goto retry;
>>>                          }
>>> -                       WARN_ON(test_and_set_bit(i, read_bitmap));
>>> +
>>> +                       info->secs_from_cache = true;
>>> +                       info->first_sec_from_cache = min_t(int, i,
>>> +                                               info->first_sec_from_cache);
>>> +
>>>                          meta->lba = cpu_to_le64(lba);
>>>                          advanced_bio = true;
>>>   #ifdef CONFIG_NVM_PBLK_DEBUG
>>> @@ -88,7 +95,9 @@ static void pblk_read_ppalist_rq(struct pblk *pblk, struct nvm_rq *rqd,
>>>   #endif
>>>                  } else {
>>>                          /* Read from media non-cached sectors */
>>> -                       rqd->ppa_list[j++] = p;
>>> +                       info->secs_from_drive = true;
>>> +                       info->first_sec_from_drive = min_t(int, i,
>>> +                                               info->first_sec_from_drive);
>>>                  }
>>>
>>>   next:
>>> @@ -223,172 +232,8 @@ static void pblk_end_io_read(struct nvm_rq *rqd)
>>>          __pblk_end_io_read(pblk, rqd, true);
>>>   }
>>>
>>> -static void pblk_end_partial_read(struct nvm_rq *rqd)
>>> -{
>>> -       struct pblk *pblk = rqd->private;
>>> -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
>>> -       struct pblk_pr_ctx *pr_ctx = r_ctx->private;
>>> -       struct pblk_sec_meta *meta;
>>> -       struct bio *new_bio = rqd->bio;
>>> -       struct bio *bio = pr_ctx->orig_bio;
>>> -       struct bio_vec src_bv, dst_bv;
>>> -       void *meta_list = rqd->meta_list;
>>> -       int bio_init_idx = pr_ctx->bio_init_idx;
>>> -       unsigned long *read_bitmap = pr_ctx->bitmap;
>>> -       int nr_secs = pr_ctx->orig_nr_secs;
>>> -       int nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
>>> -       void *src_p, *dst_p;
>>> -       int hole, i;
>>> -
>>> -       if (unlikely(nr_holes == 1)) {
>>> -               struct ppa_addr ppa;
>>> -
>>> -               ppa = rqd->ppa_addr;
>>> -               rqd->ppa_list = pr_ctx->ppa_ptr;
>>> -               rqd->dma_ppa_list = pr_ctx->dma_ppa_list;
>>> -               rqd->ppa_list[0] = ppa;
>>> -       }
>>> -
>>> -       for (i = 0; i < nr_secs; i++) {
>>> -               meta = pblk_get_meta(pblk, meta_list, i);
>>> -               pr_ctx->lba_list_media[i] = le64_to_cpu(meta->lba);
>>> -               meta->lba = cpu_to_le64(pr_ctx->lba_list_mem[i]);
>>> -       }
>>> -
>>> -       /* Fill the holes in the original bio */
>>> -       i = 0;
>>> -       hole = find_first_zero_bit(read_bitmap, nr_secs);
>>> -       do {
>>> -               struct pblk_line *line;
>>> -
>>> -               line = pblk_ppa_to_line(pblk, rqd->ppa_list[i]);
>>> -               kref_put(&line->ref, pblk_line_put);
>>> -
>>> -               meta = pblk_get_meta(pblk, meta_list, hole);
>>> -               meta->lba = cpu_to_le64(pr_ctx->lba_list_media[i]);
>>> -
>>> -               src_bv = new_bio->bi_io_vec[i++];
>>> -               dst_bv = bio->bi_io_vec[bio_init_idx + hole];
>>> -
>>> -               src_p = kmap_atomic(src_bv.bv_page);
>>> -               dst_p = kmap_atomic(dst_bv.bv_page);
>>> -
>>> -               memcpy(dst_p + dst_bv.bv_offset,
>>> -                       src_p + src_bv.bv_offset,
>>> -                       PBLK_EXPOSED_PAGE_SIZE);
>>> -
>>> -               kunmap_atomic(src_p);
>>> -               kunmap_atomic(dst_p);
>>> -
>>> -               mempool_free(src_bv.bv_page, &pblk->page_bio_pool);
>>> -
>>> -               hole = find_next_zero_bit(read_bitmap, nr_secs, hole + 1);
>>> -       } while (hole < nr_secs);
>>> -
>>> -       bio_put(new_bio);
>>> -       kfree(pr_ctx);
>>> -
>>> -       /* restore original request */
>>> -       rqd->bio = NULL;
>>> -       rqd->nr_ppas = nr_secs;
>>> -
>>> -       pblk_end_user_read(bio, rqd->error);
>>> -       __pblk_end_io_read(pblk, rqd, false);
>>> -}
>>> -
>>> -static int pblk_setup_partial_read(struct pblk *pblk, struct nvm_rq *rqd,
>>> -                           unsigned int bio_init_idx,
>>> -                           unsigned long *read_bitmap,
>>> -                           int nr_holes)
>>> -{
>>> -       void *meta_list = rqd->meta_list;
>>> -       struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd);
>>> -       struct pblk_pr_ctx *pr_ctx;
>>> -       struct bio *new_bio, *bio = r_ctx->private;
>>> -       int nr_secs = rqd->nr_ppas;
>>> -       int i;
>>> -
>>> -       new_bio = bio_alloc(GFP_KERNEL, nr_holes);
>>> -
>>> -       if (pblk_bio_add_pages(pblk, new_bio, GFP_KERNEL, nr_holes))
>>> -               goto fail_bio_put;
>>> -
>>> -       if (nr_holes != new_bio->bi_vcnt) {
>>> -               WARN_ONCE(1, "pblk: malformed bio\n");
>>> -               goto fail_free_pages;
>>> -       }
>>> -
>>> -       pr_ctx = kzalloc(sizeof(struct pblk_pr_ctx), GFP_KERNEL);
>>> -       if (!pr_ctx)
>>> -               goto fail_free_pages;
>>> -
>>> -       for (i = 0; i < nr_secs; i++) {
>>> -               struct pblk_sec_meta *meta = pblk_get_meta(pblk, meta_list, i);
>>> -
>>> -               pr_ctx->lba_list_mem[i] = le64_to_cpu(meta->lba);
>>> -       }
>>> -
>>> -       new_bio->bi_iter.bi_sector = 0; /* internal bio */
>>> -       bio_set_op_attrs(new_bio, REQ_OP_READ, 0);
>>> -
>>> -       rqd->bio = new_bio;
>>> -       rqd->nr_ppas = nr_holes;
>>> -
>>> -       pr_ctx->orig_bio = bio;
>>> -       bitmap_copy(pr_ctx->bitmap, read_bitmap, NVM_MAX_VLBA);
>>> -       pr_ctx->bio_init_idx = bio_init_idx;
>>> -       pr_ctx->orig_nr_secs = nr_secs;
>>> -       r_ctx->private = pr_ctx;
>>> -
>>> -       if (unlikely(nr_holes == 1)) {
>>> -               pr_ctx->ppa_ptr = rqd->ppa_list;
>>> -               pr_ctx->dma_ppa_list = rqd->dma_ppa_list;
>>> -               rqd->ppa_addr = rqd->ppa_list[0];
>>> -       }
>>> -       return 0;
>>> -
>>> -fail_free_pages:
>>> -       pblk_bio_free_pages(pblk, new_bio, 0, new_bio->bi_vcnt);
>>> -fail_bio_put:
>>> -       bio_put(new_bio);
>>> -
>>> -       return -ENOMEM;
>>> -}
>>> -
>>> -static int pblk_partial_read_bio(struct pblk *pblk, struct nvm_rq *rqd,
>>> -                                unsigned int bio_init_idx,
>>> -                                unsigned long *read_bitmap, int nr_secs)
>>> -{
>>> -       int nr_holes;
>>> -       int ret;
>>> -
>>> -       nr_holes = nr_secs - bitmap_weight(read_bitmap, nr_secs);
>>> -
>>> -       if (pblk_setup_partial_read(pblk, rqd, bio_init_idx, read_bitmap,
>>> -                                   nr_holes))
>>> -               return NVM_IO_ERR;
>>> -
>>> -       rqd->end_io = pblk_end_partial_read;
>>> -
>>> -       ret = pblk_submit_io(pblk, rqd);
>>> -       if (ret) {
>>> -               bio_put(rqd->bio);
>>> -               pblk_err(pblk, "partial read IO submission failed\n");
>>> -               goto err;
>>> -       }
>>> -
>>> -       return NVM_IO_OK;
>>> -
>>> -err:
>>> -       pblk_err(pblk, "failed to perform partial read\n");
>>> -
>>> -       /* Free allocated pages in new bio */
>>> -       pblk_bio_free_pages(pblk, rqd->bio, 0, rqd->bio->bi_vcnt);
>>> -       return NVM_IO_ERR;
>>> -}
>>> -
>>>   static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>>> -                        sector_t lba, unsigned long *read_bitmap)
>>> +                        sector_t lba, struct pblk_read_info *info)
>>>   {
>>>          struct pblk_sec_meta *meta = pblk_get_meta(pblk, rqd->meta_list, 0);
>>>          struct ppa_addr ppa;
>>> @@ -403,8 +248,8 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>>>          if (pblk_ppa_empty(ppa)) {
>>>                  __le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
>>>
>>> -               WARN_ON(test_and_set_bit(0, read_bitmap));
>>>                  meta->lba = addr_empty;
>>> +               info->secs_from_cache = true;
>>>                  return;
>>>          }
>>>
>>> @@ -417,14 +262,14 @@ static void pblk_read_rq(struct pblk *pblk, struct nvm_rq *rqd, struct bio *bio,
>>>                          goto retry;
>>>                  }
>>>
>>> -               WARN_ON(test_and_set_bit(0, read_bitmap));
>>>                  meta->lba = cpu_to_le64(lba);
>>> -
>>> +               info->secs_from_cache = true;
>>>   #ifdef CONFIG_NVM_PBLK_DEBUG
>>>                  atomic_long_inc(&pblk->cache_reads);
>>>   #endif
>>>          } else {
>>>                  rqd->ppa_addr = ppa;
>>> +               info->secs_from_drive = true;
>>>          }
>>>   }
>>>
>>> @@ -436,15 +281,12 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>>          unsigned int nr_secs = pblk_get_secs(bio);
>>>          struct pblk_g_ctx *r_ctx;
>>>          struct nvm_rq *rqd;
>>> -       struct bio *int_bio;
>>> -       unsigned int bio_init_idx;
>>> -       DECLARE_BITMAP(read_bitmap, NVM_MAX_VLBA);
>>> +       struct bio *int_bio, *split_bio;
>>> +       struct pblk_read_info info = {0};
>>>
>>>          generic_start_io_acct(q, REQ_OP_READ, bio_sectors(bio),
>>>                                &pblk->disk->part0);
>>>
>>> -       bitmap_zero(read_bitmap, nr_secs);
>>> -
>>>          rqd = pblk_alloc_rqd(pblk, PBLK_READ);
>>>
>>>          rqd->opcode = NVM_OP_PREAD;
>>> @@ -456,11 +298,6 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>>          r_ctx->start_time = jiffies;
>>>          r_ctx->lba = blba;
>>>
>>> -       /* Save the index for this bio's start. This is needed in case
>>> -        * we need to fill a partial read.
>>> -        */
>>> -       bio_init_idx = pblk_get_bi_idx(bio);
>>> -
>>>          if (pblk_alloc_rqd_meta(pblk, rqd)) {
>>>                  bio_io_error(bio);
>>>                  pblk_free_rqd(pblk, rqd, PBLK_READ);
>>> @@ -474,34 +311,63 @@ void pblk_submit_read(struct pblk *pblk, struct bio *bio)
>>>          int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
>>>
>>>          if (nr_secs > 1)
>>> -               pblk_read_ppalist_rq(pblk, rqd, bio, blba, read_bitmap);
>>> +               pblk_read_ppalist_rq(pblk, rqd, int_bio, blba, &info);
>>>          else
>>> -               pblk_read_rq(pblk, rqd, bio, blba, read_bitmap);
>>> +               pblk_read_rq(pblk, rqd, int_bio, blba, &info);
>>>
>>> +split_retry:
>>>          r_ctx->private = bio; /* original bio */
>>>          rqd->bio = int_bio; /* internal bio */
>>>
>>> -       if (bitmap_full(read_bitmap, nr_secs)) {
>>> +       if (info.secs_from_cache && !info.secs_from_drive) {
>>>                  pblk_end_user_read(bio, 0);
>>>                  atomic_inc(&pblk->inflight_io);
>>>                  __pblk_end_io_read(pblk, rqd, false);
>>>                  return;
>>>          }
>>>
>>> -       if (!bitmap_empty(read_bitmap, rqd->nr_ppas)) {
>>> +       if (info.secs_from_cache && info.secs_from_drive) {
>>>                  /* The read bio request could be partially filled by the write
>>>                   * buffer, but there are some holes that need to be read from
>>> -                * the drive.
>>> +                * the drive. In order to handle this, we will use block layer
>>> +                * mechanism to split this request in to smaller ones and make
>>> +                * a chain of it.
>>> +                */
>>> +               int split = max_t(int, info.first_sec_from_cache,
>>> +                                       info.first_sec_from_drive);
>>> +
>>> +               split_bio = bio_split(bio, split * NR_PHY_IN_LOG, GFP_KERNEL,
>>> +                                       &pblk_bio_set);
>>> +               bio_chain(split_bio, bio);
>>> +               generic_make_request(bio);
>>> +
>>> +               /* New bio contains first N sectors of the previous one, so
>>> +                * we can continue to use existing rqd, but we need to shrink
>>> +                * the number of PPAs in it. We also need to release line
>>> +                * references on the rest of the PPAs. Finally we also need
>>> +                * to update pblk_read_info struct properly in order to avoid
>>> +                * another interation of l2p lookup.
>>> +                */
>>> +               bio = split_bio;
>>> +               pblk_rq_to_line_partial_put(pblk, rqd, split);
>>> +#ifdef CONFIG_NVM_PBLK_DEBUG
>>> +               atomic_long_sub((rqd->nr_ppas - split), &pblk->inflight_reads);
>>> +#endif
>>> +               rqd->nr_ppas = split;
>>> +               if (rqd->nr_ppas == 1)
>>> +                       rqd->ppa_addr = rqd->ppa_list[0];
>>> +
>>> +               if (info.first_sec_from_cache > info.first_sec_from_drive)
>>> +                       info.secs_from_cache = false;
>>> +               else
>>> +                       info.secs_from_drive = false;
>>> +
>>> +               /* Recreate int_bio - existing might have some needed internal
>>> +                * fields modified already.
>>>                   */
>>>                  bio_put(int_bio);
>>> -               rqd->bio = NULL;
>>> -               if (pblk_partial_read_bio(pblk, rqd, bio_init_idx, read_bitmap,
>>> -                                           nr_secs)) {
>>> -                       pblk_err(pblk, "read IO submission failed\n");
>>> -                       bio_io_error(bio);
>>> -                       __pblk_end_io_read(pblk, rqd, false);
>>> -               }
>>> -               return;
>>> +               int_bio = bio_clone_fast(bio, GFP_KERNEL, &pblk_bio_set);
>>> +               goto split_retry;
>>>          }
>>>
>>>          /* All sectors are to be read from the device */
>>> diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
>>> index a3c925d..e284a93 100644
>>> --- a/drivers/lightnvm/pblk.h
>>> +++ b/drivers/lightnvm/pblk.h
>>> @@ -123,18 +123,6 @@ struct pblk_g_ctx {
>>>          u64 lba;
>>>   };
>>>
>>> -/* partial read context */
>>> -struct pblk_pr_ctx {
>>> -       struct bio *orig_bio;
>>> -       DECLARE_BITMAP(bitmap, NVM_MAX_VLBA);
>>> -       unsigned int orig_nr_secs;
>>> -       unsigned int bio_init_idx;
>>> -       void *ppa_ptr;
>>> -       dma_addr_t dma_ppa_list;
>>> -       u64 lba_list_mem[NVM_MAX_VLBA];
>>> -       u64 lba_list_media[NVM_MAX_VLBA];
>>> -};
>>> -
>>>   /* Pad context */
>>>   struct pblk_pad_rq {
>>>          struct pblk *pblk;
>>> @@ -726,6 +714,14 @@ struct pblk_line_ws {
>>>          struct work_struct ws;
>>>   };
>>>
>>> +/* L2P lookup on read path info */
>>> +struct pblk_read_info {
>>> +       int first_sec_from_cache;
>>> +       int first_sec_from_drive;
>>> +       bool secs_from_cache;
>>> +       bool secs_from_drive;
>>> +};
>>> +
>>>   #define pblk_g_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_g_ctx))
>>>   #define pblk_w_rq_size (sizeof(struct nvm_rq) + sizeof(struct pblk_c_ctx))
>>>
>>> @@ -809,6 +805,8 @@ struct pblk_line *pblk_line_get_first_data(struct pblk *pblk);
>>>   struct pblk_line *pblk_line_replace_data(struct pblk *pblk);
>>>   void pblk_ppa_to_line_put(struct pblk *pblk, struct ppa_addr ppa);
>>>   void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd);
>>> +void pblk_rq_to_line_partial_put(struct pblk *pblk, struct nvm_rq *rqd,
>>> +                                       int offset);
>>>   int pblk_line_recov_alloc(struct pblk *pblk, struct pblk_line *line);
>>>   void pblk_line_recov_close(struct pblk *pblk, struct pblk_line *line);
>>>   struct pblk_line *pblk_line_get_data(struct pblk *pblk);
>>> --
>>> 2.9.5
>>>

[PATCH v3 03/10] lightnvm: pblk: propagate errors when reading meta

[Igor Konopko]

Currently when smeta/emeta/oob is read errors are not always propagated
correctly. This patch changes that behaviour and propagates all the
error codes except of high ecc read warning status.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---
 drivers/lightnvm/pblk-core.c     | 9 +++++++--
 drivers/lightnvm/pblk-recovery.c | 2 +-
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index f2edec6..06ac3f0 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -761,8 +761,10 @@ int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
 
 	atomic_dec(&pblk->inflight_io);
 
-	if (rqd.error)
+	if (rqd.error && rqd.error != NVM_RSP_WARN_HIGHECC) {
 		pblk_log_read_err(pblk, &rqd);
+		ret = -EIO;
+	}
 
 clear_rqd:
 	pblk_free_rqd_meta(pblk, &rqd);
@@ -916,8 +918,11 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 
 	atomic_dec(&pblk->inflight_io);
 
-	if (rqd.error)
+	if (rqd.error && rqd.error != NVM_RSP_WARN_HIGHECC) {
 		pblk_log_read_err(pblk, &rqd);
+		ret = -EIO;
+		goto free_rqd_dma;
+	}
 
 	emeta_buf += rq_len;
 	left_ppas -= rq_ppas;
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 357e529..124d817 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -458,7 +458,7 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
 	atomic_dec(&pblk->inflight_io);
 
 	/* If a read fails, do a best effort by padding the line and retrying */
-	if (rqd->error) {
+	if (rqd->error && rqd->error != NVM_RSP_WARN_HIGHECC) {
 		int pad_distance, ret;
 
 		if (padded) {
-- 
2.9.5

Re: [PATCH v3 03/10] lightnvm: pblk: propagate errors when reading meta

[Matias Bjørling]

On 3/27/19 1:38 PM, Igor Konopko wrote:
> Currently when smeta/emeta/oob is read errors are not always propagated
> correctly. This patch changes that behaviour and propagates all the
> error codes except of high ecc read warning status.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> Reviewed-by: Javier González <javier@javigon.com>
> Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
> ---
>   drivers/lightnvm/pblk-core.c     | 9 +++++++--
>   drivers/lightnvm/pblk-recovery.c | 2 +-
>   2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
> index f2edec6..06ac3f0 100644
> --- a/drivers/lightnvm/pblk-core.c
> +++ b/drivers/lightnvm/pblk-core.c
> @@ -761,8 +761,10 @@ int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
>   
>   	atomic_dec(&pblk->inflight_io);
>   
> -	if (rqd.error)
> +	if (rqd.error && rqd.error != NVM_RSP_WARN_HIGHECC) {
>   		pblk_log_read_err(pblk, &rqd);
> +		ret = -EIO;
> +	}
>   
>   clear_rqd:
>   	pblk_free_rqd_meta(pblk, &rqd);
> @@ -916,8 +918,11 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
>   
>   	atomic_dec(&pblk->inflight_io);
>   
> -	if (rqd.error)
> +	if (rqd.error && rqd.error != NVM_RSP_WARN_HIGHECC) {
>   		pblk_log_read_err(pblk, &rqd);
> +		ret = -EIO;
> +		goto free_rqd_dma;
> +	}
>   
>   	emeta_buf += rq_len;
>   	left_ppas -= rq_ppas;
> diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
> index 357e529..124d817 100644
> --- a/drivers/lightnvm/pblk-recovery.c
> +++ b/drivers/lightnvm/pblk-recovery.c
> @@ -458,7 +458,7 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
>   	atomic_dec(&pblk->inflight_io);
>   
>   	/* If a read fails, do a best effort by padding the line and retrying */
> -	if (rqd->error) {
> +	if (rqd->error && rqd->error != NVM_RSP_WARN_HIGHECC) {
>   		int pad_distance, ret;
>   
>   		if (padded) {
> 

Thanks, applied. I've updated the wording a bit.

[PATCH v3 04/10] lightnvm: pblk: recover only written metadata

[Igor Konopko]

This patch ensures that smeta was fully written before even
trying to read it based on chunk table state and write pointer.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-recovery.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 124d817..9318f7a 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -661,10 +661,12 @@ static int pblk_line_was_written(struct pblk_line *line,
 	bppa = pblk->luns[smeta_blk].bppa;
 	chunk = &line->chks[pblk_ppa_to_pos(geo, bppa)];
 
-	if (chunk->state & NVM_CHK_ST_FREE)
-		return 0;
+	if (chunk->state & NVM_CHK_ST_CLOSED ||
+	    (chunk->state & NVM_CHK_ST_OPEN
+	     && chunk->wp >= lm->smeta_sec))
+		return 1;
 
-	return 1;
+	return 0;
 }
 
 static bool pblk_line_is_open(struct pblk *pblk, struct pblk_line *line)
-- 
2.9.5

[PATCH v3 05/10] lightnvm: pblk: wait for inflight IOs in recovery

[Igor Konopko]

This patch changes the behaviour of recovery padding in order to
support a case, when some IOs were already submitted to the drive and
some next one are not submitted due to error returned.

Currently in case of errors we simply exit the pad function without
waiting for inflight IOs, which leads to panic on inflight IOs
completion.

After the changes we always wait for all the inflight IOs before
exiting the function.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-recovery.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 9318f7a..6765d7c 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -208,7 +208,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	rq_ppas = pblk_calc_secs(pblk, left_ppas, 0, false);
 	if (rq_ppas < pblk->min_write_pgs) {
 		pblk_err(pblk, "corrupted pad line %d\n", line->id);
-		goto fail_free_pad;
+		goto fail_complete;
 	}
 
 	rq_len = rq_ppas * geo->csecs;
@@ -217,7 +217,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 						PBLK_VMALLOC_META, GFP_KERNEL);
 	if (IS_ERR(bio)) {
 		ret = PTR_ERR(bio);
-		goto fail_free_pad;
+		goto fail_complete;
 	}
 
 	bio->bi_iter.bi_sector = 0; /* internal bio */
@@ -226,8 +226,11 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	rqd = pblk_alloc_rqd(pblk, PBLK_WRITE_INT);
 
 	ret = pblk_alloc_rqd_meta(pblk, rqd);
-	if (ret)
-		goto fail_free_rqd;
+	if (ret) {
+		pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
+		bio_put(bio);
+		goto fail_complete;
+	}
 
 	rqd->bio = bio;
 	rqd->opcode = NVM_OP_PWRITE;
@@ -274,7 +277,10 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	if (ret) {
 		pblk_err(pblk, "I/O submission failed: %d\n", ret);
 		pblk_up_chunk(pblk, rqd->ppa_list[0]);
-		goto fail_free_rqd;
+		kref_put(&pad_rq->ref, pblk_recov_complete);
+		pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
+		bio_put(bio);
+		goto fail_complete;
 	}
 
 	left_line_ppas -= rq_ppas;
@@ -282,6 +288,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	if (left_ppas && left_line_ppas)
 		goto next_pad_rq;
 
+fail_complete:
 	kref_put(&pad_rq->ref, pblk_recov_complete);
 
 	if (!wait_for_completion_io_timeout(&pad_rq->wait,
@@ -297,14 +304,6 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 free_rq:
 	kfree(pad_rq);
 	return ret;
-
-fail_free_rqd:
-	pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
-	bio_put(bio);
-fail_free_pad:
-	kfree(pad_rq);
-	vfree(data);
-	return ret;
 }
 
 static int pblk_pad_distance(struct pblk *pblk, struct pblk_line *line)
-- 
2.9.5

[PATCH v3 06/10] lightnvm: pblk: remove internal IO timeout

[Igor Konopko]

Currently during pblk padding, there is internal IO timeout introduced,
which is smaller than default NVMe timeout. This can lead to various
use-after-free issues. Since in case of any IO timeouts NVMe and block
layer will handle timeout by themselves and report it back to use,
there is no need to keep this internal timeout in pblk.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/pblk-recovery.c | 7 +------
 drivers/lightnvm/pblk.h          | 2 --
 2 files changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 6765d7c..a9085b0 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -290,12 +290,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 
 fail_complete:
 	kref_put(&pad_rq->ref, pblk_recov_complete);
-
-	if (!wait_for_completion_io_timeout(&pad_rq->wait,
-				msecs_to_jiffies(PBLK_COMMAND_TIMEOUT_MS))) {
-		pblk_err(pblk, "pad write timed out\n");
-		ret = -ETIME;
-	}
+	wait_for_completion(&pad_rq->wait);
 
 	if (!pblk_line_is_full(line))
 		pblk_err(pblk, "corrupted padded line: %d\n", line->id);
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index e284a93..3a84c8a 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -43,8 +43,6 @@
 
 #define PBLK_CACHE_NAME_LEN (DISK_NAME_LEN + 16)
 
-#define PBLK_COMMAND_TIMEOUT_MS 30000
-
 /* Max 512 LUNs per device */
 #define PBLK_MAX_LUNS_BITMAP (4)
 
-- 
2.9.5

[PATCH v3 07/10] lightnvm: pblk: store multiple copies of smeta

[Igor Konopko]

Currently there is only one copy of smeta stored per line in pblk. This
is risky, because in case of read error on such a chunk, we are losing
all the data from whole line, what leads to silent data corruption.

This patch changes this behaviour and allows to store more then one
copy of the smeta (specified by module parameter) in order to provide
higher reliability by storing mirrored copies of smeta struct and
providing possibility to failover to another copy of that struct in
case of read error. Such an approach ensures that copies of that
critical structures will be stored on different dies and thus predicted
UBER is multiple times higher

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/pblk-core.c     | 124 ++++++++++++++++++++++++++++++++-------
 drivers/lightnvm/pblk-init.c     |  23 ++++++--
 drivers/lightnvm/pblk-recovery.c |  14 +++--
 drivers/lightnvm/pblk-rl.c       |   3 +-
 drivers/lightnvm/pblk.h          |   1 +
 5 files changed, 132 insertions(+), 33 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 06ac3f0..9d9a9e2 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -720,13 +720,14 @@ u64 pblk_line_smeta_start(struct pblk *pblk, struct pblk_line *line)
 	return bit * geo->ws_opt;
 }
 
-int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
+static int pblk_line_smeta_read_copy(struct pblk *pblk,
+				     struct pblk_line *line, u64 paddr)
 {
 	struct nvm_tgt_dev *dev = pblk->dev;
+	struct nvm_geo *geo = &dev->geo;
 	struct pblk_line_meta *lm = &pblk->lm;
 	struct bio *bio;
 	struct nvm_rq rqd;
-	u64 paddr = pblk_line_smeta_start(pblk, line);
 	int i, ret;
 
 	memset(&rqd, 0, sizeof(struct nvm_rq));
@@ -749,8 +750,20 @@ int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
 	rqd.nr_ppas = lm->smeta_sec;
 	rqd.is_seq = 1;
 
-	for (i = 0; i < lm->smeta_sec; i++, paddr++)
-		rqd.ppa_list[i] = addr_to_gen_ppa(pblk, paddr, line->id);
+	for (i = 0; i < rqd.nr_ppas; i++, paddr++) {
+		struct ppa_addr ppa = addr_to_gen_ppa(pblk, paddr, line->id);
+		int pos = pblk_ppa_to_pos(geo, ppa);
+
+		while (test_bit(pos, line->blk_bitmap)) {
+			paddr += pblk->min_write_pgs;
+			ppa = addr_to_gen_ppa(pblk, paddr, line->id);
+			pos = pblk_ppa_to_pos(geo, ppa);
+		}
+
+		rqd.ppa_list[i] = ppa;
+		pblk_get_meta(pblk, rqd.meta_list, i)->lba =
+				  cpu_to_le64(ADDR_EMPTY);
+	}
 
 	ret = pblk_submit_io_sync(pblk, &rqd);
 	if (ret) {
@@ -771,16 +784,63 @@ int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
 	return ret;
 }
 
-static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line,
-				 u64 paddr)
+int pblk_line_smeta_read(struct pblk *pblk, struct pblk_line *line)
+{
+	struct pblk_line_meta *lm = &pblk->lm;
+	int i, ret = 0;
+	u64 paddr = pblk_line_smeta_start(pblk, line);
+
+	for (i = 0; i < lm->smeta_copies; i++) {
+		ret = pblk_line_smeta_read_copy(pblk, line,
+						paddr + (i * lm->smeta_sec));
+		if (!ret) {
+			/*
+			 * Just one successfully read copy of smeta is
+			 * enough for us for recovery, don't need to
+			 * read another one.
+			 */
+			return ret;
+		}
+	}
+	return ret;
+}
+
+static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line)
 {
 	struct nvm_tgt_dev *dev = pblk->dev;
+	struct nvm_geo *geo = &dev->geo;
 	struct pblk_line_meta *lm = &pblk->lm;
 	struct bio *bio;
 	struct nvm_rq rqd;
 	__le64 *lba_list = emeta_to_lbas(pblk, line->emeta->buf);
 	__le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
-	int i, ret;
+	u64 paddr = 0;
+	int smeta_wr_len = lm->smeta_len;
+	int smeta_wr_sec = lm->smeta_sec;
+	int i, ret, rq_writes;
+
+	/*
+	 * Check if we can write all the smeta copies with
+	 * a single write command.
+	 * If yes -> copy smeta sector into multiple copies
+	 * in buffer to write.
+	 * If no -> issue writes one by one using the same
+	 * buffer space.
+	 * Only if all the copies are written correctly
+	 * we are treating this line as valid for proper
+	 * UBER reliability.
+	 */
+	if (lm->smeta_sec * lm->smeta_copies > pblk->max_write_pgs) {
+		rq_writes = lm->smeta_copies;
+	} else {
+		rq_writes = 1;
+		for (i = 1; i < lm->smeta_copies; i++) {
+			memcpy(line->smeta + i * lm->smeta_len,
+			       line->smeta, lm->smeta_len);
+		}
+		smeta_wr_len *= lm->smeta_copies;
+		smeta_wr_sec *= lm->smeta_copies;
+	}
 
 	memset(&rqd, 0, sizeof(struct nvm_rq));
 
@@ -788,7 +848,8 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line,
 	if (ret)
 		return ret;
 
-	bio = bio_map_kern(dev->q, line->smeta, lm->smeta_len, GFP_KERNEL);
+next_rq:
+	bio = bio_map_kern(dev->q, line->smeta, smeta_wr_len, GFP_KERNEL);
 	if (IS_ERR(bio)) {
 		ret = PTR_ERR(bio);
 		goto clear_rqd;
@@ -799,15 +860,23 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line,
 
 	rqd.bio = bio;
 	rqd.opcode = NVM_OP_PWRITE;
-	rqd.nr_ppas = lm->smeta_sec;
+	rqd.nr_ppas = smeta_wr_sec;
 	rqd.is_seq = 1;
 
-	for (i = 0; i < lm->smeta_sec; i++, paddr++) {
-		struct pblk_sec_meta *meta = pblk_get_meta(pblk,
-							   rqd.meta_list, i);
+	for (i = 0; i < rqd.nr_ppas; i++, paddr++) {
+		void *meta_list = rqd.meta_list;
+		struct ppa_addr ppa = addr_to_gen_ppa(pblk, paddr, line->id);
+		int pos = pblk_ppa_to_pos(geo, ppa);
 
-		rqd.ppa_list[i] = addr_to_gen_ppa(pblk, paddr, line->id);
-		meta->lba = lba_list[paddr] = addr_empty;
+		while (test_bit(pos, line->blk_bitmap)) {
+			paddr += pblk->min_write_pgs;
+			ppa = addr_to_gen_ppa(pblk, paddr, line->id);
+			pos = pblk_ppa_to_pos(geo, ppa);
+		}
+
+		rqd.ppa_list[i] = ppa;
+		pblk_get_meta(pblk, meta_list, i)->lba = addr_empty;
+		lba_list[paddr] = addr_empty;
 	}
 
 	ret = pblk_submit_io_sync_sem(pblk, &rqd);
@@ -822,8 +891,13 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line,
 	if (rqd.error) {
 		pblk_log_write_err(pblk, &rqd);
 		ret = -EIO;
+		goto clear_rqd;
 	}
 
+	rq_writes--;
+	if (rq_writes > 0)
+		goto next_rq;
+
 clear_rqd:
 	pblk_free_rqd_meta(pblk, &rqd);
 	return ret;
@@ -1020,7 +1094,7 @@ static void pblk_line_setup_metadata(struct pblk_line *line,
 	line->smeta = l_mg->sline_meta[meta_line];
 	line->emeta = l_mg->eline_meta[meta_line];
 
-	memset(line->smeta, 0, lm->smeta_len);
+	memset(line->smeta, 0, lm->smeta_len * lm->smeta_copies);
 	memset(line->emeta->buf, 0, lm->emeta_len[0]);
 
 	line->emeta->mem = 0;
@@ -1147,7 +1221,7 @@ static int pblk_line_init_bb(struct pblk *pblk, struct pblk_line *line,
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	u64 off;
 	int bit = -1;
-	int emeta_secs;
+	int emeta_secs, smeta_secs;
 
 	line->sec_in_line = lm->sec_per_line;
 
@@ -1163,13 +1237,19 @@ static int pblk_line_init_bb(struct pblk *pblk, struct pblk_line *line,
 	}
 
 	/* Mark smeta metadata sectors as bad sectors */
-	bit = find_first_zero_bit(line->blk_bitmap, lm->blk_per_line);
-	off = bit * geo->ws_opt;
-	bitmap_set(line->map_bitmap, off, lm->smeta_sec);
-	line->sec_in_line -= lm->smeta_sec;
-	line->cur_sec = off + lm->smeta_sec;
+	smeta_secs = lm->smeta_sec * lm->smeta_copies;
+	bit = -1;
+	while (smeta_secs) {
+		bit = find_next_zero_bit(line->blk_bitmap, lm->blk_per_line,
+					bit + 1);
+		off = bit * geo->ws_opt;
+		bitmap_set(line->map_bitmap, off, geo->ws_opt);
+		line->cur_sec = off + geo->ws_opt;
+		smeta_secs -= lm->smeta_sec;
+	}
+	line->sec_in_line -= (lm->smeta_sec * lm->smeta_copies);
 
-	if (init && pblk_line_smeta_write(pblk, line, off)) {
+	if (init && pblk_line_smeta_write(pblk, line)) {
 		pblk_debug(pblk, "line smeta I/O failed. Retry\n");
 		return 0;
 	}
diff --git a/drivers/lightnvm/pblk-init.c b/drivers/lightnvm/pblk-init.c
index b351c7f..4f6d214 100644
--- a/drivers/lightnvm/pblk-init.c
+++ b/drivers/lightnvm/pblk-init.c
@@ -27,6 +27,11 @@ static unsigned int write_buffer_size;
 module_param(write_buffer_size, uint, 0644);
 MODULE_PARM_DESC(write_buffer_size, "number of entries in a write buffer");
 
+static unsigned int smeta_copies = 1;
+
+module_param(smeta_copies, int, 0644);
+MODULE_PARM_DESC(smeta_copies, "number of smeta copies");
+
 struct pblk_global_caches {
 	struct kmem_cache	*ws;
 	struct kmem_cache	*rec;
@@ -864,7 +869,8 @@ static int pblk_line_mg_init(struct pblk *pblk)
 	 * emeta depends on the number of LUNs allocated to the pblk instance
 	 */
 	for (i = 0; i < PBLK_DATA_LINES; i++) {
-		l_mg->sline_meta[i] = kmalloc(lm->smeta_len, GFP_KERNEL);
+		l_mg->sline_meta[i] = kmalloc(lm->smeta_len
+						* lm->smeta_copies, GFP_KERNEL);
 		if (!l_mg->sline_meta[i])
 			goto fail_free_smeta;
 	}
@@ -964,6 +970,12 @@ static int pblk_line_meta_init(struct pblk *pblk)
 	lm->mid_thrs = lm->sec_per_line / 2;
 	lm->high_thrs = lm->sec_per_line / 4;
 	lm->meta_distance = (geo->all_luns / 2) * pblk->min_write_pgs;
+	lm->smeta_copies = smeta_copies;
+
+	if (lm->smeta_copies < 1 || lm->smeta_copies > geo->all_luns) {
+		pblk_err(pblk, "unsupported smeta copies parameter\n");
+		return -EINVAL;
+	}
 
 	/* Calculate necessary pages for smeta. See comment over struct
 	 * line_smeta definition
@@ -995,10 +1007,11 @@ static int pblk_line_meta_init(struct pblk *pblk)
 
 	lm->emeta_bb = geo->all_luns > i ? geo->all_luns - i : 0;
 
-	lm->min_blk_line = 1;
-	if (geo->all_luns > 1)
-		lm->min_blk_line += DIV_ROUND_UP(lm->smeta_sec +
-					lm->emeta_sec[0], geo->clba);
+	lm->min_blk_line = lm->smeta_copies;
+	if (geo->all_luns > lm->smeta_copies) {
+		lm->min_blk_line += DIV_ROUND_UP((lm->smeta_sec
+			* lm->smeta_copies) + lm->emeta_sec[0], geo->clba);
+	}
 
 	if (lm->min_blk_line > lm->blk_per_line) {
 		pblk_err(pblk, "config. not supported. Min. LUN in line:%d\n",
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index a9085b0..9a992af 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -51,7 +51,8 @@ static int pblk_recov_l2p_from_emeta(struct pblk *pblk, struct pblk_line *line)
 	if (!lba_list)
 		return 1;
 
-	data_start = pblk_line_smeta_start(pblk, line) + lm->smeta_sec;
+	data_start = pblk_line_smeta_start(pblk, line)
+					+ (lm->smeta_sec * lm->smeta_copies);
 	data_end = line->emeta_ssec;
 	nr_valid_lbas = le64_to_cpu(emeta_buf->nr_valid_lbas);
 
@@ -134,7 +135,8 @@ static u64 pblk_sec_in_open_line(struct pblk *pblk, struct pblk_line *line)
 	if (lm->blk_per_line - nr_bb != valid_chunks)
 		pblk_err(pblk, "recovery line %d is bad\n", line->id);
 
-	pblk_update_line_wp(pblk, line, written_secs - lm->smeta_sec);
+	pblk_update_line_wp(pblk, line, written_secs -
+					(lm->smeta_sec * lm->smeta_copies));
 
 	return written_secs;
 }
@@ -377,12 +379,14 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
 	void *data;
 	dma_addr_t dma_ppa_list, dma_meta_list;
 	__le64 *lba_list;
-	u64 paddr = pblk_line_smeta_start(pblk, line) + lm->smeta_sec;
+	u64 paddr = pblk_line_smeta_start(pblk, line) +
+					(lm->smeta_sec * lm->smeta_copies);
 	bool padded = false;
 	int rq_ppas, rq_len;
 	int i, j;
 	int ret;
-	u64 left_ppas = pblk_sec_in_open_line(pblk, line) - lm->smeta_sec;
+	u64 left_ppas = pblk_sec_in_open_line(pblk, line) -
+					(lm->smeta_sec * lm->smeta_copies);
 
 	if (pblk_line_wps_are_unbalanced(pblk, line))
 		pblk_warn(pblk, "recovering unbalanced line (%d)\n", line->id);
@@ -706,7 +710,7 @@ struct pblk_line *pblk_recov_l2p(struct pblk *pblk)
 
 		line = &pblk->lines[i];
 
-		memset(smeta, 0, lm->smeta_len);
+		memset(smeta, 0, lm->smeta_len * lm->smeta_copies);
 		line->smeta = smeta;
 		line->lun_bitmap = ((void *)(smeta_buf)) +
 						sizeof(struct line_smeta);
diff --git a/drivers/lightnvm/pblk-rl.c b/drivers/lightnvm/pblk-rl.c
index b014957..944372c 100644
--- a/drivers/lightnvm/pblk-rl.c
+++ b/drivers/lightnvm/pblk-rl.c
@@ -218,7 +218,8 @@ void pblk_rl_init(struct pblk_rl *rl, int budget, int threshold)
 	unsigned int rb_windows;
 
 	/* Consider sectors used for metadata */
-	sec_meta = (lm->smeta_sec + lm->emeta_sec[0]) * l_mg->nr_free_lines;
+	sec_meta = ((lm->smeta_sec * lm->smeta_copies)
+			+ lm->emeta_sec[0]) * l_mg->nr_free_lines;
 	blk_meta = DIV_ROUND_UP(sec_meta, geo->clba);
 
 	rl->high = pblk->op_blks - blk_meta - lm->blk_per_line;
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 3a84c8a..0999245 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -547,6 +547,7 @@ struct pblk_line_mgmt {
 struct pblk_line_meta {
 	unsigned int smeta_len;		/* Total length for smeta */
 	unsigned int smeta_sec;		/* Sectors needed for smeta */
+	unsigned int smeta_copies;	/* Number of smeta copies */
 
 	unsigned int emeta_len[4];	/* Lengths for emeta:
 					 *  [0]: Total
-- 
2.9.5

[PATCH v3 08/10] lightnvm: pblk: GC error handling

[Igor Konopko]

Currently when there is an IO error (or similar) on GC read path, pblk
still move the line, which was currently under GC process to free state.
Such a behaviour can lead to silent data mismatch issue.

With this patch, the line which was under GC process on which some IO
errors occurred, will be putted back to closed state (instead of free
state as it was without this patch) and the L2P mapping for such a
failed sectors will not be updated.

Then in case of any user IOs to such a failed sectors, pblk would be
able to return at least real IO error instead of stale data as it is
right now.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
---
 drivers/lightnvm/pblk-core.c | 8 ++++++++
 drivers/lightnvm/pblk-gc.c   | 5 ++---
 drivers/lightnvm/pblk-read.c | 1 -
 drivers/lightnvm/pblk.h      | 2 ++
 4 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 9d9a9e2..1b88e5e 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -1799,6 +1799,14 @@ static void __pblk_line_put(struct pblk *pblk, struct pblk_line *line)
 
 	spin_lock(&line->lock);
 	WARN_ON(line->state != PBLK_LINESTATE_GC);
+	if (line->w_err_gc->has_gc_err) {
+		spin_unlock(&line->lock);
+		pblk_err(pblk, "line %d had errors during GC\n", line->id);
+		pblk_put_line_back(pblk, line);
+		line->w_err_gc->has_gc_err = 0;
+		return;
+	}
+
 	line->state = PBLK_LINESTATE_FREE;
 	trace_pblk_line_state(pblk_disk_name(pblk), line->id,
 					line->state);
diff --git a/drivers/lightnvm/pblk-gc.c b/drivers/lightnvm/pblk-gc.c
index e23b192..63ee205 100644
--- a/drivers/lightnvm/pblk-gc.c
+++ b/drivers/lightnvm/pblk-gc.c
@@ -59,7 +59,7 @@ static void pblk_gc_writer_kick(struct pblk_gc *gc)
 	wake_up_process(gc->gc_writer_ts);
 }
 
-static void pblk_put_line_back(struct pblk *pblk, struct pblk_line *line)
+void pblk_put_line_back(struct pblk *pblk, struct pblk_line *line)
 {
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	struct list_head *move_list;
@@ -98,8 +98,7 @@ static void pblk_gc_line_ws(struct work_struct *work)
 	/* Read from GC victim block */
 	ret = pblk_submit_read_gc(pblk, gc_rq);
 	if (ret) {
-		pblk_err(pblk, "failed GC read in line:%d (err:%d)\n",
-								line->id, ret);
+		line->w_err_gc->has_gc_err = 1;
 		goto out;
 	}
 
diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 264d1d7..5481dfa 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -494,7 +494,6 @@ int pblk_submit_read_gc(struct pblk *pblk, struct pblk_gc_rq *gc_rq)
 
 	if (pblk_submit_io_sync(pblk, &rqd)) {
 		ret = -EIO;
-		pblk_err(pblk, "GC read request failed\n");
 		goto err_free_bio;
 	}
 
diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index 0999245..0bc4255 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -425,6 +425,7 @@ struct pblk_smeta {
 
 struct pblk_w_err_gc {
 	int has_write_err;
+	int has_gc_err;
 	__le64 *lba_list;
 };
 
@@ -916,6 +917,7 @@ void pblk_gc_free_full_lines(struct pblk *pblk);
 void pblk_gc_sysfs_state_show(struct pblk *pblk, int *gc_enabled,
 			      int *gc_active);
 int pblk_gc_sysfs_force(struct pblk *pblk, int force);
+void pblk_put_line_back(struct pblk *pblk, struct pblk_line *line);
 
 /*
  * pblk rate limiter
-- 
2.9.5

[PATCH v3 09/10] lightnvm: pblk: use nvm_rq_to_ppa_list()

[Igor Konopko]

This patch replaces few remaining usages of rqd->ppa_list[] with
existing nvm_rq_to_ppa_list() helpers. This is needed for theoretical
devices with ws_min/ws_opt equal to 1.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
Reviewed-by: Javier González <javier@javigon.com>
---
 drivers/lightnvm/pblk-core.c     | 26 ++++++++++++++------------
 drivers/lightnvm/pblk-recovery.c | 13 ++++++++-----
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 1b88e5e..cd99c8c 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -562,11 +562,9 @@ int pblk_submit_io_sync(struct pblk *pblk, struct nvm_rq *rqd)
 
 int pblk_submit_io_sync_sem(struct pblk *pblk, struct nvm_rq *rqd)
 {
-	struct ppa_addr *ppa_list;
+	struct ppa_addr *ppa_list = nvm_rq_to_ppa_list(rqd);
 	int ret;
 
-	ppa_list = (rqd->nr_ppas > 1) ? rqd->ppa_list : &rqd->ppa_addr;
-
 	pblk_down_chunk(pblk, ppa_list[0]);
 	ret = pblk_submit_io_sync(pblk, rqd);
 	pblk_up_chunk(pblk, ppa_list[0]);
@@ -727,6 +725,7 @@ static int pblk_line_smeta_read_copy(struct pblk *pblk,
 	struct nvm_geo *geo = &dev->geo;
 	struct pblk_line_meta *lm = &pblk->lm;
 	struct bio *bio;
+	struct ppa_addr *ppa_list;
 	struct nvm_rq rqd;
 	int i, ret;
 
@@ -749,6 +748,7 @@ static int pblk_line_smeta_read_copy(struct pblk *pblk,
 	rqd.opcode = NVM_OP_PREAD;
 	rqd.nr_ppas = lm->smeta_sec;
 	rqd.is_seq = 1;
+	ppa_list = nvm_rq_to_ppa_list(&rqd);
 
 	for (i = 0; i < rqd.nr_ppas; i++, paddr++) {
 		struct ppa_addr ppa = addr_to_gen_ppa(pblk, paddr, line->id);
@@ -760,7 +760,7 @@ static int pblk_line_smeta_read_copy(struct pblk *pblk,
 			pos = pblk_ppa_to_pos(geo, ppa);
 		}
 
-		rqd.ppa_list[i] = ppa;
+		ppa_list[i] = ppa;
 		pblk_get_meta(pblk, rqd.meta_list, i)->lba =
 				  cpu_to_le64(ADDR_EMPTY);
 	}
@@ -811,6 +811,7 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line)
 	struct nvm_geo *geo = &dev->geo;
 	struct pblk_line_meta *lm = &pblk->lm;
 	struct bio *bio;
+	struct ppa_addr *ppa_list;
 	struct nvm_rq rqd;
 	__le64 *lba_list = emeta_to_lbas(pblk, line->emeta->buf);
 	__le64 addr_empty = cpu_to_le64(ADDR_EMPTY);
@@ -862,6 +863,7 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line)
 	rqd.opcode = NVM_OP_PWRITE;
 	rqd.nr_ppas = smeta_wr_sec;
 	rqd.is_seq = 1;
+	ppa_list = nvm_rq_to_ppa_list(&rqd);
 
 	for (i = 0; i < rqd.nr_ppas; i++, paddr++) {
 		void *meta_list = rqd.meta_list;
@@ -874,7 +876,7 @@ static int pblk_line_smeta_write(struct pblk *pblk, struct pblk_line *line)
 			pos = pblk_ppa_to_pos(geo, ppa);
 		}
 
-		rqd.ppa_list[i] = ppa;
+		ppa_list[i] = ppa;
 		pblk_get_meta(pblk, meta_list, i)->lba = addr_empty;
 		lba_list[paddr] = addr_empty;
 	}
@@ -910,8 +912,9 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 	struct nvm_geo *geo = &dev->geo;
 	struct pblk_line_mgmt *l_mg = &pblk->l_mg;
 	struct pblk_line_meta *lm = &pblk->lm;
-	void *ppa_list, *meta_list;
+	void *ppa_list_buf, *meta_list;
 	struct bio *bio;
+	struct ppa_addr *ppa_list;
 	struct nvm_rq rqd;
 	u64 paddr = line->emeta_ssec;
 	dma_addr_t dma_ppa_list, dma_meta_list;
@@ -927,7 +930,7 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 	if (!meta_list)
 		return -ENOMEM;
 
-	ppa_list = meta_list + pblk_dma_meta_size(pblk);
+	ppa_list_buf = meta_list + pblk_dma_meta_size(pblk);
 	dma_ppa_list = dma_meta_list + pblk_dma_meta_size(pblk);
 
 next_rq:
@@ -948,11 +951,12 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 
 	rqd.bio = bio;
 	rqd.meta_list = meta_list;
-	rqd.ppa_list = ppa_list;
+	rqd.ppa_list = ppa_list_buf;
 	rqd.dma_meta_list = dma_meta_list;
 	rqd.dma_ppa_list = dma_ppa_list;
 	rqd.opcode = NVM_OP_PREAD;
 	rqd.nr_ppas = rq_ppas;
+	ppa_list = nvm_rq_to_ppa_list(&rqd);
 
 	for (i = 0; i < rqd.nr_ppas; ) {
 		struct ppa_addr ppa = addr_to_gen_ppa(pblk, paddr, line_id);
@@ -980,7 +984,7 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 		}
 
 		for (j = 0; j < min; j++, i++, paddr++)
-			rqd.ppa_list[i] = addr_to_gen_ppa(pblk, paddr, line_id);
+			ppa_list[i] = addr_to_gen_ppa(pblk, paddr, line_id);
 	}
 
 	ret = pblk_submit_io_sync(pblk, &rqd);
@@ -1605,11 +1609,9 @@ void pblk_ppa_to_line_put(struct pblk *pblk, struct ppa_addr ppa)
 
 void pblk_rq_to_line_put(struct pblk *pblk, struct nvm_rq *rqd)
 {
-	struct ppa_addr *ppa_list;
+	struct ppa_addr *ppa_list = nvm_rq_to_ppa_list(rqd);
 	int i;
 
-	ppa_list = (rqd->nr_ppas > 1) ? rqd->ppa_list : &rqd->ppa_addr;
-
 	for (i = 0; i < rqd->nr_ppas; i++)
 		pblk_ppa_to_line_put(pblk, ppa_list[i]);
 }
diff --git a/drivers/lightnvm/pblk-recovery.c b/drivers/lightnvm/pblk-recovery.c
index 9a992af..2fca21e 100644
--- a/drivers/lightnvm/pblk-recovery.c
+++ b/drivers/lightnvm/pblk-recovery.c
@@ -181,6 +181,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	struct pblk_pad_rq *pad_rq;
 	struct nvm_rq *rqd;
 	struct bio *bio;
+	struct ppa_addr *ppa_list;
 	void *data;
 	__le64 *lba_list = emeta_to_lbas(pblk, line->emeta->buf);
 	u64 w_ptr = line->cur_sec;
@@ -241,6 +242,7 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 	rqd->end_io = pblk_end_io_recov;
 	rqd->private = pad_rq;
 
+	ppa_list = nvm_rq_to_ppa_list(rqd);
 	meta_list = rqd->meta_list;
 
 	for (i = 0; i < rqd->nr_ppas; ) {
@@ -268,17 +270,17 @@ static int pblk_recov_pad_line(struct pblk *pblk, struct pblk_line *line,
 			lba_list[w_ptr] = addr_empty;
 			meta = pblk_get_meta(pblk, meta_list, i);
 			meta->lba = addr_empty;
-			rqd->ppa_list[i] = dev_ppa;
+			ppa_list[i] = dev_ppa;
 		}
 	}
 
 	kref_get(&pad_rq->ref);
-	pblk_down_chunk(pblk, rqd->ppa_list[0]);
+	pblk_down_chunk(pblk, ppa_list[0]);
 
 	ret = pblk_submit_io(pblk, rqd);
 	if (ret) {
 		pblk_err(pblk, "I/O submission failed: %d\n", ret);
-		pblk_up_chunk(pblk, rqd->ppa_list[0]);
+		pblk_up_chunk(pblk, ppa_list[0]);
 		kref_put(&pad_rq->ref, pblk_recov_complete);
 		pblk_free_rqd(pblk, rqd, PBLK_WRITE_INT);
 		bio_put(bio);
@@ -424,6 +426,7 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
 	rqd->ppa_list = ppa_list;
 	rqd->dma_ppa_list = dma_ppa_list;
 	rqd->dma_meta_list = dma_meta_list;
+	ppa_list = nvm_rq_to_ppa_list(rqd);
 
 	if (pblk_io_aligned(pblk, rq_ppas))
 		rqd->is_seq = 1;
@@ -442,7 +445,7 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
 		}
 
 		for (j = 0; j < pblk->min_write_pgs; j++, i++)
-			rqd->ppa_list[i] =
+			ppa_list[i] =
 				addr_to_gen_ppa(pblk, paddr + j, line->id);
 	}
 
@@ -490,7 +493,7 @@ static int pblk_recov_scan_oob(struct pblk *pblk, struct pblk_line *line,
 			continue;
 
 		line->nr_valid_lbas++;
-		pblk_update_map(pblk, lba, rqd->ppa_list[i]);
+		pblk_update_map(pblk, lba, ppa_list[i]);
 	}
 
 	left_ppas -= rq_ppas;
-- 
2.9.5

[PATCH v3 10/10] lightnvm: track inflight target creations

[Igor Konopko]

This patch adds a counter, which is responsible for tracking inflight
target creations.

When creation process is still in progress, target is not yet on
targets list. This causes a chance for removing whole lightnvm
subsystem by calling nvm_unregister() in the meantime and finally by
causing kernel panic inside target init function.

With this patch we are able to track such a scenarios and wait with
completing nvm_unregister() and freeing memory until target creation
will be completed.

Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
---
 drivers/lightnvm/core.c  | 19 ++++++++++++++++++-
 include/linux/lightnvm.h |  2 ++
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index e2abe88..62ed662 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -1083,6 +1083,7 @@ static int nvm_core_init(struct nvm_dev *dev)
 	INIT_LIST_HEAD(&dev->targets);
 	mutex_init(&dev->mlock);
 	spin_lock_init(&dev->lock);
+	dev->create_inflight = 0;
 
 	ret = nvm_register_map(dev);
 	if (ret)
@@ -1180,6 +1181,11 @@ void nvm_unregister(struct nvm_dev *dev)
 	struct nvm_target *t, *tmp;
 
 	mutex_lock(&dev->mlock);
+	while (dev->create_inflight > 0) {
+		mutex_unlock(&dev->mlock);
+		io_schedule();
+		mutex_lock(&dev->mlock);
+	}
 	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
 		if (t->dev->parent != dev)
 			continue;
@@ -1198,6 +1204,7 @@ EXPORT_SYMBOL(nvm_unregister);
 static int __nvm_configure_create(struct nvm_ioctl_create *create)
 {
 	struct nvm_dev *dev;
+	int ret;
 
 	down_write(&nvm_lock);
 	dev = nvm_find_nvm_dev(create->dev);
@@ -1208,7 +1215,17 @@ static int __nvm_configure_create(struct nvm_ioctl_create *create)
 		return -EINVAL;
 	}
 
-	return nvm_create_tgt(dev, create);
+	mutex_lock(&dev->mlock);
+	dev->create_inflight++;
+	mutex_unlock(&dev->mlock);
+
+	ret = nvm_create_tgt(dev, create);
+
+	mutex_lock(&dev->mlock);
+	dev->create_inflight--;
+	mutex_unlock(&dev->mlock);
+
+	return ret;
 }
 
 static long nvm_ioctl_info(struct file *file, void __user *arg)
diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
index d3b0270..e462d1d 100644
--- a/include/linux/lightnvm.h
+++ b/include/linux/lightnvm.h
@@ -428,6 +428,8 @@ struct nvm_dev {
 	char name[DISK_NAME_LEN];
 	void *private_data;
 
+	int create_inflight;
+
 	void *rmap;
 
 	struct mutex mlock;
-- 
2.9.5

Re: [PATCH v3 10/10] lightnvm: track inflight target creations

[Matias Bjørling]

On 3/27/19 1:39 PM, Igor Konopko wrote:
> This patch adds a counter, which is responsible for tracking inflight
> target creations.
> 
> When creation process is still in progress, target is not yet on
> targets list. This causes a chance for removing whole lightnvm
> subsystem by calling nvm_unregister() in the meantime and finally by
> causing kernel panic inside target init function.
> 
> With this patch we are able to track such a scenarios and wait with
> completing nvm_unregister() and freeing memory until target creation
> will be completed.
> 
> Signed-off-by: Igor Konopko <igor.j.konopko@intel.com>
> ---
>   drivers/lightnvm/core.c  | 19 ++++++++++++++++++-
>   include/linux/lightnvm.h |  2 ++
>   2 files changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index e2abe88..62ed662 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -1083,6 +1083,7 @@ static int nvm_core_init(struct nvm_dev *dev)
>   	INIT_LIST_HEAD(&dev->targets);
>   	mutex_init(&dev->mlock);
>   	spin_lock_init(&dev->lock);
> +	dev->create_inflight = 0;
>   
>   	ret = nvm_register_map(dev);
>   	if (ret)
> @@ -1180,6 +1181,11 @@ void nvm_unregister(struct nvm_dev *dev)
>   	struct nvm_target *t, *tmp;
>   
>   	mutex_lock(&dev->mlock);
> +	while (dev->create_inflight > 0) {
> +		mutex_unlock(&dev->mlock);
> +		io_schedule();
> +		mutex_lock(&dev->mlock);
> +	}
>   	list_for_each_entry_safe(t, tmp, &dev->targets, list) {
>   		if (t->dev->parent != dev)
>   			continue;
> @@ -1198,6 +1204,7 @@ EXPORT_SYMBOL(nvm_unregister);
>   static int __nvm_configure_create(struct nvm_ioctl_create *create)
>   {
>   	struct nvm_dev *dev;
> +	int ret;
>   
>   	down_write(&nvm_lock);
>   	dev = nvm_find_nvm_dev(create->dev);
> @@ -1208,7 +1215,17 @@ static int __nvm_configure_create(struct nvm_ioctl_create *create)
>   		return -EINVAL;
>   	}
>   
> -	return nvm_create_tgt(dev, create);
> +	mutex_lock(&dev->mlock);
> +	dev->create_inflight++;
> +	mutex_unlock(&dev->mlock);
> +
> +	ret = nvm_create_tgt(dev, create);
> +
> +	mutex_lock(&dev->mlock);
> +	dev->create_inflight--;
> +	mutex_unlock(&dev->mlock);
> +
> +	return ret;
>   }
>   
>   static long nvm_ioctl_info(struct file *file, void __user *arg)
> diff --git a/include/linux/lightnvm.h b/include/linux/lightnvm.h
> index d3b0270..e462d1d 100644
> --- a/include/linux/lightnvm.h
> +++ b/include/linux/lightnvm.h
> @@ -428,6 +428,8 @@ struct nvm_dev {
>   	char name[DISK_NAME_LEN];
>   	void *private_data;
>   
> +	int create_inflight;
> +
>   	void *rmap;
>   
>   	struct mutex mlock;
> 

Hi Igor,

I think this may be better implemented with kref. Such that target 
creator can take a reference upon target initialize and release it again 
afterwards. Then the one turning of the lights can get to run the final 
cleanup routine.

-Matias

Re: [PATCH v3 00/10] lightnvm: next set of improvements for 5.2

[Hans Holmberg]

On Wed, Mar 27, 2019 at 1:41 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>
> This is another set of fixes and improvements to both pblk and lightnvm
> core.
>
> First & second patches are the most crutial, since they changes the approach
> to the partial read path, so detailed review is needed especially here.
>
> Other patches are my other findings related to some bugs or potential
> improvements, mostly related to some corner cases.
>
> Changes v2 -> v3;
> -dropped some not needed patches
> -dropped patches which were already pulled into for-5.2/core branch
> -commit messages cleanup
>
> Changes v1 -> v2:
> -dropped some not needed patches
> -review feedback incorporated for some of the patches
> -partial read path changes patch splitted into two patches
>

Hi Igor,

I ran the quick xfstests and generic/247 hangs with this patchset on qemu:

FSTYP         -- xfs (non-debug)
PLATFORM      -- Linux/x86_64 u1804-qatd 5.0.0-rc6_for-5.2/core_igor_v3

generic/247 95s ...
<hang>

Note: If i run it with Matias 5.2 core branch with my biovec fix, it
completes successfully, but with this in dmesg:

[  358.028158] Page cache invalidation failure on direct I/O.
Possible data corruption due to collision with buffered I/O!
[  358.030768] File: /mnt/247 PID: 4431 Comm: dd

(I have not looked into that any further)

Best regards,
Hans


> Igor Konopko (10):
>   lightnvm: pblk: IO path reorganization
>   lightnvm: pblk: simplify partial read path
>   lightnvm: pblk: propagate errors when reading meta
>   lightnvm: pblk: recover only written metadata
>   lightnvm: pblk: wait for inflight IOs in recovery
>   lightnvm: pblk: remove internal IO timeout
>   lightnvm: pblk: store multiple copies of smeta
>   lightnvm: pblk: GC error handling
>   lightnvm: pblk: use nvm_rq_to_ppa_list()
>   lightnvm: track inflight target creations
>
>  drivers/lightnvm/core.c          |  19 ++-
>  drivers/lightnvm/pblk-cache.c    |   8 +-
>  drivers/lightnvm/pblk-core.c     | 179 +++++++++++++++++----
>  drivers/lightnvm/pblk-gc.c       |   5 +-
>  drivers/lightnvm/pblk-init.c     |  37 +++--
>  drivers/lightnvm/pblk-read.c     | 336 +++++++++++----------------------------
>  drivers/lightnvm/pblk-recovery.c |  69 ++++----
>  drivers/lightnvm/pblk-rl.c       |   3 +-
>  drivers/lightnvm/pblk.h          |  31 ++--
>  include/linux/lightnvm.h         |   2 +
>  10 files changed, 343 insertions(+), 346 deletions(-)
>
> --
> 2.9.5
>

Re: [PATCH v3 00/10] lightnvm: next set of improvements for 5.2

[Igor Konopko]

On 03.04.2019 09:12, Hans Holmberg wrote:
> On Wed, Mar 27, 2019 at 1:41 PM Igor Konopko <igor.j.konopko@intel.com> wrote:
>>
>> This is another set of fixes and improvements to both pblk and lightnvm
>> core.
>>
>> First & second patches are the most crutial, since they changes the approach
>> to the partial read path, so detailed review is needed especially here.
>>
>> Other patches are my other findings related to some bugs or potential
>> improvements, mostly related to some corner cases.
>>
>> Changes v2 -> v3;
>> -dropped some not needed patches
>> -dropped patches which were already pulled into for-5.2/core branch
>> -commit messages cleanup
>>
>> Changes v1 -> v2:
>> -dropped some not needed patches
>> -review feedback incorporated for some of the patches
>> -partial read path changes patch splitted into two patches
>>
> 
> Hi Igor,
> 
> I ran the quick xfstests and generic/247 hangs with this patchset on qemu:
> 
> FSTYP         -- xfs (non-debug)
> PLATFORM      -- Linux/x86_64 u1804-qatd 5.0.0-rc6_for-5.2/core_igor_v3
> 
> generic/247 95s ...
> <hang>
> 
> Note: If i run it with Matias 5.2 core branch with my biovec fix, it
> completes successfully, but with this in dmesg:
> 
> [  358.028158] Page cache invalidation failure on direct I/O.
> Possible data corruption due to collision with buffered I/O!
> [  358.030768] File: /mnt/247 PID: 4431 Comm: dd
> 
> (I have not looked into that any further)
> 

Thanks Hans for testing that and reporting back. Will definitely look 
into that and resubmit when fixed.
Igor

> Best regards,
> Hans
> 
> 
>> Igor Konopko (10):
>>    lightnvm: pblk: IO path reorganization
>>    lightnvm: pblk: simplify partial read path
>>    lightnvm: pblk: propagate errors when reading meta
>>    lightnvm: pblk: recover only written metadata
>>    lightnvm: pblk: wait for inflight IOs in recovery
>>    lightnvm: pblk: remove internal IO timeout
>>    lightnvm: pblk: store multiple copies of smeta
>>    lightnvm: pblk: GC error handling
>>    lightnvm: pblk: use nvm_rq_to_ppa_list()
>>    lightnvm: track inflight target creations
>>
>>   drivers/lightnvm/core.c          |  19 ++-
>>   drivers/lightnvm/pblk-cache.c    |   8 +-
>>   drivers/lightnvm/pblk-core.c     | 179 +++++++++++++++++----
>>   drivers/lightnvm/pblk-gc.c       |   5 +-
>>   drivers/lightnvm/pblk-init.c     |  37 +++--
>>   drivers/lightnvm/pblk-read.c     | 336 +++++++++++----------------------------
>>   drivers/lightnvm/pblk-recovery.c |  69 ++++----
>>   drivers/lightnvm/pblk-rl.c       |   3 +-
>>   drivers/lightnvm/pblk.h          |  31 ++--
>>   include/linux/lightnvm.h         |   2 +
>>   10 files changed, 343 insertions(+), 346 deletions(-)
>>
>> --
>> 2.9.5
>>

Re: [PATCH v3 00/10] lightnvm: next set of improvements for 5.2

[Matias Bjørling]

On 3/27/19 1:38 PM, Igor Konopko wrote:
> This is another set of fixes and improvements to both pblk and lightnvm
> core.
> 
> First & second patches are the most crutial, since they changes the approach
> to the partial read path, so detailed review is needed especially here.
> 
> Other patches are my other findings related to some bugs or potential
> improvements, mostly related to some corner cases.
> 
> Changes v2 -> v3;
> -dropped some not needed patches
> -dropped patches which were already pulled into for-5.2/core branch
> -commit messages cleanup
> 
> Changes v1 -> v2:
> -dropped some not needed patches
> -review feedback incorporated for some of the patches
> -partial read path changes patch splitted into two patches
> 
> Igor Konopko (10):
>    lightnvm: pblk: IO path reorganization
>    lightnvm: pblk: simplify partial read path
>    lightnvm: pblk: propagate errors when reading meta
>    lightnvm: pblk: recover only written metadata
>    lightnvm: pblk: wait for inflight IOs in recovery
>    lightnvm: pblk: remove internal IO timeout
>    lightnvm: pblk: store multiple copies of smeta
>    lightnvm: pblk: GC error handling
>    lightnvm: pblk: use nvm_rq_to_ppa_list()
>    lightnvm: track inflight target creations
> 
>   drivers/lightnvm/core.c          |  19 ++-
>   drivers/lightnvm/pblk-cache.c    |   8 +-
>   drivers/lightnvm/pblk-core.c     | 179 +++++++++++++++++----
>   drivers/lightnvm/pblk-gc.c       |   5 +-
>   drivers/lightnvm/pblk-init.c     |  37 +++--
>   drivers/lightnvm/pblk-read.c     | 336 +++++++++++----------------------------
>   drivers/lightnvm/pblk-recovery.c |  69 ++++----
>   drivers/lightnvm/pblk-rl.c       |   3 +-
>   drivers/lightnvm/pblk.h          |  31 ++--
>   include/linux/lightnvm.h         |   2 +
>   10 files changed, 343 insertions(+), 346 deletions(-)
> 

Thanks Igor. I've picked up 5,6, and 8.

Hans will be resend his previous patch to fix the bvec bug. Then we have 
time to get to the right solution without the stress of the -rc window.