Re: [PATCH] ubi: do not re-read the data already read out in retry

[Richard Weinberger <richard@nod.at>]

Am 24.08.2015 um 03:05 schrieb Dongsheng Yang:
> On 08/21/2015 05:17 PM, Richard Weinberger wrote:
>> Am 21.08.2015 um 09:59 schrieb Dongsheng Yang:
>>> In ubi_io_read(), we will retry if current reading failed
>>> to read all data we wanted. But we are doing a full re-do
>>> in the re-try path. Actually, we can skip the data which
>>> we have already read out in the last reading.
>>>
>>> Signed-off-by: Dongsheng Yang <yangds.fnst@cn.fujitsu.com>
>>> ---
>>>   drivers/mtd/ubi/io.c | 19 +++++++++++++------
>>>   1 file changed, 13 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
>>> index 5bbd1f0..a3ac643 100644
>>> --- a/drivers/mtd/ubi/io.c
>>> +++ b/drivers/mtd/ubi/io.c
>>> @@ -127,7 +127,7 @@ int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
>>>           int len)
>>>   {
>>>       int err, retries = 0;
>>> -    size_t read;
>>> +    size_t read, already_read = 0;
>>>       loff_t addr;
>>>
>>>       dbg_io("read %d bytes from PEB %d:%d", len, pnum, offset);
>>> @@ -165,6 +165,7 @@ int ubi_io_read(const struct ubi_device *ubi, void *buf, int pnum, int offset,
>>>       addr = (loff_t)pnum * ubi->peb_size + offset;
>>>   retry:
>>>       err = mtd_read(ubi->mtd, addr, len, &read, buf);
>>> +    already_read += read;
>>
>> Hmm, this change makes me nervous.
> 
> Ha, yes, understandable.
>>
>> Brian, does MTD core guarantee that upon an erroneous mtd_read() the number of "read" bytes
>> in "buf" are valid?
>>
>> So, my fear is that this change will introduce new regressions (due faulty MTD drivers, etc..)
>> without a real gain.
> 
> I would say "big gain" rather than "real gain". Consider this case, if
> you are going to read 4M from driver but it failed at the last byte
> twice. When we success on the third retry, we have read out 4*3=12M for
> 4M data user requested. That tripled the latency.

How do you hit this case?
What error is mtd_read() returning?

I had the feeling that this is more a theoretical fix.

Thanks,
//richard