[PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[Javier González]

Matias,

This patchset implements support for retrieving chunk metadata when
submitting a reset/erase command. Patches 0 and 1 are small fixes that
can be directly merged into your patch:
  
  lightnvm: move bad block and chunk state logic to core

Also, note that these do not apply on top of your for-4.20/core due them
depending on patches that I sent before that you have not picked up yet.
You can see them though in for-4.20/pblk. I'll rebase as patches in the
list appear in your tree.

Thanks,
Javier

Javier González (4):
  lightnvm: use right address format on 1.2 path
  lightnvm: assign block address before slba
  lightnvm: pblk: add helper for printing chunk state
  lightnvm: pblk: retrieve chunk metadata on erase

 drivers/lightnvm/core.c      | 51 +++++++++++++++++++++++++++++++++++++++-----
 drivers/lightnvm/pblk-core.c | 47 ++++++++++++++++++++++++++++++----------
 drivers/lightnvm/pblk.h      |  9 ++++++++
 3 files changed, 91 insertions(+), 16 deletions(-)

-- 
2.7.4

[PATCH 1/4] lightnvm: use right address format on 1.2 path

[Javier González]

struct ppa_addr defines different address formats. When we are in the
1.2 path, use 1.2 addressing, even when the generic format works too.
This improves readability.

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

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index f7154398ba38..e9f14c67f4f3 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -998,8 +998,8 @@ static int nvm_get_bb_meta(struct nvm_dev *dev, sector_t slba,
 				goto done;
 
 			ppa_gen.ppa = 0;
-			ppa_gen.a.ch = ch;
-			ppa_gen.a.lun = lun;
+			ppa_gen.g.ch = ch;
+			ppa_gen.g.lun = lun;
 			ppa_dev = generic_to_dev_addr(dev, ppa_gen);
 
 			ret = dev->ops->get_bb_tbl(dev, ppa_dev, blks);
-- 
2.7.4

[PATCH 2/4] lightnvm: assign block address before slba

[Javier González]

In 1.2, the chunk slba is set to the physical representation of the
block. Thus, assigning the block to the ppa must occur before the slba
is assign.

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/lightnvm/core.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index e9f14c67f4f3..efb976a863d2 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -945,6 +945,8 @@ static int nvm_bb_to_chunk(struct nvm_dev *dev, struct ppa_addr ppa,
 			}
 		}
 
+		ppa.g.blk = blk;
+
 		meta->wp = 0;
 		meta->type = NVM_CHK_TP_W_SEQ;
 		meta->wi = 0;
@@ -952,7 +954,6 @@ static int nvm_bb_to_chunk(struct nvm_dev *dev, struct ppa_addr ppa,
 		meta->cnlb = dev->geo.clba;
 
 		if (blktype == NVM_BLK_T_FREE) {
-			ppa.a.blk = blk;
 			ret = nvm_bb_chunk_scan(dev, ppa, meta);
 			if (ret)
 				return ret;
-- 
2.7.4

[PATCH 3/4] lightnvm: pblk: add helper for printing chunk state

[Javier González]

Implement pblk helper for printing chunk state.

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

diff --git a/drivers/lightnvm/pblk.h b/drivers/lightnvm/pblk.h
index bc852bb9e3f0..5c35dcd57395 100644
--- a/drivers/lightnvm/pblk.h
+++ b/drivers/lightnvm/pblk.h
@@ -1239,6 +1239,15 @@ static inline int pblk_io_aligned(struct pblk *pblk, int nr_secs)
 	return !(nr_secs % pblk->min_write_pgs);
 }
 
+static inline void print_chunk(struct pblk *pblk, struct nvm_chk_meta *chk,
+			       char *msg, int error)
+{
+	pblk_err(pblk, "chunk: (%s: %x) s:%d,t:%d,wi:%d,slba:%llu,cnlb:%llu,wp:%llu\n",
+			msg, error,
+			chk->state, chk->type, chk->wi,
+			chk->slba, chk->cnlb, chk->wp);
+}
+
 #ifdef CONFIG_NVM_PBLK_DEBUG
 static inline void print_ppa(struct pblk *pblk, struct ppa_addr *p,
 			     char *msg, int error)
-- 
2.7.4

[PATCH 4/4] lightnvm: pblk: retrieve chunk metadata on erase

[Javier González]

On the OCSSD 2.0 spec, the device populates the metadata pointer (if
provided) when a chunk is reset. Implement this path in pblk and use it
for sanity chunk checks.

For 1.2, reset the write pointer and the state on core so that the erase
path is transparent to pblk wrt OCSSD version.

Signed-off-by: Javier González <javier@cnexlabs.com>
---
 drivers/lightnvm/core.c      | 44 +++++++++++++++++++++++++++++++++++++++--
 drivers/lightnvm/pblk-core.c | 47 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
index efb976a863d2..dceaae4e795f 100644
--- a/drivers/lightnvm/core.c
+++ b/drivers/lightnvm/core.c
@@ -750,9 +750,40 @@ int nvm_submit_io(struct nvm_tgt_dev *tgt_dev, struct nvm_rq *rqd)
 }
 EXPORT_SYMBOL(nvm_submit_io);
 
+/* Take only addresses in generic format */
+static void nvm_set_chunk_state_12(struct nvm_dev *dev, struct nvm_rq *rqd)
+{
+	struct ppa_addr *ppa_list = nvm_rq_to_ppa_list(rqd);
+	int i;
+
+	for (i = 0; i < rqd->nr_ppas; i++) {
+		struct ppa_addr ppa;
+		struct nvm_chk_meta *chunk;
+
+		chunk = ((struct nvm_chk_meta *)rqd->meta_list) + i;
+
+		if (rqd->error)
+			chunk->state = NVM_CHK_ST_OFFLINE;
+		else
+			chunk->state = NVM_CHK_ST_FREE;
+
+		chunk->wp = 0;
+		chunk->wi = 0;
+		chunk->type = NVM_CHK_TP_W_SEQ;
+		chunk->cnlb = dev->geo.clba;
+
+		/* recalculate slba for the chunk */
+		ppa = ppa_list[i];
+		ppa.g.pg = ppa.g.pl = ppa.g.sec = 0;
+
+		chunk->slba = generic_to_dev_addr(dev, ppa).ppa;
+	}
+}
+
 int nvm_submit_io_sync(struct nvm_tgt_dev *tgt_dev, struct nvm_rq *rqd)
 {
 	struct nvm_dev *dev = tgt_dev->parent;
+	struct nvm_geo *geo = &dev->geo;
 	int ret;
 
 	if (!dev->ops->submit_io_sync)
@@ -765,8 +796,12 @@ int nvm_submit_io_sync(struct nvm_tgt_dev *tgt_dev, struct nvm_rq *rqd)
 
 	/* In case of error, fail with right address format */
 	ret = dev->ops->submit_io_sync(dev, rqd);
+
 	nvm_rq_dev_to_tgt(tgt_dev, rqd);
 
+	if (geo->version == NVM_OCSSD_SPEC_12 && rqd->opcode == NVM_OP_ERASE)
+		nvm_set_chunk_state_12(dev, rqd);
+
 	return ret;
 }
 EXPORT_SYMBOL(nvm_submit_io_sync);
@@ -775,10 +810,15 @@ void nvm_end_io(struct nvm_rq *rqd)
 {
 	struct nvm_tgt_dev *tgt_dev = rqd->dev;
 
-	/* Convert address space */
-	if (tgt_dev)
+	if (tgt_dev) {
+		/* Convert address space */
 		nvm_rq_dev_to_tgt(tgt_dev, rqd);
 
+		if (tgt_dev->geo.version == NVM_OCSSD_SPEC_12 &&
+						rqd->opcode == NVM_OP_ERASE)
+			nvm_set_chunk_state_12(tgt_dev->parent, rqd);
+	}
+
 	if (rqd->end_io)
 		rqd->end_io(rqd);
 }
diff --git a/drivers/lightnvm/pblk-core.c b/drivers/lightnvm/pblk-core.c
index 9968100cb170..0bcea243db90 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -79,7 +79,7 @@ static void __pblk_end_io_erase(struct pblk *pblk, struct nvm_rq *rqd)
 {
 	struct nvm_tgt_dev *dev = pblk->dev;
 	struct nvm_geo *geo = &dev->geo;
-	struct nvm_chk_meta *chunk;
+	struct nvm_chk_meta *chunk, *dev_chunk;
 	struct pblk_line *line;
 	int pos;
 
@@ -89,22 +89,39 @@ static void __pblk_end_io_erase(struct pblk *pblk, struct nvm_rq *rqd)
 
 	atomic_dec(&line->left_seblks);
 
+	/* pblk submits a single erase per command */
+	dev_chunk = rqd->meta_list;
+
+	if (dev_chunk->slba != chunk->slba || dev_chunk->wp)
+		print_chunk(pblk, chunk, "corrupted erase chunk", 0);
+
+	memcpy(chunk, dev_chunk, sizeof(struct nvm_chk_meta));
+
 	if (rqd->error) {
 		trace_pblk_chunk_reset(pblk_disk_name(pblk),
 				&rqd->ppa_addr, PBLK_CHUNK_RESET_FAILED);
 
-		chunk->state = NVM_CHK_ST_OFFLINE;
+#ifdef CONFIG_NVM_PBLK_DEBUG
+		if (chunk->state != NVM_CHK_ST_OFFLINE)
+			print_chunk(pblk, chunk,
+					"corrupted erase chunk state", 0);
+#endif
 		pblk_mark_bb(pblk, line, rqd->ppa_addr);
 	} else {
 		trace_pblk_chunk_reset(pblk_disk_name(pblk),
 				&rqd->ppa_addr, PBLK_CHUNK_RESET_DONE);
 
-		chunk->state = NVM_CHK_ST_FREE;
+#ifdef CONFIG_NVM_PBLK_DEBUG
+		if (chunk->state != NVM_CHK_ST_FREE)
+			print_chunk(pblk, chunk,
+					"corrupted erase chunk state", 0);
+#endif
 	}
 
 	trace_pblk_chunk_state(pblk_disk_name(pblk), &rqd->ppa_addr,
 				chunk->state);
 
+	pblk_free_rqd_meta(pblk, rqd);
 	atomic_dec(&pblk->inflight_io);
 }
 
@@ -952,14 +969,16 @@ int pblk_line_emeta_read(struct pblk *pblk, struct pblk_line *line,
 	return ret;
 }
 
-static void pblk_setup_e_rq(struct pblk *pblk, struct nvm_rq *rqd,
-			    struct ppa_addr ppa)
+static int pblk_setup_e_rq(struct pblk *pblk, struct nvm_rq *rqd,
+			   struct ppa_addr ppa)
 {
 	rqd->opcode = NVM_OP_ERASE;
 	rqd->ppa_addr = ppa;
 	rqd->nr_ppas = 1;
 	rqd->is_seq = 1;
 	rqd->bio = NULL;
+
+	return pblk_alloc_rqd_meta(pblk, rqd);
 }
 
 static int pblk_blk_erase_sync(struct pblk *pblk, struct ppa_addr ppa)
@@ -967,7 +986,9 @@ static int pblk_blk_erase_sync(struct pblk *pblk, struct ppa_addr ppa)
 	struct nvm_rq rqd = {NULL};
 	int ret;
 
-	pblk_setup_e_rq(pblk, &rqd, ppa);
+	ret = pblk_setup_e_rq(pblk, &rqd, ppa);
+	if (ret)
+		return ret;
 
 	trace_pblk_chunk_reset(pblk_disk_name(pblk),
 				&ppa, PBLK_CHUNK_RESET_START);
@@ -1774,11 +1795,15 @@ void pblk_line_put_wq(struct kref *ref)
 int pblk_blk_erase_async(struct pblk *pblk, struct ppa_addr ppa)
 {
 	struct nvm_rq *rqd;
-	int err;
+	int ret;
 
 	rqd = pblk_alloc_rqd(pblk, PBLK_ERASE);
 
-	pblk_setup_e_rq(pblk, rqd, ppa);
+	ret = pblk_setup_e_rq(pblk, rqd, ppa);
+	if (ret) {
+		pblk_free_rqd(pblk, rqd, PBLK_ERASE);
+		return ret;
+	}
 
 	rqd->end_io = pblk_end_io_erase;
 	rqd->private = pblk;
@@ -1789,8 +1814,8 @@ int pblk_blk_erase_async(struct pblk *pblk, struct ppa_addr ppa)
 	/* The write thread schedules erases so that it minimizes disturbances
 	 * with writes. Thus, there is no need to take the LUN semaphore.
 	 */
-	err = pblk_submit_io(pblk, rqd);
-	if (err) {
+	ret = pblk_submit_io(pblk, rqd);
+	if (ret) {
 		struct nvm_tgt_dev *dev = pblk->dev;
 		struct nvm_geo *geo = &dev->geo;
 
@@ -1799,7 +1824,7 @@ int pblk_blk_erase_async(struct pblk *pblk, struct ppa_addr ppa)
 					pblk_ppa_to_pos(geo, ppa));
 	}
 
-	return err;
+	return ret;
 }
 
 struct pblk_line *pblk_line_get_data(struct pblk *pblk)
-- 
2.7.4

Re: [PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[Matias Bjørling]

On 08/31/2018 03:34 PM, Javier González wrote:
> Matias,
> 
> This patchset implements support for retrieving chunk metadata when
> submitting a reset/erase command. Patches 0 and 1 are small fixes that
> can be directly merged into your patch:
>    
>    lightnvm: move bad block and chunk state logic to core
> 
> Also, note that these do not apply on top of your for-4.20/core due them
> depending on patches that I sent before that you have not picked up yet.
> You can see them though in for-4.20/pblk. I'll rebase as patches in the
> list appear in your tree.

Thanks. It is really confusing when you guys maintains an implicit order 
and posts the patches separately. I will appreciate that patches that 
are related are posted together, such that I don't have to manually 
track what comes before another. That makes it less of a pain for me to 
keep track of and we can keep the reviews together.

This is the patches that I have in the pipeline (from before the e-mails 
from today):

   - This serie - Pending review
   - Serie: pblk: support variable OOB size - Waiting on review from Igor
   - lightnvm: pblk: recover open lines on 2.0 devices. Which doesn't 
apply due to the fixes to the pad distance patch.

> 
> Thanks,
> Javier
> 
> Javier González (4):
>    lightnvm: use right address format on 1.2 path
>    lightnvm: assign block address before slba
>    lightnvm: pblk: add helper for printing chunk state
>    lightnvm: pblk: retrieve chunk metadata on erase
> 
>   drivers/lightnvm/core.c      | 51 +++++++++++++++++++++++++++++++++++++++-----
>   drivers/lightnvm/pblk-core.c | 47 ++++++++++++++++++++++++++++++----------
>   drivers/lightnvm/pblk.h      |  9 ++++++++
>   3 files changed, 91 insertions(+), 16 deletions(-)
> 

Re: [PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[Javier Gonzalez]

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

> On 31 Aug 2018, at 15.57, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 08/31/2018 03:34 PM, Javier González wrote:
>> Matias,
>> This patchset implements support for retrieving chunk metadata when
>> submitting a reset/erase command. Patches 0 and 1 are small fixes that
>> can be directly merged into your patch:
>>      lightnvm: move bad block and chunk state logic to core
>> Also, note that these do not apply on top of your for-4.20/core due them
>> depending on patches that I sent before that you have not picked up yet.
>> You can see them though in for-4.20/pblk. I'll rebase as patches in the
>> list appear in your tree.
> 
> Thanks. It is really confusing when you guys maintains an implicit order and posts the patches separately. I will appreciate that patches that are related are posted together, such that I don't have to manually track what comes before another. That makes it less of a pain for me to keep track of and we can keep the reviews together.
> 
> This is the patches that I have in the pipeline (from before the e-mails from today):
> 
>  - This serie - Pending review
>  - Serie: pblk: support variable OOB size - Waiting on review from Igor
>  - lightnvm: pblk: recover open lines on 2.0 devices. Which doesn't apply due to the fixes to the pad distance patch.
> 

Yes, I know and I apologize - we should have a better flow. What do you
say that for windows like this, where we have a number of patches that
have dependencies that we post them in meaningful patchsets and point to
a branch where they are ordered, like in a PR? Then we can rebase and
propagate changes properly?

For this window, I'll rebase the rest of the patches in for-4.20/pblk on
top of your for-4.20/core, then we can propagate changes as they come.

Javier

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

Re: [PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[Matias Bjørling]

On 09/03/2018 11:16 AM, Javier Gonzalez wrote:
>> On 31 Aug 2018, at 15.57, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 08/31/2018 03:34 PM, Javier González wrote:
>>> Matias,
>>> This patchset implements support for retrieving chunk metadata when
>>> submitting a reset/erase command. Patches 0 and 1 are small fixes that
>>> can be directly merged into your patch:
>>>       lightnvm: move bad block and chunk state logic to core
>>> Also, note that these do not apply on top of your for-4.20/core due them
>>> depending on patches that I sent before that you have not picked up yet.
>>> You can see them though in for-4.20/pblk. I'll rebase as patches in the
>>> list appear in your tree.
>>
>> Thanks. It is really confusing when you guys maintains an implicit order and posts the patches separately. I will appreciate that patches that are related are posted together, such that I don't have to manually track what comes before another. That makes it less of a pain for me to keep track of and we can keep the reviews together.
>>
>> This is the patches that I have in the pipeline (from before the e-mails from today):
>>
>>   - This serie - Pending review
>>   - Serie: pblk: support variable OOB size - Waiting on review from Igor
>>   - lightnvm: pblk: recover open lines on 2.0 devices. Which doesn't apply due to the fixes to the pad distance patch.
>>
> 
> Yes, I know and I apologize - we should have a better flow. What do you
> say that for windows like this, where we have a number of patches that
> have dependencies that we post them in meaningful patchsets and point to
> a branch where they are ordered, like in a PR? Then we can rebase and
> propagate changes properly?

I am with the patchset posted, that should have the order. I just wanted 
to mention it. One thing that would be good, if you do have patches you 
have upstream, feel free to push them in smaller increments, so we can 
pull them in as we go. Only a nitpick, it is obviously up to you guys 
how you want to do it :)

> 
> For this window, I'll rebase the rest of the patches in for-4.20/pblk on
> top of your for-4.20/core, then we can propagate changes as they come.
> 
> Javier
> 

Re: [PATCH 2/4] lightnvm: assign block address before slba

[Matias Bjørling]

On 08/31/2018 03:34 PM, Javier González wrote:
> In 1.2, the chunk slba is set to the physical representation of the
> block. Thus, assigning the block to the ppa must occur before the slba
> is assign.
> 
> Signed-off-by: Javier González <javier@cnexlabs.com>
> ---
>   drivers/lightnvm/core.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/lightnvm/core.c b/drivers/lightnvm/core.c
> index e9f14c67f4f3..efb976a863d2 100644
> --- a/drivers/lightnvm/core.c
> +++ b/drivers/lightnvm/core.c
> @@ -945,6 +945,8 @@ static int nvm_bb_to_chunk(struct nvm_dev *dev, struct ppa_addr ppa,
>   			}
>   		}
>   
> +		ppa.g.blk = blk;
> +
>   		meta->wp = 0;
>   		meta->type = NVM_CHK_TP_W_SEQ;
>   		meta->wi = 0;
> @@ -952,7 +954,6 @@ static int nvm_bb_to_chunk(struct nvm_dev *dev, struct ppa_addr ppa,
>   		meta->cnlb = dev->geo.clba;
>   
>   		if (blktype == NVM_BLK_T_FREE) {
> -			ppa.a.blk = blk;
>   			ret = nvm_bb_chunk_scan(dev, ppa, meta);
>   			if (ret)
>   				return ret;
> 

Thanks. I've folded 1 & 2 into the bb patch.

Re: [PATCH 0/4] lightnvm: pblk: add support for chunk metadata on erase

[Javier Gonzalez]

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

> On 4 Sep 2018, at 02.54, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 09/03/2018 11:16 AM, Javier Gonzalez wrote:
>>> On 31 Aug 2018, at 15.57, Matias Bjørling <mb@lightnvm.io> wrote:
>>> 
>>> On 08/31/2018 03:34 PM, Javier González wrote:
>>>> Matias,
>>>> This patchset implements support for retrieving chunk metadata when
>>>> submitting a reset/erase command. Patches 0 and 1 are small fixes that
>>>> can be directly merged into your patch:
>>>>      lightnvm: move bad block and chunk state logic to core
>>>> Also, note that these do not apply on top of your for-4.20/core due them
>>>> depending on patches that I sent before that you have not picked up yet.
>>>> You can see them though in for-4.20/pblk. I'll rebase as patches in the
>>>> list appear in your tree.
>>> 
>>> Thanks. It is really confusing when you guys maintains an implicit order and posts the patches separately. I will appreciate that patches that are related are posted together, such that I don't have to manually track what comes before another. That makes it less of a pain for me to keep track of and we can keep the reviews together.
>>> 
>>> This is the patches that I have in the pipeline (from before the e-mails from today):
>>> 
>>>  - This serie - Pending review
>>>  - Serie: pblk: support variable OOB size - Waiting on review from Igor
>>>  - lightnvm: pblk: recover open lines on 2.0 devices. Which doesn't apply due to the fixes to the pad distance patch.
>> Yes, I know and I apologize - we should have a better flow. What do you
>> say that for windows like this, where we have a number of patches that
>> have dependencies that we post them in meaningful patchsets and point to
>> a branch where they are ordered, like in a PR? Then we can rebase and
>> propagate changes properly?
> 
> I am with the patchset posted, that should have the order. I just
> wanted to mention it. One thing that would be good, if you do have
> patches you have upstream, feel free to push them in smaller
> increments, so we can pull them in as we go. Only a nitpick, it is
> obviously up to you guys how you want to do it :)
> 

Sure. If we can improve the workflow to make things easier for you, then
we should.


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