Re: [PATCH 08/13] lightnvm: pblk: Set proper read stutus in bio

["Javier González" <javier@javigon.com>]

[-- Attachment #1: Type: text/plain, Size: 3025 bytes --]



> On 4 Mar 2019, at 10.35, Hans Holmberg <hans.ml.holmberg@owltronix.com> wrote:
> 
> On Mon, Mar 4, 2019 at 9:03 AM Javier González <javier@javigon.com> wrote:
>>> On 27 Feb 2019, at 18.14, 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 3789185144da..39c1d6ccaedb 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.17.1
>> 
>> This is by design. We do not report the read errors as in any other
>> block device - this is why we clone the read bio.
> 
> Could you elaborate on why not reporting read errors is a good thing in pblk?
> 

Normal block devices do not report read errors on the completion path
unless it is a fatal error. This is actually not well understood by the
upper layers, which tend to assume that the device is completely broken.

This is a challenge for OCSSD / Denali / Zone devices as there are cases
where reads can fail. Unfortunately at this point, we need to mask these
errors and deal with them in the different layers.

For OCSSD currently, we do this in pblk, which I think fits well the
model as we exposed a normal block device.

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]