Re: [PATCH] ubi: Select fastmap anchor PEBs considering wear level rules

[Richard Weinberger <richard.weinberger@gmail.com>]

On Mon, Jan 13, 2020 at 3:56 PM Arne Edholm <arne.edholm@axis.com> wrote:
>
> There is a risk that the fastmap anchor PEB is alternating between
> just two PEBs, the current anchor and the previous anchor that was just
> deleted. As the fastmap pools gets the first take on free PEBs, the
> pools may leave no free PEBs to be selected as the new anchor,
> resulting in the two PEBs alternating behaviour. If the anchor PEBs gets
> a high erase count the PEBs will not be used by the pools but remain in
> ubi->free, even more increasing the likelihood they will be used as
> anchors.
>
> Getting stuck using only a couple of PEBs continuously will result in an
> uneven wear, eventually leading to failure.
>
> To fix this:
>
> - Choose the fastmap anchor when the most free PEBs are available. This is
>   during rebuilding of the fastmap pools, after the unused pool PEBs are
>   added to ubi->free but before the pools are populated again from the
>   free PEBs. Also reserve an additional second best PEB as a candidate
>   for the next time the fast map anchor is updated. If a better PEB is
>   found the next time the fast map anchor is updated, the candidate is
>   made available for building the pools.
>
> - Enable anchor move within the anchor area again as it is useful for
>   distributing wear.
>
> - The anchor candidate for the next fastmap update is the most suited free
>   PEB. Check this PEB's erase count during wear leveling. If the wear
>   leveling limit is exceeded, the PEB is considered unsuitable for now. As
>   all other non used anchor area PEBs should be even worse, free up the
>   used anchor area PEB with the lowest erase count.
>
> Signed-off-by: Arne Edholm <arne.edholm@axis.com>
> ---
>  drivers/mtd/ubi/fastmap-wl.c | 39 ++++++++++++++++++++++++---------------
>  drivers/mtd/ubi/fastmap.c    | 11 +++++++++++
>  drivers/mtd/ubi/ubi.h        |  4 +++-
>  drivers/mtd/ubi/wl.c         | 28 +++++++++++++++++++---------
>  4 files changed, 57 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/mtd/ubi/fastmap-wl.c b/drivers/mtd/ubi/fastmap-wl.c
> index 426820ab9afe..97cb57a12440 100644
> --- a/drivers/mtd/ubi/fastmap-wl.c
> +++ b/drivers/mtd/ubi/fastmap-wl.c
> @@ -110,6 +110,21 @@ void ubi_refill_pools(struct ubi_device *ubi)
>         wl_pool->size = 0;
>         pool->size = 0;
>
> +       if (ubi->fm_anchor) {
> +               wl_tree_add(ubi->fm_anchor, &ubi->free);
> +               ubi->free_count++;
> +       }
> +       if (ubi->fm_next_anchor) {
> +               wl_tree_add(ubi->fm_next_anchor, &ubi->free);
> +               ubi->free_count++;
> +       }
> +
> +       /* All available PEBs are in ubi->free, now is the time to get
> +        * the best anchor PEBs.
> +        */
> +       ubi->fm_anchor = ubi_wl_get_fm_peb(ubi, 1);
> +       ubi->fm_next_anchor = ubi_wl_get_fm_peb(ubi, 1);
> +
>         for (;;) {
>                 enough = 0;
>                 if (pool->size < pool->max_size) {
> @@ -265,26 +280,20 @@ static struct ubi_wl_entry *get_peb_for_wl(struct ubi_device *ubi)
>  int ubi_ensure_anchor_pebs(struct ubi_device *ubi)
>  {
>         struct ubi_work *wrk;
> -       struct ubi_wl_entry *anchor;
>
>         spin_lock(&ubi->wl_lock);
>
> -       /* Do we already have an anchor? */
> -       if (ubi->fm_anchor) {
> -               spin_unlock(&ubi->wl_lock);
> -               return 0;
> -       }
> -
> -       /* See if we can find an anchor PEB on the list of free PEBs */
> -       anchor = ubi_wl_get_fm_peb(ubi, 1);
> -       if (anchor) {
> -               ubi->fm_anchor = anchor;
> -               spin_unlock(&ubi->wl_lock);
> -               return 0;
> +       /* Do we have a next anchor? */
> +       if (!ubi->fm_next_anchor) {
> +               ubi->fm_next_anchor = ubi_wl_get_fm_peb(ubi, 1);
> +               if (!ubi->fm_next_anchor)
> +                       /* Tell wear leveling to produce a new anchor PEB */
> +                       ubi->fm_do_produce_anchor = 1;
>         }
>
> -       /* No luck, trigger wear leveling to produce a new anchor PEB */
> -       ubi->fm_do_produce_anchor = 1;
> +       /* Do wear leveling to get a new anchor PEB or check the
> +        * existing next anchor candidate.
> +        */
>         if (ubi->wl_scheduled) {
>                 spin_unlock(&ubi->wl_lock);
>                 return 0;
> diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
> index 1c7be4eb3ba6..02332f9ea996 100644
> --- a/drivers/mtd/ubi/fastmap.c
> +++ b/drivers/mtd/ubi/fastmap.c
> @@ -1220,6 +1220,17 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
>                 fm_pos += sizeof(*fec);
>                 ubi_assert(fm_pos <= ubi->fm_size);
>         }
> +       if (ubi->fm_next_anchor) {
> +               fec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
> +
> +               fec->pnum = cpu_to_be32(ubi->fm_next_anchor->pnum);
> +               set_seen(ubi, ubi->fm_next_anchor->pnum, seen_pebs);
> +               fec->ec = cpu_to_be32(ubi->fm_next_anchor->ec);
> +
> +               free_peb_count++;
> +               fm_pos += sizeof(*fec);
> +               ubi_assert(fm_pos <= ubi->fm_size);
> +       }
>         fmh->free_peb_count = cpu_to_be32(free_peb_count);
>
>         ubi_for_each_used_peb(ubi, wl_e, tmp_rb) {
> diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
> index 9688b411c930..a12fdb137fa4 100644
> --- a/drivers/mtd/ubi/ubi.h
> +++ b/drivers/mtd/ubi/ubi.h
> @@ -491,7 +491,8 @@ struct ubi_debug_info {
>   * @fm_work: fastmap work queue
>   * @fm_work_scheduled: non-zero if fastmap work was scheduled
>   * @fast_attach: non-zero if UBI was attached by fastmap
> - * @fm_anchor: The next anchor PEB to use for fastmap
> + * @fm_anchor: The new anchor PEB used during fastmap update
> + * @fm_next_anchor: An anchor PEB candidate for the next time fastmap is updated
>   * @fm_do_produce_anchor: If true produce an anchor PEB in wl
>   *
>   * @used: RB-tree of used physical eraseblocks
> @@ -602,6 +603,7 @@ struct ubi_device {
>         int fm_work_scheduled;
>         int fast_attach;
>         struct ubi_wl_entry *fm_anchor;
> +       struct ubi_wl_entry *fm_next_anchor;
>         int fm_do_produce_anchor;
>
>         /* Wear-leveling sub-system's stuff */
> diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
> index 5d77a38dba54..804c434928aa 100644
> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -688,20 +688,27 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
>         }
>
>  #ifdef CONFIG_MTD_UBI_FASTMAP
> +       e1 = find_anchor_wl_entry(&ubi->used);
> +       if (e1 && ubi->fm_next_anchor &&
> +           (ubi->fm_next_anchor->ec - e1->ec >= UBI_WL_THRESHOLD)) {
> +               ubi->fm_do_produce_anchor = 1;
> +               /* fm_next_anchor is no longer considered a good anchor
> +                * candidate.
> +                * NULL assignment also prevents multiple wear level checks
> +                * of this PEB.
> +                */
> +               wl_tree_add(ubi->fm_next_anchor, &ubi->free);
> +               ubi->fm_next_anchor = NULL;
> +               ubi->free_count++;
> +       }
> +
>         if (ubi->fm_do_produce_anchor) {
> -               e1 = find_anchor_wl_entry(&ubi->used);
>                 if (!e1)
>                         goto out_cancel;

The logic here is a little strange. Why don't you check for e1 being
NULL in the new code block
above?

>                 e2 = get_peb_for_wl(ubi);
>                 if (!e2)
>                         goto out_cancel;
>
> -               /*
> -                * Anchor move within the anchor area is useless.
> -                */
> -               if (e2->pnum < UBI_FM_MAX_START)
> -                       goto out_cancel;
> -

Are you sure we can drop this optimization?

Other than that I like your approach and would merge it. :-)

>                 self_check_in_wl_tree(ubi, e1, &ubi->used);
>                 rb_erase(&e1->u.rb, &ubi->used);
>                 dbg_wl("anchor-move PEB %d to PEB %d", e1->pnum, e2->pnum);
> @@ -1080,8 +1087,11 @@ static int __erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk)
>         if (!err) {
>                 spin_lock(&ubi->wl_lock);
>
> -               if (!ubi->fm_anchor && e->pnum < UBI_FM_MAX_START) {
> -                       ubi->fm_anchor = e;
> +               if (!ubi->fm_next_anchor && e->pnum < UBI_FM_MAX_START) {
> +                       /* Abort anchor production, if needed it will be
> +                        * enabled again in the wear leveling started below.
> +                        */
> +                       ubi->fm_next_anchor = e;
>                         ubi->fm_do_produce_anchor = 0;
>                 } else {
>                         wl_tree_add(e, &ubi->free);
> --
> 2.11.0
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/



-- 
Thanks,
//richard

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/