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 1ZTgIU-00023m-OY for linux-mtd@lists.infradead.org; Mon, 24 Aug 2015 01:11:39 +0000 Message-ID: <55DA6DC8.9010308@cn.fujitsu.com> Date: Mon, 24 Aug 2015 09:05:12 +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> In-Reply-To: <55D6ECC4.5070503@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/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. 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 > . >