Re: UBI: Incorrect initial value for ubi->avail_pebs

[Artem Bityutskiy <dedekind@infradead.org>]

Hi,

On Sat, 2007-07-14 at 19:08 +0100, John Smith wrote:
> I suggest that the initial calculation of ubi->avail_pebs
> is incorrect. But perhaps it is intentionally done this way.
> 
> A number of erase blocks, controlled by CONFIG_MTD_UBI_BEB_RESERVE,
> are reserved for handling bad blocks. The number of actual bad blocks
> are also reserved. I think it would be better if the actual bad blocks
> were taken from the CONFIG_MTD_UBI_BEB_RESERVE allocation.

Yeah, we always reserve this configurable percent of good eraseblocks in
case we have more bad eraseblock in future

> 
> In more detail, during scan time the calculation of ubi->avail_pebs
> goes roughly:
> 
>   /* in attach_by_scanning() */
>   ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count;
> 
>   /* in ubi_read_volume_table() */
>   ubi->avail_pebs = ubi->good_peb_count;
> 
>   /* in init_volumes() */
>   reserved_pebs =  sum of volume sizes
>   reserved_pebs += size of layout volume
>   ubi->avail_pebs -= reserved_pebs
> 
>   /* in ubi_wl_init_scan() */
>   #define WL_RESERVED_PEBS 1
>   ubi->avail_pebs -= WL_RESERVED_PEBS
> 
>   /* In calculate_reserved() */
>   ubi->beb_rsvd_level = ubi->good_peb_count/100;
>   ubi->beb_rsvd_level *= CONFIG_MTD_UBI_BEB_RESERVE;
> 
>   /* in ubi_eba_init_scan() */
>   ubi->beb_rsvd_pebs = min(ubi->beb_rsvd_level, ubi->avail_pebs)
>   ubi->avail_pebs -= ubi->beb_rsvd_pebs;
> 
> The effect is that existing bad blocks on the device are allocated
> from the budget of blocks for data rather than from the budget of
> blocks for bad blocks.

When a NAND device is shipped it already has some bad eraseblocks. The
number of them is unknown, but they usually guarantee that the number
does not exceed some limit. I just wanted to ensure we always have a
reserved pool which does not depend on the number of already existing
bad eraseblocks. But yes, I can believe that this approach may be too
simple and cause troubles.

> 
> To fix, I suggest that in ubi_read_volume_table() in vtbl.c the
> initial value of ubi->avail_pebs should be set by
> 
>     ubi->avail_pebs = ubi->peb_count ;
> 
> and for consistancy, the beb_rsvd_level should come from
> 
>   ubi->beb_rsvd_level = ubi->peb_count/100;
>   ubi->beb_rsvd_level *= CONFIG_MTD_UBI_BEB_RESERVE;

Well, may be. Provide some arguments against current approach please.

-- 
Best regards,
Artem Bityutskiy (Битюцкий Артём)