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

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

[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;
 	if (err) {
 		const char *errstr = mtd_is_eccerr(err) ? " (ECC error)" : "";
 
@@ -179,19 +180,25 @@ retry:
 			 */
 			ubi_msg(ubi, "fixable bit-flip detected at PEB %d",
 				pnum);
-			ubi_assert(len == read);
+			ubi_assert(len == already_read);
 			return UBI_IO_BITFLIPS;
 		}
 
 		if (retries++ < UBI_IO_RETRIES) {
-			ubi_warn(ubi, "error %d%s while reading %d bytes from PEB %d:%d, read only %zd bytes, retry",
-				 err, errstr, len, pnum, offset, read);
+			ubi_warn(ubi, "error %d%s while reading %d bytes from PEB %d:%lu, read only %zd bytes, retry",
+				 err, errstr, len, pnum, offset + already_read, read);
 			yield();
+			/*
+			 * Skip to do retry for data which was already read out;
+			 */
+			addr += read;
+			len -= read;
+			buf += read;
 			goto retry;
 		}
 
 		ubi_err(ubi, "error %d%s while reading %d bytes from PEB %d:%d, read %zd bytes",
-			err, errstr, len, pnum, offset, read);
+			err, errstr, len, pnum, offset, already_read);
 		dump_stack();
 
 		/*
@@ -204,7 +211,7 @@ retry:
 			err = -EIO;
 		}
 	} else {
-		ubi_assert(len == read);
+		ubi_assert(len == already_read);
 
 		if (ubi_dbg_is_bitflip(ubi)) {
 			dbg_gen("bit-flip (emulated)");
-- 
1.8.4.2

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

[Richard Weinberger]

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.

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.

Thanks,
//richard

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

[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.

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
> .
>

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

[Richard Weinberger]

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

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

[Dongsheng Yang]

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
> .
>

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

[Brian Norris]

On Mon, Aug 24, 2015 at 09:05:12AM +0800, Dongsheng Yang wrote:
> On 08/21/2015 05:17 PM, Richard Weinberger wrote:
> >Brian, does MTD core guarantee that upon an erroneous mtd_read() the number of "read" bytes
> >in "buf" are valid?

I'll take a look. I believe the Dongsheng is interpreting the mtd_read()
API correctly, but most MTD "guarantees" are more like surveys of
existing practice; most implementations may do something sane, but there
may be a few oddballs.

> >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".

I'd like evidence, rather than just bold statements :)

> 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.

On what driver do you see this? Many drivers take an all-or-nothing
approach already; they don't fill in 'retlen' to any non-zero value
until the last byte, so Richard is right in those cases.

(I can partly answer this question; nand_do_read_ops() incrementally
fills pages, and *retlen increases when a (portion of) a page is read
correctly.)

Also, what sort of failure modes are you seeing? It seems a bit dubious
to be relying on UBI I/O retry, let alone optimizing for performance,
since the retry may be masking significant problems anyway.

> 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.

Brian

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

[Brian Norris]

On Mon, Aug 24, 2015 at 03:04:02PM +0800, Dongsheng Yang wrote:
> TBH, my customer send me this requirement and I cooked this patch
> to them.

Ah, so it's highly likely the driver is just terrible! In that case, I'd
suggest a nice NAK, until we find a legitimate use case.

Regards,
Brian

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

[Dongsheng Yang]

On 08/25/2015 12:59 AM, Brian Norris wrote:
> On Mon, Aug 24, 2015 at 03:04:02PM +0800, Dongsheng Yang wrote:
>> TBH, my customer send me this requirement and I cooked this patch
>> to them.
>
> Ah, so it's highly likely the driver is just terrible! In that case, I'd
> suggest a nice NAK, until we find a legitimate use case.

Agreed, thanx Brian.

Yang
>
> Regards,
> Brian
>