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

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

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.

But with this patch applied. we do not re-try the data we have already
read out. So the latency is 1/3 of it mentioned above.

But, yes, I said understandable, I also want Brian to say is this
change safe enough currently.

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