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

[Dongsheng Yang <yangds.fnst@cn.fujitsu.com>]

On 08/24/2015 02:46 PM, Richard Weinberger wrote:
> 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.

Yes, so I agreed that both of us would feel nervous about it.
I did not hit this kind of case in practise, but tested it with
some hacked code in mtdram.

TBH, my customer send me this requirement and I cooked this patch
to them.I just want to send this patch to you to see would you
be interested in it. IMO, I did not find the practical usecase
for it.

So, I understand your nervous and I am okey to drop it currently.
Maybe we can come back when we found a device driver really get
a big gain from this change. :)

Thanx
Yang
>
> Thanks,
> //richard
> .
>