| Message ID | 20180613212344.11608-3-richard@nod.at |
|---|---|
| State | Superseded |
| Delegated to: | Richard Weinberger |
| Headers | show |
| Series | ubi: Fastmap updates | expand |
On Wed, 13 Jun 2018 23:23:32 +0200 Richard Weinberger <richard@nod.at> wrote: > When multiple PEBs are used for a fastmap, found_pebs > can be wrong and the assert triggers. > > The current approach is broken in two ways: > 1. The "continue" in list_for_each_entry() over all fastmap PEBs > misses the counter at all. > 2. When the fastmap changes in size, growing due to a preseeded fastmap > or shrinking due to new bad blocks, iterating over the current > fastmap will give wrong numbers. We have to exclude the fastmap size > at all from the calculation to be able to compare the numbers. > At this stage we simply have no longer the information whether the > fastmap changed in size. Should this patch be backported to stable releases? You say the problem arises when new bad blocks appear, so it's not only a problem you'll have with the preseeded fastmap changes. > > Signed-off-by: Richard Weinberger <richard@nod.at> > --- > drivers/mtd/ubi/wl.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c > index f66b3b22f328..6bbb968fe9da 100644 > --- a/drivers/mtd/ubi/wl.c > +++ b/drivers/mtd/ubi/wl.c > @@ -1695,11 +1695,19 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai) > err = erase_aeb(ubi, aeb, sync); > if (err) > goto out_free; > - } > > - found_pebs++; > + /* > + * If no fastmap is used, all fastmap PEBs will get be remove either "get" or "be" here ^ > + * erased and are member of ai->fastmap. > + */ > + if (!ubi->fm) > + found_pebs++; > + } > } > > + if (ubi->fm) > + found_pebs += ubi->fm->used_blocks; > + Hm, are we sure this is always correct? I mean, what if you had an old fastmap scheduled for erasure but a power-cut happened before it was erased. Are you sure we won't have an inconsistent found_pebs number (found_pebs != ubi->good_peb_count). I understand that this problem already exists because of if (ubi->lookuptbl[aeb->pnum]) continue; but I'm not sure your solution fixes that. > dbg_wl("found %i PEBs", found_pebs); > > ubi_assert(ubi->good_peb_count == found_pebs);
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c index f66b3b22f328..6bbb968fe9da 100644 --- a/drivers/mtd/ubi/wl.c +++ b/drivers/mtd/ubi/wl.c @@ -1695,11 +1695,19 @@ int ubi_wl_init(struct ubi_device *ubi, struct ubi_attach_info *ai) err = erase_aeb(ubi, aeb, sync); if (err) goto out_free; - } - found_pebs++; + /* + * If no fastmap is used, all fastmap PEBs will get be + * erased and are member of ai->fastmap. + */ + if (!ubi->fm) + found_pebs++; + } } + if (ubi->fm) + found_pebs += ubi->fm->used_blocks; + dbg_wl("found %i PEBs", found_pebs); ubi_assert(ubi->good_peb_count == found_pebs);
When multiple PEBs are used for a fastmap, found_pebs can be wrong and the assert triggers. The current approach is broken in two ways: 1. The "continue" in list_for_each_entry() over all fastmap PEBs misses the counter at all. 2. When the fastmap changes in size, growing due to a preseeded fastmap or shrinking due to new bad blocks, iterating over the current fastmap will give wrong numbers. We have to exclude the fastmap size at all from the calculation to be able to compare the numbers. At this stage we simply have no longer the information whether the fastmap changed in size. Signed-off-by: Richard Weinberger <richard@nod.at> --- drivers/mtd/ubi/wl.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)