UBI: Incorrect initial value for ubi->avail_pebs

UBI: Incorrect initial value for ubi->avail_pebs

[John Smith]

Hello,

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.

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.

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;

But I accept that one could argue that the current implementation is
correct.

Regards,
John

John Smith
Blackburn, England.

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

[Artem Bityutskiy]

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 (Битюцкий Артём)