[PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Sascha Hauer]

When reading the vid hdr from the device UBI always reads a whole
page. Instead, read only the data we actually need and speed up
attachment of UBI devices by potentially making use of reading
subpages.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---

Please review carefully. It obviously works and speeds up UBI attachment
from 3s to 2s here in one case and I have not found places where this patch
makes problems, but there might be a reason I haven't seen why in case of
the ec header only the header is read while in case of the vid header the
whole page is read.

 drivers/mtd/ubi/io.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 10cf3b5..31918a0 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 
 	p = (char *)vid_hdr - ubi->vid_hdr_shift;
 	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
-			  ubi->vid_hdr_alsize);
+			  UBI_VID_HDR_SIZE);
 	if (read_err && read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err))
 		return read_err;
 
-- 
2.8.1

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Richard Weinberger]

Am 23.06.2016 um 15:29 schrieb Sascha Hauer:
> When reading the vid hdr from the device UBI always reads a whole
> page. Instead, read only the data we actually need and speed up
> attachment of UBI devices by potentially making use of reading
> subpages.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> Please review carefully. It obviously works and speeds up UBI attachment
> from 3s to 2s here in one case and I have not found places where this patch
> makes problems, but there might be a reason I haven't seen why in case of
> the ec header only the header is read while in case of the vid header the
> whole page is read.
> 
>  drivers/mtd/ubi/io.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 10cf3b5..31918a0 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
>  
>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> -			  ubi->vid_hdr_alsize);
> +			  UBI_VID_HDR_SIZE);

Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.

Thanks,
//richard

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Sascha Hauer]

On Thu, Jun 23, 2016 at 04:38:25PM +0200, Richard Weinberger wrote:
> Am 23.06.2016 um 15:29 schrieb Sascha Hauer:
> > When reading the vid hdr from the device UBI always reads a whole
> > page. Instead, read only the data we actually need and speed up
> > attachment of UBI devices by potentially making use of reading
> > subpages.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> > 
> > Please review carefully. It obviously works and speeds up UBI attachment
> > from 3s to 2s here in one case and I have not found places where this patch
> > makes problems, but there might be a reason I haven't seen why in case of
> > the ec header only the header is read while in case of the vid header the
> > whole page is read.
> > 
> >  drivers/mtd/ubi/io.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> > index 10cf3b5..31918a0 100644
> > --- a/drivers/mtd/ubi/io.c
> > +++ b/drivers/mtd/ubi/io.c
> > @@ -1019,7 +1019,7 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
> >  
> >  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> >  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> > -			  ubi->vid_hdr_alsize);
> > +			  UBI_VID_HDR_SIZE);
> 
> Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.

Ok, just tried and indeed it does break. Would it be an option to read
UBI_VID_HDR_SIZE + ubi->vid_hdr_shift bytes instead?


Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Richard Weinberger]

Am 23.06.2016 um 17:06 schrieb Sascha Hauer:
>>>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>>>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
>>> -			  ubi->vid_hdr_alsize);
>>> +			  UBI_VID_HDR_SIZE);
>>
>> Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.
> 
> Ok, just tried and indeed it does break. Would it be an option to read
> UBI_VID_HDR_SIZE + ubi->vid_hdr_shift bytes instead?

Well, you need to satisfy the trick UBI does.
Please read the huge comment on it on top of io.c.

Since in most cases ubi->vid_hdr_shift is 0 we could also do a fast path.
i.e.
if (ubi->vid_hdr_shift)
	read_len = ubi->vid_hdr_alsize
else
	read_len = UBI_VID_HDR_SIZE;

But first I have to review a view call sites. :-)

Can you tell a bit more on the NAND you're facing that speedup?
I find it surprising that you gain a full second.

Thanks,
//richard

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Sascha Hauer]

On Thu, Jun 23, 2016 at 05:16:06PM +0200, Richard Weinberger wrote:
> Am 23.06.2016 um 17:06 schrieb Sascha Hauer:
> >>>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> >>>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> >>> -			  ubi->vid_hdr_alsize);
> >>> +			  UBI_VID_HDR_SIZE);
> >>
> >> Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.
> > 
> > Ok, just tried and indeed it does break. Would it be an option to read
> > UBI_VID_HDR_SIZE + ubi->vid_hdr_shift bytes instead?
> 
> Well, you need to satisfy the trick UBI does.
> Please read the huge comment on it on top of io.c.
> 
> Since in most cases ubi->vid_hdr_shift is 0 we could also do a fast path.
> i.e.
> if (ubi->vid_hdr_shift)
> 	read_len = ubi->vid_hdr_alsize
> else
> 	read_len = UBI_VID_HDR_SIZE;

Yes. I thought reading UBI_VID_HDR_SIZE + ubi->vid_hdr_shift has the
advantage that even with vid_hdr_shift != 0 we can profit from reading
subpages. I tested it with a vid hdr offset of 512 and it works ok.

> 
> But first I have to review a view call sites. :-)

Yes, please. It lowers the chance that I break the kernel ;)

> 
> Can you tell a bit more on the NAND you're facing that speedup?
> I find it surprising that you gain a full second.

It's a Micron Nand with a page size of 8192+448. On an i.MX6 we only
have to read 512 bytes when reading the vid header.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Boris Brezillon]

On Fri, 24 Jun 2016 08:10:37 +0200
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Thu, Jun 23, 2016 at 05:16:06PM +0200, Richard Weinberger wrote:
> > Am 23.06.2016 um 17:06 schrieb Sascha Hauer:  
> > >>>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> > >>>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> > >>> -			  ubi->vid_hdr_alsize);
> > >>> +			  UBI_VID_HDR_SIZE);  
> > >>
> > >> Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.  
> > > 
> > > Ok, just tried and indeed it does break. Would it be an option to read
> > > UBI_VID_HDR_SIZE + ubi->vid_hdr_shift bytes instead?  
> > 
> > Well, you need to satisfy the trick UBI does.
> > Please read the huge comment on it on top of io.c.
> > 
> > Since in most cases ubi->vid_hdr_shift is 0 we could also do a fast path.
> > i.e.
> > if (ubi->vid_hdr_shift)
> > 	read_len = ubi->vid_hdr_alsize
> > else
> > 	read_len = UBI_VID_HDR_SIZE;  
> 
> Yes. I thought reading UBI_VID_HDR_SIZE + ubi->vid_hdr_shift has the
> advantage that even with vid_hdr_shift != 0 we can profit from reading
> subpages. I tested it with a vid hdr offset of 512 and it works ok.
> 
> > 
> > But first I have to review a view call sites. :-)  
> 
> Yes, please. It lowers the chance that I break the kernel ;)
> 
> > 
> > Can you tell a bit more on the NAND you're facing that speedup?
> > I find it surprising that you gain a full second.

Not so surprising to me. I tried the same trick on a 16k page NAND a
while a ago, and it drastically decreased the attach time (don't recall
the exact numbers).

> 
> It's a Micron Nand with a page size of 8192+448. On an i.MX6 we only
> have to read 512 bytes when reading the vid header.
> 
> Sascha
> 

[PATCH] ubi: Speedup ubi_io_read_vid_hdr()

[Richard Weinberger]

Currently we read a whole (sub)page from flash when reading
the VID header.
Sascha reported that reading only UBI_VID_HDR_SIZE instead
gives him a decent speedup during a full scan.

Since we have to obey shifted headers too we have to read
ubi->vid_hdr_shift + UBI_VID_HDR_SIZE bytes.
It is also important to set the buffer to 0xff bytes before
reading as in ubi_io_write_vid_hdr() the whole (sub)page
is written back to flash and we cannot risk leaking
kernel memory into the storage medium.

Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Richard Weinberger <richard@nod.at>
---
Sascha,

can you please this patch a try?

Thanks,
//richard
---
 drivers/mtd/ubi/io.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
index 10cf3b5..803a22d 100644
--- a/drivers/mtd/ubi/io.c
+++ b/drivers/mtd/ubi/io.c
@@ -1018,8 +1018,9 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
 	ubi_assert(pnum >= 0 &&  pnum < ubi->peb_count);
 
 	p = (char *)vid_hdr - ubi->vid_hdr_shift;
+	memset(p, 0xff, ubi->vid_hdr_alsize);
 	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
-			  ubi->vid_hdr_alsize);
+			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
 	if (read_err && read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err))
 		return read_err;
 
-- 
2.7.3

Re: [PATCH] UBI: only read UBI_VID_HDR_SIZE when reading the vid_hdr

[Richard Weinberger]

Am 24.06.2016 um 09:39 schrieb Boris Brezillon:
> On Fri, 24 Jun 2016 08:10:37 +0200
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
>> On Thu, Jun 23, 2016 at 05:16:06PM +0200, Richard Weinberger wrote:
>>> Am 23.06.2016 um 17:06 schrieb Sascha Hauer:  
>>>>>>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
>>>>>>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
>>>>>> -			  ubi->vid_hdr_alsize);
>>>>>> +			  UBI_VID_HDR_SIZE);  
>>>>>
>>>>> Hmm, I fear this will break as soon ubi->vid_hdr_shift is non-zero.  
>>>>
>>>> Ok, just tried and indeed it does break. Would it be an option to read
>>>> UBI_VID_HDR_SIZE + ubi->vid_hdr_shift bytes instead?  
>>>
>>> Well, you need to satisfy the trick UBI does.
>>> Please read the huge comment on it on top of io.c.
>>>
>>> Since in most cases ubi->vid_hdr_shift is 0 we could also do a fast path.
>>> i.e.
>>> if (ubi->vid_hdr_shift)
>>> 	read_len = ubi->vid_hdr_alsize
>>> else
>>> 	read_len = UBI_VID_HDR_SIZE;  
>>
>> Yes. I thought reading UBI_VID_HDR_SIZE + ubi->vid_hdr_shift has the
>> advantage that even with vid_hdr_shift != 0 we can profit from reading
>> subpages. I tested it with a vid hdr offset of 512 and it works ok.
>>
>>>
>>> But first I have to review a view call sites. :-)  
>>
>> Yes, please. It lowers the chance that I break the kernel ;)
>>
>>>
>>> Can you tell a bit more on the NAND you're facing that speedup?
>>> I find it surprising that you gain a full second.
> 
> Not so surprising to me. I tried the same trick on a 16k page NAND a
> while a ago, and it drastically decreased the attach time (don't recall
> the exact numbers).

I assumed it will give you only on large pages (as found on MLC NAND)
a speedup.

Thanks,
//richard

Re: [PATCH] ubi: Speedup ubi_io_read_vid_hdr()

[Richard Weinberger]

Am 25.06.2016 um 10:39 schrieb Richard Weinberger:
> Currently we read a whole (sub)page from flash when reading
> the VID header.
> Sascha reported that reading only UBI_VID_HDR_SIZE instead
> gives him a decent speedup during a full scan.
> 
> Since we have to obey shifted headers too we have to read
> ubi->vid_hdr_shift + UBI_VID_HDR_SIZE bytes.
> It is also important to set the buffer to 0xff bytes before
> reading as in ubi_io_write_vid_hdr() the whole (sub)page
> is written back to flash and we cannot risk leaking
> kernel memory into the storage medium.
> 
> Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Sascha,
> 
> can you please this patch a try?
> 
> Thanks,
> //richard
> ---
>  drivers/mtd/ubi/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 10cf3b5..803a22d 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -1018,8 +1018,9 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
>  	ubi_assert(pnum >= 0 &&  pnum < ubi->peb_count);
>  
>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> +	memset(p, 0xff, ubi->vid_hdr_alsize);

Given a second thought on that, we can drop this memset().

>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> -			  ubi->vid_hdr_alsize);
> +			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
>  	if (read_err && read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err))
>  		return read_err;

Thanks,
//richard

Re: [PATCH] ubi: Speedup ubi_io_read_vid_hdr()

[Sascha Hauer]

On Sat, Jun 25, 2016 at 10:39:51AM +0200, Richard Weinberger wrote:
> Currently we read a whole (sub)page from flash when reading
> the VID header.
> Sascha reported that reading only UBI_VID_HDR_SIZE instead
> gives him a decent speedup during a full scan.
> 
> Since we have to obey shifted headers too we have to read
> ubi->vid_hdr_shift + UBI_VID_HDR_SIZE bytes.
> It is also important to set the buffer to 0xff bytes before
> reading as in ubi_io_write_vid_hdr() the whole (sub)page
> is written back to flash and we cannot risk leaking
> kernel memory into the storage medium.
> 
> Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Sascha,
> 
> can you please this patch a try?

I already did, this was exactly my suggestion after you mentioned that
my initial patch breaks when vid_hdr_shift is non zero.
I tested it without the memset though.

For reference: I tested this on a 8k page size Nand formatted with
ubiformat -O 512 and attached with ubiattach -O 512.

Sascha

> 
> Thanks,
> //richard
> ---
>  drivers/mtd/ubi/io.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/ubi/io.c b/drivers/mtd/ubi/io.c
> index 10cf3b5..803a22d 100644
> --- a/drivers/mtd/ubi/io.c
> +++ b/drivers/mtd/ubi/io.c
> @@ -1018,8 +1018,9 @@ int ubi_io_read_vid_hdr(struct ubi_device *ubi, int pnum,
>  	ubi_assert(pnum >= 0 &&  pnum < ubi->peb_count);
>  
>  	p = (char *)vid_hdr - ubi->vid_hdr_shift;
> +	memset(p, 0xff, ubi->vid_hdr_alsize);
>  	read_err = ubi_io_read(ubi, p, pnum, ubi->vid_hdr_aloffset,
> -			  ubi->vid_hdr_alsize);
> +			  ubi->vid_hdr_shift + UBI_VID_HDR_SIZE);
>  	if (read_err && read_err != UBI_IO_BITFLIPS && !mtd_is_eccerr(read_err))
>  		return read_err;
>  
> -- 
> 2.7.3
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] ubi: Speedup ubi_io_read_vid_hdr()

[Richard Weinberger]

Am 27.06.2016 um 07:22 schrieb Sascha Hauer:
> On Sat, Jun 25, 2016 at 10:39:51AM +0200, Richard Weinberger wrote:
>> Currently we read a whole (sub)page from flash when reading
>> the VID header.
>> Sascha reported that reading only UBI_VID_HDR_SIZE instead
>> gives him a decent speedup during a full scan.
>>
>> Since we have to obey shifted headers too we have to read
>> ubi->vid_hdr_shift + UBI_VID_HDR_SIZE bytes.
>> It is also important to set the buffer to 0xff bytes before
>> reading as in ubi_io_write_vid_hdr() the whole (sub)page
>> is written back to flash and we cannot risk leaking
>> kernel memory into the storage medium.
>>
>> Reported-by: Sascha Hauer <s.hauer@pengutronix.de>
>> Signed-off-by: Richard Weinberger <richard@nod.at>
>> ---
>> Sascha,
>>
>> can you please this patch a try?
> 
> I already did, this was exactly my suggestion after you mentioned that
> my initial patch breaks when vid_hdr_shift is non zero.
> I tested it without the memset though.

Yeah, the memset() is not needed, I was wrong.
So we can take your version.

Thanks,
//richard