On 05.03.2019 09:20, Hans Holmberg wrote:
On Mon, Mar 4, 2019 at 2:00 PM Matias Bjørling <mb@xxxxxxxxxxx> wrote:
On 3/4/19 1:44 PM, Igor Konopko wrote:
On 04.03.2019 12:43, Hans Holmberg wrote:
On Mon, Mar 4, 2019 at 10:11 AM Javier González <javier@xxxxxxxxxxx>
wrote:
On 4 Mar 2019, at 10.05, Hans Holmberg
<hans.ml.holmberg@xxxxxxxxxxxxx> wrote:
I strongly disagree with adding code that would mask implantation
errors.
If we want more internal checks, we could add an if statement that
would only be compiled in if CONFIG_NVM_PBLK_DEBUG is enabled.
Not sure who this is for - better not to top post.
In any case, this is a spec grey zone. I’m ok with cleaning the bits as
they mean nothing for the reset command. If you feel that strongly about
this, you can take if with Igor.
Pardon the top-post. It was meant for both you and Igor.
OCSSD 2.0 spec for vector chunk reset (chapter 2.2.2) explicitly says
"The addresses in the LBA list shall be the first logical block address
of each chunk to be reset.". So in my understanding we suppose to clear
the sectors bits of the PPA address in order to be spec compliant.
Agree. And since ppa_addr is allocated on the stack, it should be either
memset or the remaining fields should be set to 0. Maybe better to zero
initialize in declaration?
Ah, I thought this was not needed, as ppa is initialized as:
ppa = pblk->luns[bit].bppa; /* set ch and lun */
and luns[bit].bppa is initialized to on a value that originally comes
from drivers/lightnvm/core.c:196
(and that's explicitly zeroing all 64 bits before setting ch and lun)
Let me know if i don't make sense here.
I just noticed the same.
In two places (pblk-core:1095 and pblk-map:205) we are using values
initialized previously in core.c - so my changes are not needed here.
But still there is one place (pblk-map:163) where we initializing
erase_ppa based on ppa_list[i], which has PPA sector set in most of the
cases, so this zeroing is still needed here.
On Mon, Mar 4, 2019 at 8:48 AM Javier González <javier@xxxxxxxxxxx>
wrote:
On 27 Feb 2019, at 18.14, Igor Konopko <igor.j.konopko@xxxxxxxxx>
wrote:
In current pblk implementation of erase command
there is a chance tha sector bits are set to some
random values for erase PPA. This is unexpected
situation, since erase shall be always chunk
aligned. This patch fixes that issue
Signed-off-by: Igor Konopko <igor.j.konopko@xxxxxxxxx>
---
drivers/lightnvm/pblk-core.c | 1 +
drivers/lightnvm/pblk-map.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/lightnvm/pblk-core.c
b/drivers/lightnvm/pblk-core.c
index a98b2255f963..78b1eea4ab67 100644
--- a/drivers/lightnvm/pblk-core.c
+++ b/drivers/lightnvm/pblk-core.c
@@ -978,6 +978,7 @@ int pblk_line_erase(struct pblk *pblk, struct
pblk_line *line)
ppa = pblk->luns[bit].bppa; /* set ch and lun */
ppa.a.blk = line->id;
+ ppa.a.reserved = 0;
atomic_dec(&line->left_eblks);
WARN_ON(test_and_set_bit(bit, line->erase_bitmap));
diff --git a/drivers/lightnvm/pblk-map.c
b/drivers/lightnvm/pblk-map.c
index 79df583ea709..aea46b4ec40f 100644
--- a/drivers/lightnvm/pblk-map.c
+++ b/drivers/lightnvm/pblk-map.c
@@ -161,6 +161,7 @@ int pblk_map_erase_rq(struct pblk *pblk,
struct nvm_rq *rqd,
*erase_ppa = ppa_list[i];
erase_ppa->a.blk = e_line->id;
+ erase_ppa->a.reserved = 0;
spin_unlock(&e_line->lock);
@@ -202,6 +203,7 @@ int pblk_map_erase_rq(struct pblk *pblk,
struct nvm_rq *rqd,
atomic_dec(&e_line->left_eblks);
*erase_ppa = pblk->luns[bit].bppa; /* set ch and lun */
erase_ppa->a.blk = e_line->id;
+ erase_ppa->a.reserved = 0;
}
return 0;
--
2.17.1
I’m fine with adding this, but note that there is actually no
requirement for the erase to be chunk aligned - the only bits that
should be looked at are group, PU and chunk.
Reviewed-by: Javier González <javier@xxxxxxxxxxx>
