Re: [PATCH v2] UBI: only read necessary size when reading the VID header

[Artem Bityutskiy <dedekind1@gmail.com>]

On Tue, 2016-06-28 at 13:51 +0200, Sascha Hauer wrote:
> 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 if the NAND driver supports it.
> 
> Since the VID header may be at offset vid_hdr_shift in the page and
> we can only read from the beginning of a page we have to add that
> offset to the read size.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
> 
> change since v1:
> - properly handle vid_hdr_shift != 0
> 
>  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..ff8cafe 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_shift + UBI_VID_HDR_SIZE);

There are pages and subpages underneath. Let me use old sizes. Say,
2KiB pages, and 512KiB subpages.

If the VID header is in the first subpage, the MTD level probably needs
to read the entire subpage anyway, because of per-subpage ECC. This is
why we give it a buffer of subpage size.

Now if you give it a buffer of smaller size, say, 256 bytes, MTD will
have to read the subpage into its own buffer first, validate ECC, then
copy 256 bytes from the internal buffer to your buffer. Compare this
with MTD copying data directly to your buffer.

This was the original idea, and there was measurable difference on real
setups. So this is actually a read speed optimization for those old
setups.

Therefore, unless I misunderstood this patch - it introduces a
regression to those old setups at least (forces MTD to use an
intermediate buffer rather than copy data from NAND directly to the
buffer supplied by UBI)

Now, one can argue this depends on how the underlying MTD driver works,
this is true. One may argue that UBI should not make assumptions about
the aspects of the MTD level like that - true as well. But that would
need a bit better fix then.

I am inclined to Nack, but I feel like I may miss something, so just
letting you know instead.