From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [59.151.112.132] (helo=heian.cn.fujitsu.com) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZTltn-0001jD-Cf for linux-mtd@lists.infradead.org; Mon, 24 Aug 2015 07:10:32 +0000 Message-ID: <55DAC1E2.2040700@cn.fujitsu.com> Date: Mon, 24 Aug 2015 15:04:02 +0800 From: Dongsheng Yang MIME-Version: 1.0 To: Richard Weinberger , Brian Norris CC: Subject: Re: [PATCH] ubi: do not re-read the data already read out in retry References: <1440143981-20826-1-git-send-email-yangds.fnst@cn.fujitsu.com> <55D6ECC4.5070503@nod.at> <55DA6DC8.9010308@cn.fujitsu.com> <55DABDBB.4080404@nod.at> In-Reply-To: <55DABDBB.4080404@nod.at> Content-Type: text/plain; charset="ISO-8859-15"; format=flowed Content-Transfer-Encoding: 7bit List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >>>> --- >>>> 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 > . >