[PATCH 03/11] lightnvm: pblk: check read lba on gc path

[Javier González]

Check that the lba stored in the LBA metadata is correct in the GC path
too. This requires a new helper function to check random reads in the
vector read.

Signed-off-by: Javier González 
---
 drivers/lightnvm/pblk-read.c | 39 +--
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 9eee10f69df0..1f699c09e0ea 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -113,15 +113,14 @@ static int pblk_submit_read_io(struct pblk *pblk, struct 
nvm_rq *rqd)
return NVM_IO_OK;
 }
 
-static void pblk_read_check(struct pblk *pblk, struct nvm_rq *rqd,
-  sector_t blba)
+static void pblk_read_check_seq(struct pblk *pblk, void *meta_list,
+   sector_t blba, int nr_lbas)
 {
-   struct pblk_sec_meta *meta_list = rqd->meta_list;
-   int nr_lbas = rqd->nr_ppas;
+   struct pblk_sec_meta *meta_lba_list = meta_list;
int i;
 
for (i = 0; i < nr_lbas; i++) {
-   u64 lba = le64_to_cpu(meta_list[i].lba);
+   u64 lba = le64_to_cpu(meta_lba_list[i].lba);
 
if (lba == ADDR_EMPTY)
continue;
@@ -130,6 +129,32 @@ static void pblk_read_check(struct pblk *pblk, struct 
nvm_rq *rqd,
}
 }
 
+/*
+ * There can be holes in the lba list.
+ */
+static void pblk_read_check_rand(struct pblk *pblk, void *meta_list,
+   u64 *lba_list, int nr_lbas)
+{
+   struct pblk_sec_meta *meta_lba_list = meta_list;
+   int i, j;
+
+   for (i = 0, j = 0; i < nr_lbas; i++) {
+   u64 lba = lba_list[i];
+   u64 meta_lba;
+
+   if (lba == ADDR_EMPTY)
+   continue;
+
+   meta_lba = le64_to_cpu(meta_lba_list[j++].lba);
+
+   if (lba != meta_lba) {
+   pr_err("pblk: corrupted read LBA (%llu/%llu)\n",
+   lba, meta_lba);
+   WARN_ON(1);
+   }
+   }
+}
+
 static void pblk_read_put_rqd_kref(struct pblk *pblk, struct nvm_rq *rqd)
 {
struct ppa_addr *ppa_list;
@@ -172,7 +197,7 @@ static void __pblk_end_io_read(struct pblk *pblk, struct 
nvm_rq *rqd,
WARN_ONCE(bio->bi_status, "pblk: corrupted read error\n");
 #endif
 
-   pblk_read_check(pblk, rqd, r_ctx->lba);
+   pblk_read_check_seq(pblk, rqd->meta_list, r_ctx->lba, rqd->nr_ppas);
 
bio_put(bio);
if (r_ctx->private)
@@ -585,6 +610,8 @@ int pblk_submit_read_gc(struct pblk *pblk, struct 
pblk_gc_rq *gc_rq)
goto err_free_bio;
}
 
+   pblk_read_check_rand(pblk, rqd.meta_list, gc_rq->lba_list, rqd.nr_ppas);
+
atomic_dec(>inflight_io);
 
if (rqd.error) {
-- 
2.7.4

Re: [PATCH 03/11] lightnvm: pblk: check read lba on gc path

[Javier González]

> On 17 Apr 2018, at 05.03, Matias Bjørling  wrote:
> 
> On 4/16/18 12:25 PM, Javier González wrote:
>> Check that the lba stored in the LBA metadata is correct in the GC path
>> too. This requires a new helper function to check random reads in the
>> vector read.
>> Signed-off-by: Javier González 
>> ---
>>  drivers/lightnvm/pblk-read.c | 39 +--
>>  1 file changed, 33 insertions(+), 6 deletions(-)
>> diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
>> index 9eee10f69df0..0d45d4ffc370 100644
>> --- a/drivers/lightnvm/pblk-read.c
>> +++ b/drivers/lightnvm/pblk-read.c
>> @@ -113,15 +113,14 @@ static int pblk_submit_read_io(struct pblk *pblk, 
>> struct nvm_rq *rqd)
>>  return NVM_IO_OK;
>>  }
>>  -static void pblk_read_check(struct pblk *pblk, struct nvm_rq *rqd,
>> -   sector_t blba)
>> +static void pblk_read_check_seq(struct pblk *pblk, void *meta_list,
>> +sector_t blba, int nr_lbas)
>>  {
>> -struct pblk_sec_meta *meta_list = rqd->meta_list;
>> -int nr_lbas = rqd->nr_ppas;
>> +struct pblk_sec_meta *meta_lba_list = meta_list;
>>  int i;
>>  for (i = 0; i < nr_lbas; i++) {
>> -u64 lba = le64_to_cpu(meta_list[i].lba);
>> +u64 lba = le64_to_cpu(meta_lba_list[i].lba);
>>  if (lba == ADDR_EMPTY)
>>  continue;
>> @@ -130,6 +129,32 @@ static void pblk_read_check(struct pblk *pblk, struct 
>> nvm_rq *rqd,
>>  }
>>  }
>>  +/*
>> + * There can be wholes in the lba list.
>> + */
> 
> holes

Can you change it?

> 
>> +static void pblk_read_check_rand(struct pblk *pblk, void *meta_list,
>> +u64 *lba_list, int nr_lbas)
>> +{
>> +struct pblk_sec_meta *meta_lba_list = meta_list;
>> +int i, j;
>> +
>> +for (i = 0, j = 0; i < nr_lbas; i++) {
>> +u64 lba = lba_list[i];
>> +u64 meta_lba;
>> +
>> +if (lba == ADDR_EMPTY)
>> +continue;
>> +
>> +meta_lba = le64_to_cpu(meta_lba_list[j++].lba);
> 
> You can move the j++ into the for loop. Although, why not use just the i 
> iterator?

Look above, we only increase on valid lbas.

> 
>> +
>> +if (lba != meta_lba) {
>> +pr_err("pblk: corrupted read LBA (%llu/%llu)\n",
>> +lba, meta_lba);
>> +WARN_ON(1);
> 
> I don't understand this, if a corrupted LBA is found here, how is it
> communicated back to pblk and the LBA can be recovered from elsewhere.
> If the data is wrong, this should have been communicated by the drive
> on the completion path. The drive is defect if this happens.
> 

This happens if a read does not match the lba stored on the LBA. This is
definitely an error and it has helped us in the past to find race
conditions on pblk's read path, specially when forming partial I/Os.

If the read failed from the drive (e.g., when enabling HECC), we still
want information about where it failed for debugging purposes. Same we
do on the normal read path.

Javier


signature.asc
Description: Message signed with OpenPGP

Re: [PATCH 03/11] lightnvm: pblk: check read lba on gc path

[Matias Bjørling]

On 4/16/18 12:25 PM, Javier González wrote:

Check that the lba stored in the LBA metadata is correct in the GC path
too. This requires a new helper function to check random reads in the
vector read.

Signed-off-by: Javier González 
---
  drivers/lightnvm/pblk-read.c | 39 +--
  1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 9eee10f69df0..0d45d4ffc370 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -113,15 +113,14 @@ static int pblk_submit_read_io(struct pblk *pblk, struct 
nvm_rq *rqd)
return NVM_IO_OK;
  }
  
-static void pblk_read_check(struct pblk *pblk, struct nvm_rq *rqd,

-  sector_t blba)
+static void pblk_read_check_seq(struct pblk *pblk, void *meta_list,
+   sector_t blba, int nr_lbas)
  {
-   struct pblk_sec_meta *meta_list = rqd->meta_list;
-   int nr_lbas = rqd->nr_ppas;
+   struct pblk_sec_meta *meta_lba_list = meta_list;
int i;
  
  	for (i = 0; i < nr_lbas; i++) {

-   u64 lba = le64_to_cpu(meta_list[i].lba);
+   u64 lba = le64_to_cpu(meta_lba_list[i].lba);
  
  		if (lba == ADDR_EMPTY)

continue;
@@ -130,6 +129,32 @@ static void pblk_read_check(struct pblk *pblk, struct 
nvm_rq *rqd,
}
  }
  
+/*

+ * There can be wholes in the lba list.
+ */


holes


+static void pblk_read_check_rand(struct pblk *pblk, void *meta_list,
+   u64 *lba_list, int nr_lbas)
+{
+   struct pblk_sec_meta *meta_lba_list = meta_list;
+   int i, j;
+
+   for (i = 0, j = 0; i < nr_lbas; i++) {
+   u64 lba = lba_list[i];
+   u64 meta_lba;
+
+   if (lba == ADDR_EMPTY)
+   continue;
+
+   meta_lba = le64_to_cpu(meta_lba_list[j++].lba);


You can move the j++ into the for loop. Although, why not use just the i 
iterator?



+
+   if (lba != meta_lba) {
+   pr_err("pblk: corrupted read LBA (%llu/%llu)\n",
+   lba, meta_lba);
+   WARN_ON(1);


I don't understand this, if a corrupted LBA is found here, how is it 
communicated back to pblk and the LBA can be recovered from elsewhere. 
If the data is wrong, this should have been communicated by the drive on 
the completion path. The drive is defect if this happens.




+   }
+   }
+}
+
  static void pblk_read_put_rqd_kref(struct pblk *pblk, struct nvm_rq *rqd)
  {
struct ppa_addr *ppa_list;
@@ -172,7 +197,7 @@ static void __pblk_end_io_read(struct pblk *pblk, struct 
nvm_rq *rqd,
WARN_ONCE(bio->bi_status, "pblk: corrupted read error\n");
  #endif
  
-	pblk_read_check(pblk, rqd, r_ctx->lba);

+   pblk_read_check_seq(pblk, rqd->meta_list, r_ctx->lba, rqd->nr_ppas);
  
  	bio_put(bio);

if (r_ctx->private)
@@ -585,6 +610,8 @@ int pblk_submit_read_gc(struct pblk *pblk, struct 
pblk_gc_rq *gc_rq)
goto err_free_bio;
}
  
+	pblk_read_check_rand(pblk, rqd.meta_list, gc_rq->lba_list, rqd.nr_ppas);

+
atomic_dec(>inflight_io);
  
  	if (rqd.error) {

[PATCH 03/11] lightnvm: pblk: check read lba on gc path

[Javier González]

Check that the lba stored in the LBA metadata is correct in the GC path
too. This requires a new helper function to check random reads in the
vector read.

Signed-off-by: Javier González 
---
 drivers/lightnvm/pblk-read.c | 39 +--
 1 file changed, 33 insertions(+), 6 deletions(-)

diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c
index 9eee10f69df0..0d45d4ffc370 100644
--- a/drivers/lightnvm/pblk-read.c
+++ b/drivers/lightnvm/pblk-read.c
@@ -113,15 +113,14 @@ static int pblk_submit_read_io(struct pblk *pblk, struct 
nvm_rq *rqd)
return NVM_IO_OK;
 }
 
-static void pblk_read_check(struct pblk *pblk, struct nvm_rq *rqd,
-  sector_t blba)
+static void pblk_read_check_seq(struct pblk *pblk, void *meta_list,
+   sector_t blba, int nr_lbas)
 {
-   struct pblk_sec_meta *meta_list = rqd->meta_list;
-   int nr_lbas = rqd->nr_ppas;
+   struct pblk_sec_meta *meta_lba_list = meta_list;
int i;
 
for (i = 0; i < nr_lbas; i++) {
-   u64 lba = le64_to_cpu(meta_list[i].lba);
+   u64 lba = le64_to_cpu(meta_lba_list[i].lba);
 
if (lba == ADDR_EMPTY)
continue;
@@ -130,6 +129,32 @@ static void pblk_read_check(struct pblk *pblk, struct 
nvm_rq *rqd,
}
 }
 
+/*
+ * There can be wholes in the lba list.
+ */
+static void pblk_read_check_rand(struct pblk *pblk, void *meta_list,
+   u64 *lba_list, int nr_lbas)
+{
+   struct pblk_sec_meta *meta_lba_list = meta_list;
+   int i, j;
+
+   for (i = 0, j = 0; i < nr_lbas; i++) {
+   u64 lba = lba_list[i];
+   u64 meta_lba;
+
+   if (lba == ADDR_EMPTY)
+   continue;
+
+   meta_lba = le64_to_cpu(meta_lba_list[j++].lba);
+
+   if (lba != meta_lba) {
+   pr_err("pblk: corrupted read LBA (%llu/%llu)\n",
+   lba, meta_lba);
+   WARN_ON(1);
+   }
+   }
+}
+
 static void pblk_read_put_rqd_kref(struct pblk *pblk, struct nvm_rq *rqd)
 {
struct ppa_addr *ppa_list;
@@ -172,7 +197,7 @@ static void __pblk_end_io_read(struct pblk *pblk, struct 
nvm_rq *rqd,
WARN_ONCE(bio->bi_status, "pblk: corrupted read error\n");
 #endif
 
-   pblk_read_check(pblk, rqd, r_ctx->lba);
+   pblk_read_check_seq(pblk, rqd->meta_list, r_ctx->lba, rqd->nr_ppas);
 
bio_put(bio);
if (r_ctx->private)
@@ -585,6 +610,8 @@ int pblk_submit_read_gc(struct pblk *pblk, struct 
pblk_gc_rq *gc_rq)
goto err_free_bio;
}
 
+   pblk_read_check_rand(pblk, rqd.meta_list, gc_rq->lba_list, rqd.nr_ppas);
+
atomic_dec(>inflight_io);
 
if (rqd.error) {
-- 
2.7.4