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
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 <[email protected]>
---
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
Implement pblk helper for printing chunk state.
Signed-off-by: Javier González <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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(-)
>
> On 31 Aug 2018, at 15.57, Matias Bjørling <[email protected]> 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
On 09/03/2018 11:16 AM, Javier Gonzalez wrote:
>> On 31 Aug 2018, at 15.57, Matias Bjørling <[email protected]> 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
>
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 <[email protected]>
> ---
> 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.
> On 4 Sep 2018, at 02.54, Matias Bjørling <[email protected]> wrote:
>
> On 09/03/2018 11:16 AM, Javier Gonzalez wrote:
>>> On 31 Aug 2018, at 15.57, Matias Bjørling <[email protected]> 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.