| Message ID | 20190305135120.29284-7-igor.j.konopko@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | lightnvm: bugfixes and improvements | expand |
> On 5 Mar 2019, at 14.51, Igor Konopko <igor.j.konopko@intel.com> wrote: > > Currently in case of read errors, bi_status is not set properly which > leads to returning inproper data to higher layer. This patch fix that > by setting proper status in case of read errors > > Patch also removes unnecessary warn_once(), which does not make sense > in that place, since user bio is not used for interation with drive > and thus bi_status will not be set here. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-read.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c > index 1f9b319..6569746 100644 > --- a/drivers/lightnvm/pblk-read.c > +++ b/drivers/lightnvm/pblk-read.c > @@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd, > WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n"); > } > > -static void pblk_end_user_read(struct bio *bio) > +static void pblk_end_user_read(struct bio *bio, int error) > { > -#ifdef CONFIG_NVM_PBLK_DEBUG > - WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n"); > -#endif > + if (error && error != NVM_RSP_WARN_HIGHECC) > + bio_io_error(bio); > bio_endio(bio); > } > > @@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd) > struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd); > struct bio *bio = (struct bio *)r_ctx->private; > > - pblk_end_user_read(bio); > + pblk_end_user_read(bio, rqd->error); > __pblk_end_io_read(pblk, rqd, true); > } > > @@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd) > rqd->bio = NULL; > rqd->nr_ppas = nr_secs; > > - bio_endio(bio); > + pblk_end_user_read(bio, rqd->error); > __pblk_end_io_read(pblk, rqd, false); > } > > -- > 2.9.5 I still believe that we should work out the error model before. Let me give an example. In the OCSSD 2.0 spec, we use the NVMe DULBE bit. If the device does not report read errors, then invalid reads (let’s pick a read ahead of the WP as an example) will return success + predefined data. In this case, pblk would catch this in the read integrity checks. Should pblk return an error in this case? I see the point of catching this at the driver level and passing the error, but the behavior is inconsistent as the same read issued outside of pblk will succeed. When I implemented the read integrity checks I considered to plug it into the block layer Integrity framework (CONFIG_BLK_DEV_INTEGRITY), but since we do not follow any PI standard I thought it was difficult to argue for it. It is maybe time to revisit this approach. This said, if you want it in to help debugging, I will not oppose to it. I guess it is the same to fix it one way or the other when we implement proper error handling. Javier
On Tue, Mar 5, 2019 at 2:54 PM Igor Konopko <igor.j.konopko@intel.com> wrote: > > Currently in case of read errors, bi_status is not set properly which > leads to returning inproper data to higher layer. This patch fix that > by setting proper status in case of read errors > > Patch also removes unnecessary warn_once(), which does not make sense > in that place, since user bio is not used for interation with drive > and thus bi_status will not be set here. > > Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> > --- > drivers/lightnvm/pblk-read.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c > index 1f9b319..6569746 100644 > --- a/drivers/lightnvm/pblk-read.c > +++ b/drivers/lightnvm/pblk-read.c > @@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd, > WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n"); > } > > -static void pblk_end_user_read(struct bio *bio) > +static void pblk_end_user_read(struct bio *bio, int error) > { > -#ifdef CONFIG_NVM_PBLK_DEBUG > - WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n"); > -#endif > + if (error && error != NVM_RSP_WARN_HIGHECC) > + bio_io_error(bio); > bio_endio(bio); > } > > @@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd) > struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd); > struct bio *bio = (struct bio *)r_ctx->private; > > - pblk_end_user_read(bio); > + pblk_end_user_read(bio, rqd->error); > __pblk_end_io_read(pblk, rqd, true); > } > > @@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd) > rqd->bio = NULL; > rqd->nr_ppas = nr_secs; > > - bio_endio(bio); > + pblk_end_user_read(bio, rqd->error); > __pblk_end_io_read(pblk, rqd, false); > } > > -- > 2.9.5 > Looks good to me, Reviewed-by: Hans Holmberg <hans.holmberg@cnexlabs.com>
diff --git a/drivers/lightnvm/pblk-read.c b/drivers/lightnvm/pblk-read.c index 1f9b319..6569746 100644 --- a/drivers/lightnvm/pblk-read.c +++ b/drivers/lightnvm/pblk-read.c @@ -175,11 +175,10 @@ static void pblk_read_check_rand(struct pblk *pblk, struct nvm_rq *rqd, WARN_ONCE(j != rqd->nr_ppas, "pblk: corrupted random request\n"); } -static void pblk_end_user_read(struct bio *bio) +static void pblk_end_user_read(struct bio *bio, int error) { -#ifdef CONFIG_NVM_PBLK_DEBUG - WARN_ONCE(bio->bi_status, "pblk: corrupted read bio\n"); -#endif + if (error && error != NVM_RSP_WARN_HIGHECC) + bio_io_error(bio); bio_endio(bio); } @@ -219,7 +218,7 @@ static void pblk_end_io_read(struct nvm_rq *rqd) struct pblk_g_ctx *r_ctx = nvm_rq_to_pdu(rqd); struct bio *bio = (struct bio *)r_ctx->private; - pblk_end_user_read(bio); + pblk_end_user_read(bio, rqd->error); __pblk_end_io_read(pblk, rqd, true); } @@ -292,7 +291,7 @@ static void pblk_end_partial_read(struct nvm_rq *rqd) rqd->bio = NULL; rqd->nr_ppas = nr_secs; - bio_endio(bio); + pblk_end_user_read(bio, rqd->error); __pblk_end_io_read(pblk, rqd, false); }
Currently in case of read errors, bi_status is not set properly which leads to returning inproper data to higher layer. This patch fix that by setting proper status in case of read errors Patch also removes unnecessary warn_once(), which does not make sense in that place, since user bio is not used for interation with drive and thus bi_status will not be set here. Signed-off-by: Igor Konopko <igor.j.konopko@intel.com> --- drivers/lightnvm/pblk-read.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)