> 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