Re: [PATCH v6 12/15] ubi: fastmap: Add all fastmap pebs into 'ai->fastmap' when fm->used_blocks>=2

[Richard Weinberger]

----- Ursprüngliche Mail -----
> Von: "chengzhihao1" <chengzhihao1@xxxxxxxxxx>
> An: "richard" <richard@xxxxxx>
> CC: "Miquel Raynal" <miquel.raynal@xxxxxxxxxxx>, "Vignesh Raghavendra" <vigneshr@xxxxxx>, "mcoquelin stm32"
> <mcoquelin.stm32@xxxxxxxxx>, "kirill shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>, "Sascha Hauer"
> <s.hauer@xxxxxxxxxxxxxx>, "linux-mtd" <linux-mtd@xxxxxxxxxxxxxxxxxxx>, "linux-kernel" <linux-kernel@xxxxxxxxxxxxxxx>
> Gesendet: Samstag, 15. Januar 2022 09:46:07
> Betreff: Re: [PATCH v6 12/15] ubi: fastmap: Add all fastmap pebs into 'ai->fastmap' when fm->used_blocks>=2

>>> Yes. But if you look into ubi_wl_init() you see that fastmap anchor PEBs
>>> get erases synchronously(!). The comment before the erasure explains why.
>> About erasing fastmap anchor PEB synchronously, I admit curreunt UBI
>> implementation cannot satisfy it, even with my fix applied. Wait, it
>> seems that UBI has never made it sure. Old fastmap PEBs could be erased
>> asynchronously, they could be counted into 'fmh->erase_peb_count' even
>> in early UBI implementation code, so old fastmap anchor PEB will be
>> added into 'ai->erase' and be erased asynchronously in next attaching.
> In next attaching old fastmap PEBs will be processed as following:
> ubi_attach_fastmap -> add_aeb(ai, &ai->erase...)
> ubi_wl_init
>   list_for_each_entry_safe(aeb, tmp, &ai->erase)
>     erase_aeb       // erase asynchronously
>       ubi->lookuptbl[e->pnum] = e
>   list_for_each_entry(aeb, &ai->fastmap, u.list)
>     e = ubi_find_fm_block(ubi, aeb->pnum)
>     if (e) {
>        ...
>     } else {
>       if (ubi->lookuptbl[aeb->pnum])     // old fastmap PEBs are
> assigned to 'ubi->lookuptbl'
>         continue;
>     }
>> But, I feel it is not a problem, find_fm_anchor() can help us find out
> > the right fastmap anchor PEB according seqnum.

FYI, I think I understand now our disagreement.
You assume that old Fastmap PEBs are *guaranteed* to be part of Fastmap's erase list.
That's okay and this is what Linux as of today does.

My point is that we need to be paranoid and check carefully for old Fastmap PEBs
which might be *not* on the erase list.
I saw such issues in the wild. These were causes by old and/or buggy Fastmap
implementations.
Keep in mind that Linux is not the only system that implements UBI (and fastmap).

So let me give the whole situation another thought on how to improve it.
I totally agree with you that currently there is a problem with fm->used_blocks > 1.
I'm just careful (maybe too careful) about changing Fastmap code.

Thanks,
//richard