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

Matias Bjørling
2019-03-04T13:08:34Z

On 3/4/19 1:51 PM, Igor Konopko wrote:
>
>
> On 04.03.2019 13:14, Hans Holmberg wrote:
>> On Mon, Mar 4, 2019 at 10:48 AM Javier González <javier@javigon.com>
>> wrote:
>>>
>>>
>>>
>>>> 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.
>>
>> So returning bogus data without even a warning is a preferred
>> solution? You want to force "the upper layers" to do checksumming?
>>
>> It's fine to mask out NVM_RSP_WARN_HIGHECC, since that is just a
>> warning that OCSSD 2.0 adds. The data should still be good.
>> All other errors (see 4.6.1.2.1 in the NVMe 1.3 spec), indicates that
>> the command did not complete (As far as I can tell)
>>
>
> My approach was exactly like that. In all cases other than WARN_HIGHECC
> we don't have a valid data. Without setting a bio_io_error() we are
> creating the impression for other layers, that we read the data
> correctly, what is not a case then.
>
> I'm also seeing that this patch is not the only user of bio_io_error()
> API, also other drivers such as md uses is commonly.

Yes agree. This is an actual error in pblk that lets it return bogus data.