UBI fastmap updates

UBI fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.
Most changes are coding style and kernel doc fixes.

The highlights are:
	- Fastmap stores now the pool size in it's anchor PEB
	  such that users can set custom pool sizes. (E.g using ubinize)

I've addressed all TODOs.
I did not remove this point from the TODO list:
"2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
   test UBI + fastmap with it."

Artem, do we really need a new power-cut emulation infrastructure?
I did some tests using integck (integck -p -m 0 /mnt).
Nothing bad happened. :-)
Ff you disable writing the fastmap at detach time you can also simulate
a power cut. I did that very often. We written fastmap has to be valid at any time!
Also if a power cut happens while writing the fastmap it's CRC will be
bad and fastmap falls back to scanning mode.
So, I'm not 100% sure what fastmap specific power cut tests need to be done.

Another point:
UBI_FM_MAX_POOL_SIZE is currently 256.
A fastmap pool cannot be greater than this value.
Are we fine with this upper limit?

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v14

Enjoy!
//richard

[PATCH 01/16] UBI: Fastmap: Check find_mean_wl_entry()'s return
[PATCH 02/16] UBI: Fastmap: Kernel doc updates
[PATCH 03/16] UBI: Fastmap: Fix license version
[PATCH 04/16] UBI: Fastmap: Rename self_check_fastmap()
[PATCH 05/16] UBI: Fastmap: Address a TODO
[PATCH 06/16] UBI: Fastmap: Address another TODO
[PATCH 07/16] UBI: Fastmap: Address jet another TOOD
[PATCH 08/16] UBI: Fastmap: Address another TOOD
[PATCH 09/16] UBI: Fastmap: Be more verbose on fastmap failure
[PATCH 10/16] UBI: Fastmap: More kernel doc updates
[PATCH 11/16] UBI: Fastmap: Store pool sizes in fastmap
[PATCH 12/16] UBI: Fastmap: Make checkpatch.pl happy
[PATCH 13/16] UBI: Fastmap: Remove one point from TODO list
[PATCH 14/16] UBI: Fastmap: Remove one point from TODO list
[PATCH 15/16] UBI: Fastmap: Remove one point from TODO list
[PATCH 16/16] UBI: Fastmap: Remove one point from TODO list

[PATCH 01/16] UBI: Fastmap: Check find_mean_wl_entry()'s return value

[Richard Weinberger]

find_mean_wl_entry() is allowed to return NULL.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/wl.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index b44cba0..85a3373 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -509,6 +509,10 @@ retry:
 	}
 
 	e = find_mean_wl_entry(ubi, &ubi->free);
+	if (!e) {
+		ubi_err("no free eraseblocks");
+		return -ENOSPC;
+	}
 
 	self_check_in_wl_tree(ubi, e, &ubi->free);
 
-- 
1.7.6.5

[PATCH 02/16] UBI: Fastmap: Kernel doc updates

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c    |    2 +-
 drivers/mtd/ubi/eba.c       |    8 ++++++++
 drivers/mtd/ubi/fastmap.c   |   16 +++++++++-------
 drivers/mtd/ubi/ubi-media.h |    5 +++--
 drivers/mtd/ubi/ubi.h       |    2 ++
 drivers/mtd/ubi/wl.c        |   32 +++++++++++++++++++++++++-------
 6 files changed, 48 insertions(+), 17 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 00e58dd..021c7d3 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -750,7 +750,7 @@ struct ubi_ainf_peb *ubi_early_get_peb(struct ubi_device *ubi,
 /**
  * check_corruption - check the data area of PEB.
  * @ubi: UBI device description object
- * @vid_hrd: the (corrupted) VID header of this PEB
+ * @vid_hdr: the (corrupted) VID header of this PEB
  * @pnum: the physical eraseblock number to check
  *
  * This is a helper function which is used to distinguish between VID header
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index cb30e7a..cb0139c 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1178,6 +1178,7 @@ out_unlock_leb:
 /**
  * print_rsvd_warning - warn about not having enough reserved PEBs.
  * @ubi: UBI device description object
+ * @ai: UBI attach info object
  *
  * This is a helper function for 'ubi_eba_init()' which is called when UBI
  * cannot reserve enough PEBs for bad block handling. This function makes a
@@ -1216,6 +1217,13 @@ static void print_rsvd_warning(struct ubi_device *ubi,
 			ubi->corr_peb_count);
 }
 
+/**
+ * self_check_eba - run a self check on the EBA table construected by fastmap.
+ *
+ * @ubi: UBI device description object
+ * @ai_fastmap: UBI attach info object created by fastmap
+ * @ai_scan: UBI attach info object created by scanning
+ */
 int self_check_eba(struct ubi_device *ubi, struct ubi_attach_info *ai_fastmap,
 		   struct ubi_attach_info *ai_scan)
 {
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index c6927a9..988f620 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -259,7 +259,7 @@ static int update_vol(struct ubi_device *ubi, struct ubi_attach_info *ai,
 }
 
 /**
- * process_pool_aeb - we found a non-empty PEB in a pool
+ * process_pool_aeb - we found a non-empty PEB in a pool.
  * @ubi: UBI device object
  * @ai: attach info object
  * @new_vh: the volume header derived from new_aeb
@@ -313,7 +313,7 @@ static int process_pool_aeb(struct ubi_device *ubi, struct ubi_attach_info *ai,
  * If fastmap detects a free PEB in the pool it has to check whether
  * this PEB has been unmapped after writing the fastmap.
  *
- * @ubi: UBI device object
+ * @ai: UBI attach info object
  * @pnum: The PEB to be unmapped
  */
 static void unmap_peb(struct ubi_attach_info *ai, int pnum)
@@ -338,13 +338,14 @@ static void unmap_peb(struct ubi_attach_info *ai, int pnum)
 }
 
 /**
- * scan_pool - scans a pool for changed (no longer empty PEBs)
+ * scan_pool - scans a pool for changed (no longer empty PEBs).
  * @ubi: UBI device object
  * @ai: attach info object
  * @pebs: an array of all PEB numbers in the to be scanned pool
  * @pool_size: size of the pool (number of entries in @pebs)
  * @max_sqnum: pointer to the maximal sequence number
  * @eba_orphans: list of PEBs which need to be scanned
+ * @free: list of PEBs which are most likely free (and go into @ai->free)
  */
 static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		     int *pebs, int pool_size, unsigned long long *max_sqnum,
@@ -484,6 +485,7 @@ static int self_check_fastmap(struct ubi_attach_info *ai)
 /**
  * ubi_attach_fastmap - creates ubi_attach_info from a fastmap.
  * @ubi: UBI device object
+ * @ai: UBI attach info object
  * @fm_raw: the fastmap it self as byte array
  * @fm_size: size of the fastmap in bytes
  */
@@ -791,7 +793,7 @@ out:
 }
 
 /**
- * ubi_scan_fastmap - scan the fastmap
+ * ubi_scan_fastmap - scan the fastmap.
  * @ubi: UBI device object
  * @ai: UBI attach info to be filled
  *
@@ -1047,7 +1049,7 @@ out:
 }
 
 /**
- * ubi_write_fastmap - writes a fastmap
+ * ubi_write_fastmap - writes a fastmap.
  * @ubi: UBI device object
  * @new_fm: the to be written fastmap
  */
@@ -1279,7 +1281,7 @@ out:
 }
 
 /**
- * erase_block - Manually erase a PEB
+ * erase_block - Manually erase a PEB.
  * @ubi: UBI device object
  * @pnum: PEB to be erased
  */
@@ -1324,7 +1326,7 @@ out:
 }
 
 /**
- * invalidate_fastmap - destroys a fastmap
+ * invalidate_fastmap - destroys a fastmap.
  * @ubi: UBI device object
  * @fm: the fastmap to be destroyed
  */
diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index a0648c1..eaf81a2 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -432,7 +432,8 @@ struct ubi_fm_sb {
  * struct ubi_fm_hdr - header of the fastmap data set
  * @magic: fastmap header magic number (%UBI_FM_HDR_MAGIC)
  * @free_peb_count: number of free PEBs known by this fastmap
- * @free_peb_count: number of used PEBs known by this fastmap
+ * @used_peb_count: number of used PEBs known by this fastmap
+ * @scrub_peb_count: number of to be scrubbed PEBs known by this fastmap
  * @bad_peb_count: number of bad PEBs known by this fastmap
  * @erase_peb_count: number of bad PEBs which have to be erased
  * @vol_count: number of UBI volumes known by this fastmap
@@ -500,7 +501,7 @@ struct ubi_fm_volhdr {
 
 /**
  * struct ubi_fm_eba - denotes an association beween a PEB and LEB
- * @magic EBA table magic number
+ * @magic: EBA table magic number
  * @reserved_pebs: number of table entries
  * @pnum: PEB number of LEB (LEB is the index)
  */
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 0ca01eb..b60818d 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -382,6 +382,8 @@ struct ubi_wl_entry;
  *
  * @fm: in-memory data structure of the currently used fastmap
  * @fm_pool: in-memory data structure of the fastmap pool
+ * @fm_wl_pool: in-memory data structure of the fastmap pool used by the WL
+ * 		sub-system
  * @fm_mutex: serializes ubi_update_fastmap()
  * @fm_sem: allows ubi_update_fastmap() to block EBA table changes
  * @fm_work: fastmap work queue
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 85a3373..06bf985 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -483,13 +483,13 @@ out:
 }
 
 /**
- * __ubi_wl_get_peb - get a physical eraseblock.
+ * __wl_get_peb - get a physical eraseblock.
  * @ubi: UBI device description object
  *
  * This function returns a physical eraseblock in case of success and a
  * negative error code in case of failure. Might sleep.
  */
-static int __ubi_wl_get_peb(struct ubi_device *ubi)
+static int __wl_get_peb(struct ubi_device *ubi)
 {
 	int err;
 	struct ubi_wl_entry *e;
@@ -585,7 +585,7 @@ static void refill_wl_user_pool(struct ubi_device *ubi)
 	return_unused_pool_pebs(ubi, pool);
 
 	for (pool->size = 0; pool->size < pool->max_size; pool->size++) {
-		pool->pebs[pool->size] = __ubi_wl_get_peb(ubi);
+		pool->pebs[pool->size] = __wl_get_peb(ubi);
 		if (pool->pebs[pool->size] < 0)
 			break;
 	}
@@ -604,7 +604,7 @@ void ubi_refill_pools(struct ubi_device *ubi)
 	spin_unlock(&ubi->wl_lock);
 }
 
-/* ubi_wl_get_peb - works exaclty like __ubi_wl_get_peb but keeps track of
+/* ubi_wl_get_peb - works exaclty like __wl_get_peb but keeps track of
  * the fastmap pool.
  */
 int ubi_wl_get_peb(struct ubi_device *ubi)
@@ -630,7 +630,7 @@ int ubi_wl_get_peb(struct ubi_device *ubi)
 	return ret;
 }
 
-/* get_peb_for_wl - returns a PEB to be used internally by the WL sub-system
+/* get_peb_for_wl - returns a PEB to be used internally by the WL sub-system.
  *
  * @ubi: UBI device description object
  */
@@ -818,7 +818,7 @@ static int erase_worker(struct ubi_device *ubi, struct ubi_work *wl_wrk,
 			int cancel);
 
 /**
- * ubi_is_erase_work - checks whether a work is erase work
+ * ubi_is_erase_work - checks whether a work is erase work.
  * @wrk: The work object to be checked
  */
 int ubi_is_erase_work(struct ubi_work *wrk)
@@ -862,6 +862,15 @@ static int schedule_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
 	return 0;
 }
 
+/**
+ * do_sync_erase - run the erase worker synchronously.
+ * @ubi: UBI device description object
+ * @e: the WL entry of the physical eraseblock to erase
+ * @vol_id: the volume ID that last used this PEB
+ * @lnum: the last used logical eraseblock number for the PEB
+ * @torture: if the physical eraseblock has to be tortured
+ *
+ */
 static int do_sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
 			 int vol_id, int lnum, int torture)
 {
@@ -884,8 +893,12 @@ static int do_sync_erase(struct ubi_device *ubi, struct ubi_wl_entry *e,
 /**
  * ubi_wl_put_fm_peb - returns a PEB used in a fastmap to the wear-leveling
  * sub-system.
- *
  * see: ubi_wl_put_peb()
+ *
+ * @ubi: UBI device description object
+ * @fm_e: physical eraseblock to return
+ * @lnum: the last used logical eraseblock number for the PEB
+ * @torture: if this physical eraseblock has to be tortured
  */
 int ubi_wl_put_fm_peb(struct ubi_device *ubi, struct ubi_wl_entry *fm_e,
 		      int lnum, int torture)
@@ -1229,6 +1242,7 @@ out_cancel:
 /**
  * ensure_wear_leveling - schedule wear-leveling if it is needed.
  * @ubi: UBI device description object
+ * @nested: set to non-zero if this function is called from UBI worker
  *
  * This function checks if it is time to start wear-leveling and schedules it
  * if yes. This function returns zero in case of success and a negative error
@@ -1295,6 +1309,10 @@ out_unlock:
 	return err;
 }
 
+/**
+ * ubi_ensure_anchor_pebs - schedule wear-leveling to produce an anchor PEB.
+ * @ubi: UBI device description object
+ */
 int ubi_ensure_anchor_pebs(struct ubi_device *ubi)
 {
 	struct ubi_work *wrk;
-- 
1.7.6.5

[PATCH 03/16] UBI: Fastmap: Fix license version

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 988f620..ba547e2 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -4,7 +4,7 @@
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License.
+ * the Free Software Foundation; version 2.
  *
  * This program is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
-- 
1.7.6.5

[PATCH 04/16] UBI: Fastmap: Rename self_check_fastmap()

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index ba547e2..a596cef 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -462,7 +462,11 @@ out:
 	return ret;
 }
 
-static int self_check_fastmap(struct ubi_attach_info *ai)
+/**
+ * count_fastmap_pebs - Counts the PEBs found by fastmap.
+ * @ai: The UBI attach info object
+ */
+static int count_fastmap_pebs(struct ubi_attach_info *ai)
 {
 	struct ubi_ainf_peb *aeb;
 	struct ubi_ainf_volume *av;
@@ -1006,7 +1010,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	 * We do this here because in ubi_wl_init() it's too late
 	 * and we cannot fall back to scanning.
 	 */
-	if (WARN_ON(self_check_fastmap(ai) != ubi->peb_count -
+	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
 		    ai->bad_peb_count - used_blocks)) {
 		ret = UBI_BAD_FASTMAP;
 		kfree(fm);
-- 
1.7.6.5

[PATCH 05/16] UBI: Fastmap: Address a TODO

[Richard Weinberger]

We cannot use an aggregate data structure because fastmaps
internals parts do not have fixed positions.
They depends on the number of avaliable PEBs.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index a596cef..c2434cd 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -535,10 +535,6 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	if (fm_pos >= fm_size)
 		goto fail_bad;
 
-	/* TODO: this is difficult to read. Can we please have instead an
-	 * aggregate data structure? I did not think hard on it may be you have
-	 * a good reason for this difficult style, but on the first glance it
-	 * does not look like. And where are all the endiness stuff?  */
 	fmhdr = (struct ubi_fm_hdr *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmhdr);
 	if (fm_pos >= fm_size)
-- 
1.7.6.5

[PATCH 06/16] UBI: Fastmap: Address another TODO

[Richard Weinberger]

Fixed.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index c2434cd..4d8ef9e 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -797,13 +797,6 @@ out:
  * @ubi: UBI device object
  * @ai: UBI attach info to be filled
  *
- * TODO: not urgent, but at some point - check the code with kernel doc and fix
- * its complaints.
- *
- * TODO: not urgent, but for consistency, follow the UBI/UBIFS style and put a
- * dot at the end of the first short description sentence (globally):
- *    ubi_scan_fastmap - scan the fastmap. (<-dot).
- *
  * TODO: not urgent, but it is desireble to document error codes in the header
  * comments and probably describe what the function does, if there is something
  * to say (globally).
-- 
1.7.6.5

[PATCH 07/16] UBI: Fastmap: Address jet another TOOD

[Richard Weinberger]

Writing the fastmap upon attach time if ai->is_empty=1
does not make sense. At this stage the erase worker is busy
with erasing all PEBs and fastmap won't get a free anchor PEB.
While detaching we write anyway a fastmap.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c |    8 --------
 1 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 021c7d3..068f11a 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1301,14 +1301,6 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	}
 
 	destroy_ai(ubi, ai);
-
-	/* TODO: UBI auto formats the flash if it is empty (see ubi->is_empty).
-	 * It is currently done so that every sub-system writes initializes its
-	 * own stuff. Well, now it is only the vtbl sub-system - it creates
-	 * empty volume table. And this is why we have "early" function for
-	 * getting free PEBs. Fastmap should do the same - so I guess it is
-	 * good to do it somewhere here. Also, we need to re-create the fastmap
-	 * on-flash data-structures if they were corrupted. */
 	return 0;
 
 out_wl:
-- 
1.7.6.5

[PATCH 08/16] UBI: Fastmap: Address another TOOD

[Richard Weinberger]

This will be "fixed" upon ubinize has fastmap support.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c |   15 ---------------
 1 files changed, 0 insertions(+), 15 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 068f11a..1ac58ec 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1250,21 +1250,6 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (err)
 		goto out_ai;
 
-	/* TODO: currently the fastmap code assumes that the fastmap data
-	 * structures are created only by the kernel when the kernel attaches
-	 * an fastmap-less image. However, this assumption is too limiting and
-	 * for sure people will want to pre-create UBI images with fastmap
-	 * using the ubinize tool. Then they wont have to waste a lot of time
-	 * waiting for full scan and fastmap initializetion during the first
-	 * boot. This is a very important feature for the factory production
-	 * line where every additional minute per device costs a lot.
-	 *
-	 * When you are attaching an MTD device which contains an image
-	 * generated by ubinize with a fastmap, you will not know the
-	 * 'bad_peb_count' value. Most probably it will contain something like
-	 * -1. The same is true for the per-PEB information in the fastmap - it
-	 * won't tell which PEBs are bad. So we need to detect this and iterate
-	 * over all PEBs, find out which are bad, and update 'ai' here. */
 	ubi->bad_peb_count = ai->bad_peb_count;
 	ubi->good_peb_count = ubi->peb_count - ubi->bad_peb_count;
 	ubi->corr_peb_count = ai->corr_peb_count;
-- 
1.7.6.5

[PATCH 09/16] UBI: Fastmap: Be more verbose on fastmap failure

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |   67 ++++++++++++++++++++++++++++++---------------
 1 files changed, 45 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 4d8ef9e..07ac06e 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -299,7 +299,6 @@ static int process_pool_aeb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		av = tmp_av;
 	else {
 		ubi_err("orphaned volume in fastmap pool!");
-
 		return UBI_BAD_FASTMAP;
 	}
 
@@ -378,21 +377,23 @@ static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		pnum = be32_to_cpu(pebs[i]);
 
 		if (ubi_io_is_bad(ubi, pnum)) {
-			dbg_bld("bad PEB in fastmap pool!");
+			ubi_err("bad PEB in fastmap pool!");
 			ret = UBI_BAD_FASTMAP;
 			goto out;
 		}
 
 		err = ubi_io_read_ec_hdr(ubi, pnum, ech, 0);
 		if (err && err != UBI_IO_BITFLIPS) {
-			dbg_bld("unable to read EC header!");
+			ubi_err("unable to read EC header! PEB:%i err:%i",
+				pnum, err);
 			ret = err > 0 ? UBI_BAD_FASTMAP : err;
 			goto out;
 		} else if (ret == UBI_IO_BITFLIPS)
 			scrub = 1;
 
 		if (be32_to_cpu(ech->image_seq) != ubi->image_seq) {
-			dbg_bld("image seq mismatch!");
+			ubi_err("bad image seq: 0x%x, expected: 0x%x",
+				be32_to_cpu(ech->image_seq), ubi->image_seq);
 			err = UBI_BAD_FASTMAP;
 			goto out;
 		}
@@ -540,22 +541,31 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	if (fm_pos >= fm_size)
 		goto fail_bad;
 
-	if (be32_to_cpu(fmhdr->magic) != UBI_FM_HDR_MAGIC)
+	if (be32_to_cpu(fmhdr->magic) != UBI_FM_HDR_MAGIC) {
+		ubi_err("bad fastmap header magic: 0x%x, expected: 0x%x",
+			be32_to_cpu(fmhdr->magic), UBI_FM_HDR_MAGIC);
 		goto fail_bad;
+	}
 
 	fmpl1 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmpl1);
 	if (fm_pos >= fm_size)
 		goto fail_bad;
-	if (be32_to_cpu(fmpl1->magic) != UBI_FM_POOL_MAGIC)
+	if (be32_to_cpu(fmpl1->magic) != UBI_FM_POOL_MAGIC) {
+		ubi_err("bad fastmap pool magic: 0x%x, expected: 0x%x",
+			be32_to_cpu(fmpl1->magic), UBI_FM_POOL_MAGIC);
 		goto fail_bad;
+	}
 
 	fmpl2 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmpl2);
 	if (fm_pos >= fm_size)
 		goto fail_bad;
-	if (be32_to_cpu(fmpl2->magic) != UBI_FM_POOL_MAGIC)
+	if (be32_to_cpu(fmpl2->magic) != UBI_FM_POOL_MAGIC) {
+		ubi_err("bad fastmap pool magic: 0x%x, expected: 0x%x",
+			be32_to_cpu(fmpl2->magic), UBI_FM_POOL_MAGIC);
 		goto fail_bad;
+	}
 
 	/* read EC values from free list */
 	for (i = 0; i < be32_to_cpu(fmhdr->free_peb_count); i++) {
@@ -611,8 +621,12 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		if (be32_to_cpu(fmvhdr->magic) != UBI_FM_VHDR_MAGIC)
+		if (be32_to_cpu(fmvhdr->magic) != UBI_FM_VHDR_MAGIC) {
+			ubi_err("bad fastmap vol header magic: 0x%x, "
+				"expected: 0x%x",
+				be32_to_cpu(fmvhdr->magic), UBI_FM_VHDR_MAGIC);
 			goto fail_bad;
+		}
 
 		av = add_vol(ai, be32_to_cpu(fmvhdr->vol_id),
 			     be32_to_cpu(fmvhdr->used_ebs),
@@ -633,8 +647,12 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		if (fm_pos >= fm_size)
 			goto fail_bad;
 
-		if (be32_to_cpu(fm_eba->magic) != UBI_FM_EBA_MAGIC)
+		if (be32_to_cpu(fm_eba->magic) != UBI_FM_EBA_MAGIC) {
+			ubi_err("bad fastmap EBA header magic: 0x%x, "
+				"expected: 0x%x",
+				be32_to_cpu(fm_eba->magic), UBI_FM_EBA_MAGIC);
 			goto fail_bad;
+		}
 
 		for (j = 0; j < be32_to_cpu(fm_eba->reserved_pebs); j++) {
 			int pnum = be32_to_cpu(fm_eba->pnum[j]);
@@ -694,6 +712,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 			int err;
 
 			if (ubi_io_is_bad(ubi, tmp_aeb->pnum)) {
+				ubi_err("bad PEB in fastmap EBA orphan list");
 				ret = UBI_BAD_FASTMAP;
 				kfree(ech);
 				goto fail;
@@ -701,7 +720,8 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 
 			err = ubi_io_read_ec_hdr(ubi, tmp_aeb->pnum, ech, 0);
 			if (err && err != UBI_IO_BITFLIPS) {
-				dbg_bld("unable to read EC header!");
+				ubi_err("unable to read EC header! PEB:%i "
+					"err:%i", tmp_aeb->pnum, err);
 				ret = err > 0 ? UBI_BAD_FASTMAP : err;
 				kfree(ech);
 
@@ -841,14 +861,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		fm->to_be_tortured[0] = 1;
 
 	if (be32_to_cpu(fmsb->magic) != UBI_FM_SB_MAGIC) {
-		/* TODO: not urgent, but examine all the error messages and
-		 * print more information there. Here you should print what was
-		 * read and what was expected. See io.c and do similarly or
-		 * better.
-		 * Please, change globally. E.g., when you print about bad
-		 * version - print what was expected and what was actually
-		 * found. */
-		ubi_err("super block magic does not match");
+		ubi_err("bad super block magic: 0x%x, expected: 0x%x",
+			be32_to_cpu(fmsb->magic), UBI_FM_SB_MAGIC);
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
 		kfree(fm);
@@ -856,7 +870,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	}
 
 	if (fmsb->version != UBI_FM_FMT_VERSION) {
-		ubi_err("unknown fastmap format version!");
+		ubi_err("bad fastmap version: %i, expected: %i",
+			fmsb->version, UBI_FM_FMT_VERSION);
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
 		kfree(fm);
@@ -865,7 +880,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 
 	used_blocks = be32_to_cpu(fmsb->used_blocks);
 	if (used_blocks > UBI_FM_MAX_BLOCKS || used_blocks < 1) {
-		ubi_err("number of fastmap blocks is invalid");
+		ubi_err("number of fastmap blocks is invalid: %i", used_blocks);
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
 		kfree(fm);
@@ -942,6 +957,10 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 
 		if (i == 0) {
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_SB_VOLUME_ID) {
+				ubi_err("bad fastmap anchor vol_id: 0x%x,"
+					" expected: 0x%x",
+					be32_to_cpu(vh->vol_id),
+					UBI_FM_SB_VOLUME_ID);
 				ret = UBI_BAD_FASTMAP;
 				kfree(fmsb);
 				kfree(fm);
@@ -949,6 +968,10 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			}
 		} else {
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_DATA_VOLUME_ID) {
+				ubi_err("bad fastmap data vol_id: 0x%x,"
+					" expected: 0x%x",
+					be32_to_cpu(vh->vol_id),
+					UBI_FM_DATA_VOLUME_ID);
 				ret = UBI_BAD_FASTMAP;
 				kfree(fmsb);
 				goto free_hdr;
@@ -961,8 +984,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ret = ubi_io_read(ubi, fm_raw + (ubi->leb_size * i), pnum,
 				  ubi->leb_start, ubi->leb_size);
 		if (ret && ret != UBI_IO_BITFLIPS) {
-			ubi_err("unable to read fastmap block# %i (PEB: %i)",
-				i, pnum);
+			ubi_err("unable to read fastmap block# %i (PEB: %i, "
+				"err: %i)", i, pnum, ret);
 			kfree(fmsb);
 			kfree(fm);
 			goto free_hdr;
-- 
1.7.6.5

[PATCH 10/16] UBI: Fastmap: More kernel doc updates

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |   34 ++++++++++++++++++++++++++++++----
 1 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 07ac06e..c234d94 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -20,6 +20,9 @@
  * new_fm_vhdr - allocate a new volume header for fastmap usage.
  * @ubi: UBI device description object
  * @vol_id: the VID of the new header
+ *
+ * Returns a new struct ubi_vid_hdr on success.
+ * NULL indicates out of memory.
  */
 static struct ubi_vid_hdr *new_fm_vhdr(struct ubi_device *ubi, int vol_id)
 {
@@ -48,6 +51,8 @@ out:
  * @pnum: PEB number of the new attach erase block
  * @ec: erease counter of the new LEB
  * @scrub: scrub this PEB after attaching
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 static int add_aeb(struct ubi_attach_info *ai, struct list_head *list,
 		   int pnum, int ec, int scrub)
@@ -79,13 +84,16 @@ static int add_aeb(struct ubi_attach_info *ai, struct list_head *list,
 }
 
 /**
- * add_vol - create and add a new scan volume to ubi_attach_info.
+ * add_vol - create and add a new volume to ubi_attach_info.
  * @ai: ubi_attach_info object
  * @vol_id: VID of the new volume
  * @used_ebs: number of used EBS
  * @data_pad: data padding value of the new volume
  * @vol_type: volume type
  * @last_eb_bytes: number of bytes in the last LEB
+ *
+ * Returns the new struct ubi_ainf_volume on success.
+ * NULL indicates an error.
  */
 static struct ubi_ainf_volume *add_vol(struct ubi_attach_info *ai, int vol_id,
 				       int used_ebs, int data_pad, u8 vol_type,
@@ -170,6 +178,8 @@ static void assign_aeb_to_av(struct ubi_attach_info *ai,
  * @av: the volume this LEB belongs to
  * @new_vh: the volume header derived from new_aeb
  * @new_aeb: the AEB to be examined
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 static int update_vol(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		      struct ubi_ainf_volume *av, struct ubi_vid_hdr *new_vh,
@@ -264,6 +274,8 @@ static int update_vol(struct ubi_device *ubi, struct ubi_attach_info *ai,
  * @ai: attach info object
  * @new_vh: the volume header derived from new_aeb
  * @new_aeb: the AEB to be examined
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 static int process_pool_aeb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 			    struct ubi_vid_hdr *new_vh,
@@ -345,6 +357,9 @@ static void unmap_peb(struct ubi_attach_info *ai, int pnum)
  * @max_sqnum: pointer to the maximal sequence number
  * @eba_orphans: list of PEBs which need to be scanned
  * @free: list of PEBs which are most likely free (and go into @ai->free)
+ *
+ * Returns 0 on success, if the pool is unusable UBI_BAD_FASTMAP is returned.
+ * < 0 indicates an internal error.
  */
 static int scan_pool(struct ubi_device *ubi, struct ubi_attach_info *ai,
 		     int *pebs, int pool_size, unsigned long long *max_sqnum,
@@ -493,6 +508,9 @@ static int count_fastmap_pebs(struct ubi_attach_info *ai)
  * @ai: UBI attach info object
  * @fm_raw: the fastmap it self as byte array
  * @fm_size: size of the fastmap in bytes
+ *
+ * Returns 0 on success, UBI_BAD_FASTMAP if the found fastmap was unusable.
+ * < 0 indicates an internal error.
  */
 static int ubi_attach_fastmap(struct ubi_device *ubi,
 			      struct ubi_attach_info *ai,
@@ -817,9 +835,9 @@ out:
  * @ubi: UBI device object
  * @ai: UBI attach info to be filled
  *
- * TODO: not urgent, but it is desireble to document error codes in the header
- * comments and probably describe what the function does, if there is something
- * to say (globally).
+ * Returns 0 on success, UBI_NO_FASTMAP if no fastmap was found,
+ * UBI_BAD_FASTMAP if one was found but is not usable.
+ * < 0 indicates an internal error.
  */
 int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 {
@@ -1068,6 +1086,8 @@ out:
  * ubi_write_fastmap - writes a fastmap.
  * @ubi: UBI device object
  * @new_fm: the to be written fastmap
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 static int ubi_write_fastmap(struct ubi_device *ubi,
 			     struct ubi_fastmap_layout *new_fm)
@@ -1300,6 +1320,8 @@ out:
  * erase_block - Manually erase a PEB.
  * @ubi: UBI device object
  * @pnum: PEB to be erased
+ *
+ * Returns the new EC value on success, < 0 indicates an internal error.
  */
 static int erase_block(struct ubi_device *ubi, int pnum)
 {
@@ -1345,6 +1367,8 @@ out:
  * invalidate_fastmap - destroys a fastmap.
  * @ubi: UBI device object
  * @fm: the fastmap to be destroyed
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 static int invalidate_fastmap(struct ubi_device *ubi,
 			      struct ubi_fastmap_layout *fm)
@@ -1376,6 +1400,8 @@ static int invalidate_fastmap(struct ubi_device *ubi,
  * ubi_update_fastmap - will be called by UBI if a volume changes or
  * a fastmap pool becomes full.
  * @ubi: UBI device object
+ *
+ * Returns 0 on success, < 0 indicates an internal error.
  */
 int ubi_update_fastmap(struct ubi_device *ubi)
 {
-- 
1.7.6.5

[PATCH 11/16] UBI: Fastmap: Store pool sizes in fastmap

[Richard Weinberger]

Later this can be used by ubinize to allow custom
pool sizes.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/attach.c    |    8 ++++
 drivers/mtd/ubi/build.c     |    4 +-
 drivers/mtd/ubi/fastmap.c   |   84 ++++++++++++++++++++++++++++--------------
 drivers/mtd/ubi/ubi-media.h |    4 ++-
 drivers/mtd/ubi/ubi.h       |    6 +++
 5 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 1ac58ec..7552d25 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1261,6 +1261,14 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (err)
 		goto out_ai;
 
+	if (ubi->fm) {
+		ubi->fm_pool.max_size = ubi->fm->max_pool_size;
+		ubi->fm_wl_pool.max_size = ubi->fm->max_wl_pool_size;
+
+		ubi_msg("fastmap pool size: %d", ubi->fm_pool.max_size);
+		ubi_msg("fastmap WL pool size: %d", ubi->fm_wl_pool.max_size);
+	}
+
 	err = ubi_wl_init(ubi, ai);
 	if (err)
 		goto out_vtbl;
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index d00101e..7094550 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -900,8 +900,8 @@ int ubi_attach_mtd_dev(struct mtd_info *mtd, int ubi_num, int vid_hdr_offset)
 
 	ubi->fm_wl_pool.max_size = UBI_FM_WL_POOL_SIZE;
 
-	ubi_msg("fastmap pool size: %d", ubi->fm_pool.max_size);
-	ubi_msg("fastmap WL pool size: %d", ubi->fm_wl_pool.max_size);
+	ubi_msg("default fastmap pool size: %d", ubi->fm_pool.max_size);
+	ubi_msg("default fastmap WL pool size: %d", ubi->fm_wl_pool.max_size);
 
 	mutex_init(&ubi->buf_mutex);
 	mutex_init(&ubi->ckvol_mutex);
diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index c234d94..e285e3b 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -506,15 +506,14 @@ static int count_fastmap_pebs(struct ubi_attach_info *ai)
  * ubi_attach_fastmap - creates ubi_attach_info from a fastmap.
  * @ubi: UBI device object
  * @ai: UBI attach info object
- * @fm_raw: the fastmap it self as byte array
- * @fm_size: size of the fastmap in bytes
+ * @fm: the fastmap to be attached
  *
  * Returns 0 on success, UBI_BAD_FASTMAP if the found fastmap was unusable.
  * < 0 indicates an internal error.
  */
 static int ubi_attach_fastmap(struct ubi_device *ubi,
 			      struct ubi_attach_info *ai,
-			      void *fm_raw, size_t fm_size)
+			      struct ubi_fastmap_layout *fm)
 {
 	struct list_head used, eba_orphans, free;
 	struct ubi_ainf_volume *av;
@@ -526,9 +525,10 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 	struct ubi_fm_ec *fmec;
 	struct ubi_fm_volhdr *fmvhdr;
 	struct ubi_fm_eba *fm_eba;
-	int ret, i, j;
-	size_t fm_pos = 0;
+	int ret, i, j, pool_size, wl_pool_size;
+	size_t fm_pos = 0, fm_size = fm->size;
 	unsigned long long max_sqnum = 0;
+	void *fm_raw = fm->raw;
 
 	INIT_LIST_HEAD(&used);
 	INIT_LIST_HEAD(&free);
@@ -585,6 +585,34 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		goto fail_bad;
 	}
 
+	pool_size = be16_to_cpu(fmpl1->size);
+	wl_pool_size = be16_to_cpu(fmpl2->size);
+	fm->max_pool_size = be16_to_cpu(fmpl1->max_size);
+	fm->max_wl_pool_size = be16_to_cpu(fmpl2->max_size);
+
+	if (pool_size > UBI_FM_MAX_POOL_SIZE || pool_size < 0) {
+		ubi_err("bad pool size: %i", pool_size);
+		goto fail_bad;
+	}
+
+	if (wl_pool_size > UBI_FM_MAX_POOL_SIZE || wl_pool_size < 0) {
+		ubi_err("bad WL pool size: %i", wl_pool_size);
+		goto fail_bad;
+	}
+
+
+	if (fm->max_pool_size > UBI_FM_MAX_POOL_SIZE ||
+	    fm->max_pool_size < 0) {
+		ubi_err("bad maximal pool size: %i", fm->max_pool_size);
+		goto fail_bad;
+	}
+
+	if (fm->max_wl_pool_size > UBI_FM_MAX_POOL_SIZE ||
+	    fm->max_wl_pool_size < 0) {
+		ubi_err("bad maximal WL pool size: %i", fm->max_wl_pool_size);
+		goto fail_bad;
+	}
+
 	/* read EC values from free list */
 	for (i = 0; i < be32_to_cpu(fmhdr->free_peb_count); i++) {
 		fmec = (struct ubi_fm_ec *)(fm_raw + fm_pos);
@@ -754,13 +782,13 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		kfree(ech);
 	}
 
-	ret = scan_pool(ubi, ai, fmpl1->pebs, be32_to_cpu(fmpl1->size),
-			&max_sqnum, &eba_orphans, &free);
+	ret = scan_pool(ubi, ai, fmpl1->pebs, pool_size, &max_sqnum,
+			&eba_orphans, &free);
 	if (ret)
 		goto fail;
 
-	ret = scan_pool(ubi, ai, fmpl2->pebs, be32_to_cpu(fmpl2->size),
-			&max_sqnum, &eba_orphans, &free);
+	ret = scan_pool(ubi, ai, fmpl2->pebs, wl_pool_size, &max_sqnum,
+			&eba_orphans, &free);
 	if (ret)
 		goto fail;
 
@@ -772,6 +800,16 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 		list_add_tail(&tmp_aeb->u.list, &ai->free);
 	}
 
+	/*
+	 * If fastmap is leaking PEBs (must not happen), raise a
+	 * fat warning and fall back to scanning mode.
+	 * We do this here because in ubi_wl_init() it's too late
+	 * and we cannot fall back to scanning.
+	 */
+	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
+		    ai->bad_peb_count - fm->used_blocks))
+		goto fail_bad;
+
 	return 0;
 
 fail_bad:
@@ -1026,7 +1064,11 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 
 	fmsb->sqnum = sqnum;
 
-	ret = ubi_attach_fastmap(ubi, ai, fm_raw, fm_size);
+	fm->size = fm_size;
+	fm->used_blocks = used_blocks;
+	fm->raw = fm_raw;
+
+	ret = ubi_attach_fastmap(ubi, ai, fm);
 	if (ret) {
 		if (ret > 0)
 			ret = UBI_BAD_FASTMAP;
@@ -1034,22 +1076,6 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		goto free_hdr;
 	}
 
-	/*
-	 * If fastmap is leaking PEBs (must not happen), raise a
-	 * fat warning and fall back to scanning mode.
-	 * We do this here because in ubi_wl_init() it's too late
-	 * and we cannot fall back to scanning.
-	 */
-	if (WARN_ON(count_fastmap_pebs(ai) != ubi->peb_count -
-		    ai->bad_peb_count - used_blocks)) {
-		ret = UBI_BAD_FASTMAP;
-		kfree(fm);
-		goto free_hdr;
-	}
-
-	fm->size = fm_size;
-	fm->used_blocks = used_blocks;
-
 	for (i = 0; i < used_blocks; i++) {
 		struct ubi_wl_entry *e;
 
@@ -1153,7 +1179,8 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	fmpl1 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmpl1);
 	fmpl1->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
-	fmpl1->size = cpu_to_be32(ubi->fm_pool.size);
+	fmpl1->size = cpu_to_be16(ubi->fm_pool.size);
+	fmpl1->max_size = cpu_to_be16(ubi->fm_pool.max_size);
 
 	for (i = 0; i < ubi->fm_pool.size; i++)
 		fmpl1->pebs[i] = cpu_to_be32(ubi->fm_pool.pebs[i]);
@@ -1161,7 +1188,8 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	fmpl2 = (struct ubi_fm_scan_pool *)(fm_raw + fm_pos);
 	fm_pos += sizeof(*fmpl2);
 	fmpl2->magic = cpu_to_be32(UBI_FM_POOL_MAGIC);
-	fmpl2->size = cpu_to_be32(ubi->fm_wl_pool.size);
+	fmpl2->size = cpu_to_be16(ubi->fm_wl_pool.size);
+	fmpl2->max_size = cpu_to_be16(ubi->fm_wl_pool.max_size);
 
 	for (i = 0; i < ubi->fm_wl_pool.size; i++)
 		fmpl2->pebs[i] = cpu_to_be32(ubi->fm_wl_pool.pebs[i]);
diff --git a/drivers/mtd/ubi/ubi-media.h b/drivers/mtd/ubi/ubi-media.h
index eaf81a2..f1b85a4f 100644
--- a/drivers/mtd/ubi/ubi-media.h
+++ b/drivers/mtd/ubi/ubi-media.h
@@ -455,11 +455,13 @@ struct ubi_fm_hdr {
  * struct ubi_fm_scan_pool - Fastmap pool PEBs to be scanned while attaching
  * @magic: pool magic numer (%UBI_FM_POOL_MAGIC)
  * @size: current pool size
+ * @max_size: maximal pool size
  * @pebs: an array containing the location of all PEBs in this pool
  */
 struct ubi_fm_scan_pool {
 	__be32 magic;
-	__be32 size;
+	__be16 size;
+	__be16 max_size;
 	__be32 pebs[UBI_FM_MAX_POOL_SIZE];
 	__be32 padding[4];
 } __packed;
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index b60818d..8e2592d 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -218,12 +218,18 @@ struct ubi_volume_desc;
  * @to_be_tortured: if non-zero tortured this PEB
  * @size: size of the fastmap in bytes
  * @used_blocks: number of used PEBs
+ * @max_pool_size: maximal size of the user pool
+ * @max_wl_pool_size: maximal size of the pooly used by the WL sub-system
+ * @raw: the fastmap itself as byte array (only valid while attaching)
  */
 struct ubi_fastmap_layout {
 	struct ubi_wl_entry *e[UBI_FM_MAX_BLOCKS];
 	int to_be_tortured[UBI_FM_MAX_BLOCKS];
 	size_t size;
 	int used_blocks;
+	int max_pool_size;
+	int max_wl_pool_size;
+	void *raw;
 };
 
 /**
-- 
1.7.6.5

[PATCH 12/16] UBI: Fastmap: Make checkpatch.pl happy

[Richard Weinberger]

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 drivers/mtd/ubi/fastmap.c |   21 ++++++++++++---------
 drivers/mtd/ubi/wl.c      |   17 ++++++++---------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index e285e3b..2c7c350 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -668,7 +668,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 			goto fail_bad;
 
 		if (be32_to_cpu(fmvhdr->magic) != UBI_FM_VHDR_MAGIC) {
-			ubi_err("bad fastmap vol header magic: 0x%x, "
+			ubi_err("bad fastmap vol header magic: 0x%x, " \
 				"expected: 0x%x",
 				be32_to_cpu(fmvhdr->magic), UBI_FM_VHDR_MAGIC);
 			goto fail_bad;
@@ -694,7 +694,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 			goto fail_bad;
 
 		if (be32_to_cpu(fm_eba->magic) != UBI_FM_EBA_MAGIC) {
-			ubi_err("bad fastmap EBA header magic: 0x%x, "
+			ubi_err("bad fastmap EBA header magic: 0x%x, " \
 				"expected: 0x%x",
 				be32_to_cpu(fm_eba->magic), UBI_FM_EBA_MAGIC);
 			goto fail_bad;
@@ -766,7 +766,7 @@ static int ubi_attach_fastmap(struct ubi_device *ubi,
 
 			err = ubi_io_read_ec_hdr(ubi, tmp_aeb->pnum, ech, 0);
 			if (err && err != UBI_IO_BITFLIPS) {
-				ubi_err("unable to read EC header! PEB:%i "
+				ubi_err("unable to read EC header! PEB:%i " \
 					"err:%i", tmp_aeb->pnum, err);
 				ret = err > 0 ? UBI_BAD_FASTMAP : err;
 				kfree(ech);
@@ -1013,7 +1013,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 
 		if (i == 0) {
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_SB_VOLUME_ID) {
-				ubi_err("bad fastmap anchor vol_id: 0x%x,"
+				ubi_err("bad fastmap anchor vol_id: 0x%x," \
 					" expected: 0x%x",
 					be32_to_cpu(vh->vol_id),
 					UBI_FM_SB_VOLUME_ID);
@@ -1024,7 +1024,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 			}
 		} else {
 			if (be32_to_cpu(vh->vol_id) != UBI_FM_DATA_VOLUME_ID) {
-				ubi_err("bad fastmap data vol_id: 0x%x,"
+				ubi_err("bad fastmap data vol_id: 0x%x," \
 					" expected: 0x%x",
 					be32_to_cpu(vh->vol_id),
 					UBI_FM_DATA_VOLUME_ID);
@@ -1040,7 +1040,7 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		ret = ubi_io_read(ubi, fm_raw + (ubi->leb_size * i), pnum,
 				  ubi->leb_start, ubi->leb_size);
 		if (ret && ret != UBI_IO_BITFLIPS) {
-			ubi_err("unable to read fastmap block# %i (PEB: %i, "
+			ubi_err("unable to read fastmap block# %i (PEB: %i, " \
 				"err: %i)", i, pnum, ret);
 			kfree(fmsb);
 			kfree(fm);
@@ -1305,7 +1305,8 @@ static int ubi_write_fastmap(struct ubi_device *ubi,
 	}
 
 	fmsb->data_crc = 0;
-	fmsb->data_crc = cpu_to_be32(crc32(UBI_CRC32_INIT, fm_raw, new_fm->size));
+	fmsb->data_crc = cpu_to_be32(crc32(UBI_CRC32_INIT, fm_raw,
+					   new_fm->size));
 
 	for (i = 1; i < new_fm->used_blocks; i++) {
 		dvhdr->sqnum = cpu_to_be64(ubi_next_sqnum(ubi));
@@ -1510,7 +1511,8 @@ int ubi_update_fastmap(struct ubi_device *ubi)
 				int j;
 
 				for (j = 1; j < i; j++)
-					ubi_wl_put_fm_peb(ubi, new_fm->e[j], j, 0);
+					ubi_wl_put_fm_peb(ubi, new_fm->e[j],
+							  j, 0);
 
 				ubi_err("could not erase old fastmap PEB");
 				goto err;
@@ -1541,7 +1543,8 @@ int ubi_update_fastmap(struct ubi_device *ubi)
 				ubi_err("could not erase old anchor PEB");
 
 				for (i = 1; i < new_fm->used_blocks; i++)
-					ubi_wl_put_fm_peb(ubi, new_fm->e[i], i, 0);
+					ubi_wl_put_fm_peb(ubi, new_fm->e[i],
+							  i, 0);
 				goto err;
 			}
 
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 06bf985..28385d2 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -349,8 +349,8 @@ static void prot_queue_add(struct ubi_device *ubi, struct ubi_wl_entry *e)
  * This function looks for a wear leveling entry with erase counter closest to
  * min + @diff, where min is the smallest erase counter.
  */
-static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi, struct rb_root *root,
-					  int diff)
+static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi,
+					  struct rb_root *root, int diff)
 {
 	struct rb_node *p;
 	struct ubi_wl_entry *e, *prev_e = NULL;
@@ -403,12 +403,12 @@ static struct ubi_wl_entry *find_mean_wl_entry(struct ubi_device *ubi,
 		e = rb_entry(root->rb_node, struct ubi_wl_entry, u.rb);
 
 		/* If no fastmap has been written and this WL entry can be used
-		 * as anchor PEB, hold it back and return the second best WL entry
-		 * such that fastmap can use the anchor PEB later. */
+		 * as anchor PEB, hold it back and return the second best
+		 * WL entry such that fastmap can use the anchor PEB later. */
 		if (e && !ubi->fm && e->pnum < UBI_FM_MAX_START)
-			e = rb_entry(rb_next(root->rb_node), struct ubi_wl_entry, u.rb);
-	}
-	else
+			e = rb_entry(rb_next(root->rb_node),
+				     struct ubi_wl_entry, u.rb);
+	} else
 		e = find_wl_entry(ubi, root, WL_FREE_MAX_DIFF/2);
 
 	return e;
@@ -997,8 +997,7 @@ static int wear_leveling_worker(struct ubi_device *ubi, struct ubi_work *wrk,
 		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);
-	}
-	else if (!ubi->scrub.rb_node) {
+	} else if (!ubi->scrub.rb_node) {
 		/*
 		 * Now pick the least worn-out used physical eraseblock and a
 		 * highly worn-out free physical eraseblock. If the erase
-- 
1.7.6.5

[PATCH 13/16] UBI: Fastmap: Remove one point from TODO list

[Richard Weinberger]

fastmap works fine with ro mtd devices.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 TODO |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/TODO b/TODO
index a944159..b1ae872 100644
--- a/TODO
+++ b/TODO
@@ -4,7 +4,6 @@ it is merged.
 For all or most of the items there we need to write a testcase. We can put it
 to the ubi-utils.git repository, to a separate branch at the beginning
 
-1. Test with a R/O MTD device: !(mtd->flags & MTD_WRITEABLE)
 2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
    test UBI + fastmap with it.
 3. Test the autoresize feature
-- 
1.7.6.5

[PATCH 14/16] UBI: Fastmap: Remove one point from TODO list

[Richard Weinberger]

autoresize works too.
It was tested with ubirsvol and ubinize which triggered
an autoresize.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 TODO |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/TODO b/TODO
index b1ae872..7386d3b 100644
--- a/TODO
+++ b/TODO
@@ -6,7 +6,6 @@ to the ubi-utils.git repository, to a separate branch at the beginning
 
 2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
    test UBI + fastmap with it.
-3. Test the autoresize feature
 4. Test 'ubi_flush()'
 5. Test that the same UBI image works fine on both LE and BE machines. I guess
    we can do this using sime kind of emulators?
-- 
1.7.6.5

[PATCH 15/16] UBI: Fastmap: Remove one point from TODO list

[Richard Weinberger]

ubi_flush() ahas been tested more than once.
In early versions fastmap called it itself

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 TODO |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/TODO b/TODO
index 7386d3b..267b8d3 100644
--- a/TODO
+++ b/TODO
@@ -6,6 +6,5 @@ to the ubi-utils.git repository, to a separate branch at the beginning
 
 2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
    test UBI + fastmap with it.
-4. Test 'ubi_flush()'
 5. Test that the same UBI image works fine on both LE and BE machines. I guess
    we can do this using sime kind of emulators?
-- 
1.7.6.5

[PATCH 16/16] UBI: Fastmap: Remove one point from TODO list

[Richard Weinberger]

BE/LE has been tested using qemu-ppc and an x86 box.

Signed-off-by: Richard Weinberger <richard@nod.at>
---
 TODO |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/TODO b/TODO
index 267b8d3..12a019a 100644
--- a/TODO
+++ b/TODO
@@ -6,5 +6,3 @@ to the ubi-utils.git repository, to a separate branch at the beginning
 
 2. Implement power-cut emulation infrastructure similar to what is in UBIFS and
    test UBI + fastmap with it.
-5. Test that the same UBI image works fine on both LE and BE machines. I guess
-   we can do this using sime kind of emulators?
-- 
1.7.6.5

Re: [PATCH 04/16] UBI: Fastmap: Rename self_check_fastmap()

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 7624 bytes --]

On Wed, 2012-06-27 at 17:57 +0200, Richard Weinberger wrote:
> Signed-off-by: Richard Weinberger <richard@nod.at>

Hi, pushed this and added a couple of TODOs. Could you please take a
look? Most TODOs are non-essential and easy, but one is more important -
the one about regression in attach time of non-fastmap images.

Thanks!

From b43ceb01a33b322898232fb05e5cc7f754656421 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Thu, 28 Jun 2012 15:08:29 +0300
Subject: [PATCH 1/2] UBI: Fastmap: rename new_ai to alloc_ia

Just more consistent, we do not use "new" in, e.g., 'ubi_zalloc_vid_hdr()'.
Also move the code to avoid forward references, which we also do not use.

Also get rid of unneeded goto in alloc_ia(). Nothing major.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/attach.c |   41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 7552d25..8204b2d 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -91,7 +91,6 @@
 
 static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
 static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
-static struct ubi_attach_info *new_ai(void);
 
 /* Temporary variables used during scanning */
 static struct ubi_ec_hdr *ech;
@@ -1216,6 +1215,22 @@ out_ai:
 	return err;
 }
 
+static struct ubi_attach_info *alloc_ai(void)
+{
+	static struct ubi_attach_info *ai;
+
+	ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL);
+	if (ai) {
+		INIT_LIST_HEAD(&ai->corr);
+		INIT_LIST_HEAD(&ai->free);
+		INIT_LIST_HEAD(&ai->erase);
+		INIT_LIST_HEAD(&ai->alien);
+		ai->volumes = RB_ROOT;
+	}
+
+	return ai;
+}
+
 /**
  * ubi_attach - attach an MTD device.
  * @ubi: UBI device descriptor
@@ -1229,7 +1244,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	int err;
 	struct ubi_attach_info *ai;
 
-	ai = new_ai();
+	ai = alloc_ai();
 	if (!ai)
 		return -ENOMEM;
 
@@ -1239,7 +1254,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 		err = ubi_scan_fastmap(ubi, ai);
 		if (err > 0) {
 			destroy_ai(ubi, ai);
-			ai = new_ai();
+			ai = alloc_ai();
 			if (!ai)
 				return -ENOMEM;
 
@@ -1279,7 +1294,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 
 	if (ubi->fm && ubi->dbg->chk_gen) {
 		struct ubi_attach_info *scan_ai;
-		scan_ai = new_ai();
+		scan_ai = alloc_ai();
 		if (!scan_ai)
 			goto out_ai;
 
@@ -1395,24 +1410,6 @@ static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	kfree(ai);
 }
 
-static struct ubi_attach_info *new_ai(void)
-{
-	static struct ubi_attach_info *ai;
-
-	ai = kzalloc(sizeof(struct ubi_attach_info), GFP_KERNEL);
-	if (!ai)
-		goto out;
-
-	INIT_LIST_HEAD(&ai->corr);
-	INIT_LIST_HEAD(&ai->free);
-	INIT_LIST_HEAD(&ai->erase);
-	INIT_LIST_HEAD(&ai->alien);
-	ai->volumes = RB_ROOT;
-
-out:
-	return ai;
-}
-
 /**
  * self_check_ai - check the attaching information.
  * @ubi: UBI device description object
-- 
1.7.10


From 400a911b104d88bbf6cc593e8bb24c9865a1c449 Mon Sep 17 00:00:00 2001
From: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
Date: Thu, 28 Jun 2012 15:34:17 +0300
Subject: [PATCH 2/2] UBI: fastmap: some todo and random changes

Add few consmetic changes and a bunch of TODOs.

Signed-off-by: Artem Bityutskiy <artem.bityutskiy@linux.intel.com>
---
 drivers/mtd/ubi/attach.c |   29 +++++++++++++++++++++++++++++
 drivers/mtd/ubi/eba.c    |    7 ++++---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 8204b2d..3d9be42 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -89,6 +89,13 @@
 #include <linux/random.h>
 #include "ubi.h"
 
+/*
+ * TODO: please, no forward declarations. We do not use them in UBI code.
+ * Actually initially I did use them a lot, but when upstreaming, I was asked
+ * to remove. Please, follow this convention as well. Please, change globally.
+ * I mean, I am already used to that _all_ the code is upside-down, let's keep
+ * it that way, or re-structure all the code. :-)
+ */
 static int self_check_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
 static void destroy_ai(struct ubi_device *ubi, struct ubi_attach_info *ai);
 
@@ -1170,6 +1177,7 @@ static int scan_all(struct ubi_device *ubi, struct ubi_attach_info *ai)
 	if (ai->ec_count)
 		ai->mean_ec = div_u64(ai->ec_sum, ai->ec_count);
 
+	/* TODO: if we attach by fastmap, we do not execute this? */
 	err = late_analysis(ubi, ai);
 	if (err)
 		goto out_vidh;
@@ -1251,6 +1259,25 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (force_scan)
 		err = scan_all(ubi, ai);
 	else {
+		/* TODO: this is a regression. If I have an old image, and I do
+		 * not want to use fastmap, I will be forced to waste time for
+		 * useless scan of 64 first eraseblocks. Not good.
+		 *
+		 * Can you teach ubi_scan_fastmap() to use 'scan_peb()'
+		 * function for scanning and build normal ai information? If it
+		 * finds fastmap - it can destroy the collected ai. If it does
+		 * not find, it returns ai. Then you just confinue scanning.
+		 *
+		 * I buess what we'll need is:
+		 * 1. scan_all() -> scan_range(..., int pnum1, int pnum2);
+		 * 2. ubi_scan_fastmap() returns the pnum of the last scanned
+		 *    eraseblock if fastmap was not found;
+		 *    Also 'ubi_scan_fastmap()' uses scan_peb() for scanning.
+		 * 3. You call 'scan_range(..., pnum, c->peb_cnt - 1)' and
+		 *    it continues.
+		 *
+		 * And no regressions.
+		 */
 		err = ubi_scan_fastmap(ubi, ai);
 		if (err > 0) {
 			destroy_ai(ubi, ai);
@@ -1276,6 +1303,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 	if (err)
 		goto out_ai;
 
+	/* TODO: Hmm why this code is not hidden in 'ubi_scan_fastmap()' ? */
 	if (ubi->fm) {
 		ubi->fm_pool.max_size = ubi->fm->max_pool_size;
 		ubi->fm_wl_pool.max_size = ubi->fm->max_wl_pool_size;
@@ -1294,6 +1322,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 
 	if (ubi->fm && ubi->dbg->chk_gen) {
 		struct ubi_attach_info *scan_ai;
+
 		scan_ai = alloc_ai();
 		if (!scan_ai)
 			goto out_ai;
diff --git a/drivers/mtd/ubi/eba.c b/drivers/mtd/ubi/eba.c
index cb0139c..6d6e301 100644
--- a/drivers/mtd/ubi/eba.c
+++ b/drivers/mtd/ubi/eba.c
@@ -1218,11 +1218,12 @@ static void print_rsvd_warning(struct ubi_device *ubi,
 }
 
 /**
- * self_check_eba - run a self check on the EBA table construected by fastmap.
- *
+ * self_check_eba - run a self check on the EBA table constructed by fastmap.
  * @ubi: UBI device description object
  * @ai_fastmap: UBI attach info object created by fastmap
  * @ai_scan: UBI attach info object created by scanning
+ *
+ * TODO: what we do and what return.
  */
 int self_check_eba(struct ubi_device *ubi, struct ubi_attach_info *ai_fastmap,
 		   struct ubi_attach_info *ai_scan)
@@ -1290,6 +1291,7 @@ int self_check_eba(struct ubi_device *ubi, struct ubi_attach_info *ai_fastmap,
 
 				ubi_err("LEB:%i:%i is PEB:%i instead of %i!",
 					vol->vol_id, i, fm_eba[i][j], scan_eba[i][j]);
+				/* TODO: no, please, return error instead */
 				BUG();
 			}
 		}
@@ -1306,7 +1308,6 @@ out_free:
 
 	kfree(scan_eba);
 	kfree(fm_eba);
-
 	return ret;
 }
 
-- 
1.7.10

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

On Sat, 2013-09-28 at 15:55 +0200, Richard Weinberger wrote:
> this series is a collection of all pending UBI fastmap updates.
> 
> Richard Genoud found issues in combination with U-Boot.
> These issues also uncovered other minor issues in some error paths.
> Only one fix is not really fastmap specific and touches generic 
> UBI code, "UBI: simplify image sequence test".
> 

Pushed to linux-ubi, thanks!

-- 
Best Regards,
Artem Bityutskiy

UBI fastmap updates

[Richard Weinberger]

this series is a collection of all pending UBI fastmap updates.

Richard Genoud found issues in combination with U-Boot.
These issues also uncovered other minor issues in some error paths.
Only one fix is not really fastmap specific and touches generic 
UBI code, "UBI: simplify image sequence test".

Thanks,
//richard

Richard Genoud (2):
      UBI: fastmap: fix backward compatibility with image_seq
      UBI: simplify image sequence test

Richard Weinberger (5):
      UBI: fix refill_wl_user_pool()
      UBI: Fix error path in scan_pool()
      UBI: Call scan_all() with correct offset in error case
      UBI: Fix memory leak in ubi_attach_fastmap() error path
      UBI: Add some asserts to ubi_attach_fastmap()

 drivers/mtd/ubi/attach.c  | 11 ++++++-----
 drivers/mtd/ubi/fastmap.c | 41 +++++++++++++++++++++++++++++++++++++----
 drivers/mtd/ubi/wl.c      |  4 ----
 3 files changed, 43 insertions(+), 13 deletions(-)

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 273 bytes --]

On Fri, 2012-08-17 at 15:43 +0200, Richard Weinberger wrote:

> No, data is never added to a half-filled PEB.

OK.

> Fastmap does not share PEBs with other sub-systems by design.

It could in theory add own data to own PEBs.
-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Fri, 17 Aug 2012 16:41:24 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Fri, 2012-08-17 at 15:33 +0200, Richard Weinberger wrote:
> > Am Fri, 17 Aug 2012 16:11:55 +0300
> > schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> > > We do not do anything like this in UBI because UBI does not need
> > > this, it does not have any complex data structures on the media.
> > > 
> > > With fastmap - I am unsure. I think it is not a problem, because
> > > probably you never append more data, you just re-write everything,
> > > right?
> > 
> > Yep, the on-flash fastmap is immutable.
> 
> Here the question is - if you have PEB containing fastmap data,
> half-filled. Will you ever add more data there to the empty space in
> the PEB or not?
> 

No, data is never added to a half-filled PEB.
Fastmap does not share PEBs with other sub-systems by design.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

On Fri, 2012-08-17 at 15:33 +0200, Richard Weinberger wrote:
> Am Fri, 17 Aug 2012 16:11:55 +0300
> schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> > We do not do anything like this in UBI because UBI does not need this,
> > it does not have any complex data structures on the media.
> > 
> > With fastmap - I am unsure. I think it is not a problem, because
> > probably you never append more data, you just re-write everything,
> > right?
> 
> Yep, the on-flash fastmap is immutable.

Here the question is - if you have PEB containing fastmap data,
half-filled. Will you ever add more data there to the empty space in the
PEB or not?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Fri, 17 Aug 2012 16:11:55 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> We do not do anything like this in UBI because UBI does not need this,
> it does not have any complex data structures on the media.
> 
> With fastmap - I am unsure. I think it is not a problem, because
> probably you never append more data, you just re-write everything,
> right?

Yep, the on-flash fastmap is immutable.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1363 bytes --]

On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> It fixes all issues pointed out by Shmulik. :-)
> 
> If you want to test fastmap you can use my git repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17

One thing which just came to my mind why looking at other MTD e-mails. I
do not know if it is relevant for fastmap or not, just want you to
check.

In UBIFS we have we have the "fixup" feature which is used to
work-around dumb flashers which are present in many factories:
http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup

This is because the "correct" UBI flasher should be able to skip empty
space when flashing, see here:
http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo

So with this fixup flag UBIFS will basically read all its eraseblocks
which have empty spece which it will use, and write them back. Just to
make the empty space writable again. This is done only once on the very
first boot.

We do not do anything like this in UBI because UBI does not need this,
it does not have any complex data structures on the media.

With fastmap - I am unsure. I think it is not a problem, because
probably you never append more data, you just re-write everything,
right?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1604 bytes --]

On Tue, 2012-08-07 at 09:29 +0200, Richard Weinberger wrote:
> Am 07.08.2012 06:21, schrieb Artem Bityutskiy:
> > On Mon, 2012-08-06 at 19:36 +0200, Richard Weinberger wrote:
> >> I think we enable fastmap only if a MTD device has more than
> >> UBI_FM_MAX_START*2 PEBs.
> >> Any comments?
> > 
> > With double space one can make it power-cut tolerant, because you should
> > be able to have either old or new fastmap at any point of time.
> 
> UBI_FM_MAX_START*2 has nothing do to with the Fastmap size.
> IMHO we need a threshold where Fastmap makes sense.
> Technically Fastmap can only be used if a MTD device has >= UBI_FM_MAX_START
> PEBs.
> But does this makes sense? Fastmap was invented to speedup attaching on *large* MTDs,
> The benefit in small MTDs is very little.

You may measure when it starts being reasonable to have fastmap enabled,
or interpolate the data you already have (everything is roughly linear,
should be rather easy).

But of course small flashes do not need fastmap.

Also, did you say in the past that you are going to come up with a
document describing the design, its cons and pros, limitations, and some
numbers. It would help a lot. Even a limited document would be better
than none.

Speaking about numbers, how long does it take to re-write fastmap on a
given flash (say, 1GiB or larger, depending on what HW you have)? How
big is fastmap for a given partitions size? I'd really prefer to see
this information in a document, e.g., a text file, rather than spread
over many e-mails.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

Am 07.08.2012 06:21, schrieb Artem Bityutskiy:
> On Mon, 2012-08-06 at 19:36 +0200, Richard Weinberger wrote:
>> I think we enable fastmap only if a MTD device has more than
>> UBI_FM_MAX_START*2 PEBs.
>> Any comments?
> 
> With double space one can make it power-cut tolerant, because you should
> be able to have either old or new fastmap at any point of time.

UBI_FM_MAX_START*2 has nothing do to with the Fastmap size.
IMHO we need a threshold where Fastmap makes sense.
Technically Fastmap can only be used if a MTD device has >= UBI_FM_MAX_START
PEBs.
But does this makes sense? Fastmap was invented to speedup attaching on *large* MTDs,
The benefit in small MTDs is very little.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 352 bytes --]

On Mon, 2012-08-06 at 19:36 +0200, Richard Weinberger wrote:
> I think we enable fastmap only if a MTD device has more than
> UBI_FM_MAX_START*2 PEBs.
> Any comments?

With double space one can make it power-cut tolerant, because you should
be able to have either old or new fastmap at any point of time.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

Am 02.08.2012 16:58, schrieb Artem Bityutskiy:
> On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
>> This is the next round of UBI fastmap updates.
>> It fixes all issues pointed out by Shmulik. :-)
> 
> Hi Richard,
> 
> when I try to attach mtdram (NOR flash), UBI fails:

Fastmap works fine with mtdram and NOR flash.
But if your MTD device has less than UBI_FM_MAX_START (64) PEBs
ubi_io_is_bad() will trigger.
The fix is to disable Fastmap if you have not enough PEBs.

I think we enable fastmap only if a MTD device has more than
UBI_FM_MAX_START*2 PEBs.
Any comments?

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 871 bytes --]

Am 05.08.2012 10:23, schrieb Shmulik Ladkani:
> On Thu, 2 Aug 2012 19:45:38 +0200 Richard Weinberger <richard@nod.at> wrote:
>> Okay, then let's explicitly reserve a few PEBs for fastmap.
>> This should be very easy task.
> 
> Need to consider what's expected when migrating from a former non-FM
> UBI system to an FM enabled system, in the case where all PEBs where
> consumed (reserved) in the former system.

If no PEBs are available no fastmap can be installed.
*Maybe* we can steal some PEBs which reserved for bad block handling.

>> How much PEB should be reserved? 2 x sizeof(fastmap)?
> 
> Since FM does not use EBA's atomic LEB change when writing the new
> fastmap, but instead implements its own FM "leb change" internally -
> then reserving 2x is needed if we'd like to avoid reusing the same
> fastmap PEB.

Yep.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Shmulik Ladkani]

Hi,

On Thu, 2 Aug 2012 19:45:38 +0200 Richard Weinberger <richard@nod.at> wrote:
> Okay, then let's explicitly reserve a few PEBs for fastmap.
> This should be very easy task.

Need to consider what's expected when migrating from a former non-FM
UBI system to an FM enabled system, in the case where all PEBs where
consumed (reserved) in the former system.

> How much PEB should be reserved? 2 x sizeof(fastmap)?

Since FM does not use EBA's atomic LEB change when writing the new
fastmap, but instead implements its own FM "leb change" internally -
then reserving 2x is needed if we'd like to avoid reusing the same
fastmap PEB.

Regards,
Shmulik

Re: UBI fastmap updates

[Richard Weinberger]

Am Fri, 03 Aug 2012 11:47:17 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> > This is the next round of UBI fastmap updates.
> > It fixes all issues pointed out by Shmulik. :-)
> > 
> > If you want to test fastmap you can use my git repo:
> > git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17
> 
> Richard,
> 
> I've added 'stress-test.sh' script to the UBI tests. This script runs
> UBI tests on nandsim of different geometry. I plan to extend it
> further: add mtdram tests, test with bit-flips emulation enabled, may
> be something else.
> 
> We need to make sure all the tests pass and fastmap does not introduce
> regressions.
> 
> Feel free to send patches. I am going to extend the test today, so
> 'git pull' from time to time.
> 
> The tests will run very long time, so for debugging you can always
> comment out unneeded things.
> 
> ATM, I have only nandsim tests with different geometry: 64MiB to 1GiB
> total size, 2KiB and 512 byte pages, 16-256KiB eraseblocks.
> 
> Currently I am running this to unpatched UBI to check if they really
> pass.
> 
> I tried to run it on the patched UBI and hit this issue:
> 
> ======================================================================
> 16MiB nandsim with 16KiB PEB, 512KiB NAND pages, fastmap enabled
> Loaded NAND simulator (16MiB, 16KiB eraseblock, 512 bytes NAND page)
> Running mkvol_basic /dev/ubi0
> Running mkvol_bad /dev/ubi0
> Running mkvol_paral /dev/ubi0
> Running rsvol /dev/ubi0
> Running io_basic /dev/ubi0
> [io_basic] test_basic():70: function write() failed with error 28 (No
> space left on device)
> [io_basic] test_basic():70: written = 15808000, ret = -1
> Error: io_basic failed
> FAILURE
> ======================================================================
> 
> On non-patched UBI it works. I think it is exactly what I was talking
> about yesterday - fastmap grows unexpectedly because you do not
> reserve the space.

Yeah, this must be the case.
I'm wondering why the test passes with the default nandsim settings?
I have also tested with other flash sizes and did lots of tests on
real hardware.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]

On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> It fixes all issues pointed out by Shmulik. :-)
> 
> If you want to test fastmap you can use my git repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17

Richard,

I've added 'stress-test.sh' script to the UBI tests. This script runs
UBI tests on nandsim of different geometry. I plan to extend it further:
add mtdram tests, test with bit-flips emulation enabled, may be
something else.

We need to make sure all the tests pass and fastmap does not introduce
regressions.

Feel free to send patches. I am going to extend the test today, so 'git
pull' from time to time.

The tests will run very long time, so for debugging you can always
comment out unneeded things.

ATM, I have only nandsim tests with different geometry: 64MiB to 1GiB
total size, 2KiB and 512 byte pages, 16-256KiB eraseblocks.

Currently I am running this to unpatched UBI to check if they really
pass.

I tried to run it on the patched UBI and hit this issue:

======================================================================
16MiB nandsim with 16KiB PEB, 512KiB NAND pages, fastmap enabled
Loaded NAND simulator (16MiB, 16KiB eraseblock, 512 bytes NAND page)
Running mkvol_basic /dev/ubi0
Running mkvol_bad /dev/ubi0
Running mkvol_paral /dev/ubi0
Running rsvol /dev/ubi0
Running io_basic /dev/ubi0
[io_basic] test_basic():70: function write() failed with error 28 (No
space left on device)
[io_basic] test_basic():70: written = 15808000, ret = -1
Error: io_basic failed
FAILURE
======================================================================

On non-patched UBI it works. I think it is exactly what I was talking
about yesterday - fastmap grows unexpectedly because you do not reserve
the space.

Enjoy :-)

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

On Thu, 2012-08-02 at 21:50 +0300, Artem Bityutskiy wrote:
> On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> > This is the next round of UBI fastmap updates.
> > It fixes all issues pointed out by Shmulik. :-)
> > 
> > If you want to test fastmap you can use my git repo:
> > git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17
> 
> Richard, would you please rename the 'fm_auto' kernel parameter to
> "fastmap_auto", which is a bit more user-friendly.

Sorry, I meant module parameter. Too late, time to go home.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 466 bytes --]

On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> It fixes all issues pointed out by Shmulik. :-)
> 
> If you want to test fastmap you can use my git repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17

Richard, would you please rename the 'fm_auto' kernel parameter to
"fastmap_auto", which is a bit more user-friendly.

Thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 611 bytes --]

On Thu, 2012-08-02 at 20:03 +0200, Richard Weinberger wrote:
> > If I understand correctly, fastmap size is a function of total PEBs
> > count. You should be able to calculate the maximum size precisely.
> 
> It does.
> I was thinking of 2 x sizeof(fastmap) to have reserved PEBs for the
> currently used fastmap and PEBs for the new to be installed fastmap.

Up to you. If you are fine with the overhead, you can go for 2x, I do
not have objections. But would be nice to include the overhead numbers
when you submit the patches. Also print on initialization.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 20:59:28 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> > How much PEB should be reserved? 2 x sizeof(fastmap)? 
> 
> Is there any reason why it cannot be the _exact_ maximum number? Not
> more and not less.

The fastmap size is an exact number. 
 
> If I understand correctly, fastmap size is a function of total PEBs
> count. You should be able to calculate the maximum size precisely.

It does.
I was thinking of 2 x sizeof(fastmap) to have reserved PEBs for the
currently used fastmap and PEBs for the new to be installed fastmap.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Thu, 2012-08-02 at 19:45 +0200, Richard Weinberger wrote:
> This should be very easy task.

Right. But unfortunately, I had to spend a lot of time writing lengthy
e-mails. You could hold your horses for a minute, ask specific questions
and find out what was my concern.

> How much PEB should be reserved? 2 x sizeof(fastmap)? 

Is there any reason why it cannot be the _exact_ maximum number? Not
more and not less.

If I understand correctly, fastmap size is a function of total PEBs
count. You should be able to calculate the maximum size precisely.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1644 bytes --]

On Thu, 2012-08-02 at 10:03 -0700, Tim Bird wrote:
> > So can I interpret this the following way. Not only fastmap give no
> > guarantees that it exists after an unclean reboot, it does not even give
> > guarantees that it exists after a clean reboot.
> > 
> > Unless I am confused, the fastmap design is over-simplified.
> 
> Fastmap is an optimization.  Maybe I'm missing something, but
> I'm not sure why, if the optimization stopped working, you
> would want to reduce the functionality of the file system.

Fastmap gives huge improvement in attach time. So big that it becomes
not just optimization, but a selling feature. Or very important
optimization.

If you design a system which requires 1s startup time, you probably want
to always guarantee 1s startup time. E.g., if we are talking about a car
system.

You probably may get away with the fact that in case of power cut your
system starts up 10 seconds at the first boot (no fastmap).

But you probably would be disappointed if I say that even if you do
_not_ have power cuts, your system may still startup 10 seconds,
although most of the times it will take 1s.

Does this description help to accept my POW that while we cannot
simplify fastmap and give no improvement for the power cut cases, it is
quite important to guarantee that in normal cases fastmap is always
there, and UBI will always be fast.

If I buy a car which runs 200Km/h on the asphalt, I am OK if it cannot
do this on the cross-country trails, but I am not OK if it sometimes
cannot do 200Km/h even on the asphalt, when the moon is blue.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 20:40:00 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> Hi Tim,
> 
> On Thu, 2012-08-02 at 10:03 -0700, Tim Bird wrote:
> > I'm don't understand what "UBI liability" is.  Can you please
> > clarify? What breaks if the PEBs get consumed?
> 
> let me try. Let's forget about bad blocks and assume we are talking
> about NOR flash. For simplicity.
> 
> Let's also first forget about fastmap so far and talk about the
> current design.
> 
> Suppose you have 100 PEBs on your flash. Suppose UBI reserves 10 for
> its internal needs (volume table, etc). 90 PEBs are available to the
> user.
> 
> User now can create one or many volumes, but the overall size of the
> volumes cannot be larger than 90 LEBs.
> 
> This means that UBI guarantees that you can always fill all volumes
> with data there will always be enough PEBs to map to. This is UBI
> liability.
> 
> UBI will not allow you to create a volume of 100 LEBs because in this
> case it will not be able to guarantee that all LEBs will be writable.
> 
> I have invented this "liability" term on the spot actually. It
> basically means what UBI already "promised", what it reserved an put
> aside.
> 
> Now let's add fastmap support to the picture.
> 
> Suppose fastmap took another 10 PEBs and now we have 80 PEBs for the
> user.
> 
> The user can create a volume of 80 LEBs in size. And UBI has to
> guarantee that the user can at any point of time fill all of them with
> data.
> 
> This means that fastmap in no circumstances can grab any more than 10
> PEBs, because they are all reserved, UBI liability is 80 PEBs.
> 
> On other words, fastmap has to know how much PEBs it needs at the UBI
> initialization time, and reserve them. The _maximum_ value.
> 
> The same way other UBI sub-systems do. E.g., the volume table code
> reserves 2 PEBs, because this is the maximum it needs at any point of
> time. The WL subsystem reserves 1 PEB.
> 
> Of course this is not about reserving any specific PEB, this is just
> accounting - we have a couple of variable for reserved PEBs count.
> 
> So let's return to the error messages I spotted. They say that fastmap
> needs a PEB but cannot find one. The flash is nandsim and has no
> badblocks. Why fastmap did not find one? Because it did not reserve
> enough. And UBI tests create volumes of maximum possible size and fill
> it with data, so all available PEBs are mapped and thus, used.
> 
> What this means that the following situation is possible: the UBI
> volume is not fully filled yet and not all LEBs are mapped, so there
> are available PEBs, and fastmap successfully grows and reduces the
> amount of available PEBs. And when the user writes more data, he gets
> -ENOSPC.
> 
> And this is basically the problem I indicated.
> 
> To make my description complete, let's add NAND to the picture. We
> simply reserve 2% (by default, it is configurable) of PEBs for bad
> blocks handling. This is because vendors typically say that this is
> the maximum amount for the flash life-time.
> 
> If NAND wears-out a lot, and we run out of reserved PEBs, we switch to
> R/O mode, because we cannot anymore keep our "promise", the liability.
> 
> You can look at it this way. If you create an UBI volume, and mount it
> with UBIFS, you usually expect that all the free file-system space is
> available to you.
> 
> You probably will be disappointed if you write your file and get
> -ENOSPC because fastmap does grew and consumed a PEB which which was
> promised to your volume.
> 

Okay, then let's explicitly reserve a few PEBs for fastmap.
This should be very easy task.
How much PEB should be reserved? 2 x sizeof(fastmap)?

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 3377 bytes --]

Hi Tim,

On Thu, 2012-08-02 at 10:03 -0700, Tim Bird wrote:
> I'm don't understand what "UBI liability" is.  Can you please clarify?
> What breaks if the PEBs get consumed?

let me try. Let's forget about bad blocks and assume we are talking
about NOR flash. For simplicity.

Let's also first forget about fastmap so far and talk about the current
design.

Suppose you have 100 PEBs on your flash. Suppose UBI reserves 10 for its
internal needs (volume table, etc). 90 PEBs are available to the user.

User now can create one or many volumes, but the overall size of the
volumes cannot be larger than 90 LEBs.

This means that UBI guarantees that you can always fill all volumes with
data there will always be enough PEBs to map to. This is UBI liability.

UBI will not allow you to create a volume of 100 LEBs because in this
case it will not be able to guarantee that all LEBs will be writable.

I have invented this "liability" term on the spot actually. It basically
means what UBI already "promised", what it reserved an put aside.

Now let's add fastmap support to the picture.

Suppose fastmap took another 10 PEBs and now we have 80 PEBs for the
user.

The user can create a volume of 80 LEBs in size. And UBI has to
guarantee that the user can at any point of time fill all of them with
data.

This means that fastmap in no circumstances can grab any more than 10
PEBs, because they are all reserved, UBI liability is 80 PEBs.

On other words, fastmap has to know how much PEBs it needs at the UBI
initialization time, and reserve them. The _maximum_ value.

The same way other UBI sub-systems do. E.g., the volume table code
reserves 2 PEBs, because this is the maximum it needs at any point of
time. The WL subsystem reserves 1 PEB.

Of course this is not about reserving any specific PEB, this is just
accounting - we have a couple of variable for reserved PEBs count.

So let's return to the error messages I spotted. They say that fastmap
needs a PEB but cannot find one. The flash is nandsim and has no
badblocks. Why fastmap did not find one? Because it did not reserve
enough. And UBI tests create volumes of maximum possible size and fill
it with data, so all available PEBs are mapped and thus, used.

What this means that the following situation is possible: the UBI volume
is not fully filled yet and not all LEBs are mapped, so there are
available PEBs, and fastmap successfully grows and reduces the amount of
available PEBs. And when the user writes more data, he gets -ENOSPC.

And this is basically the problem I indicated.

To make my description complete, let's add NAND to the picture. We
simply reserve 2% (by default, it is configurable) of PEBs for bad
blocks handling. This is because vendors typically say that this is the
maximum amount for the flash life-time.

If NAND wears-out a lot, and we run out of reserved PEBs, we switch to
R/O mode, because we cannot anymore keep our "promise", the liability.

You can look at it this way. If you create an UBI volume, and mount it
with UBIFS, you usually expect that all the free file-system space is
available to you.

You probably will be disappointed if you write your file and get -ENOSPC
because fastmap does grew and consumed a PEB which which was promised to
your volume.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 2 Aug 2012 10:03:04 -0700
schrieb Tim Bird <tim.bird@am.sony.com>:
> >> If everything goes wrong, fastmap makes sure that no fastmap is on
> >> flash.
> >> In case of a powercut we fall back to scanning mode.
> >> R/O mode is overkill IMHO.
> > 
> > So can I interpret this the following way. Not only fastmap give no
> > guarantees that it exists after an unclean reboot, it does not even
> > give guarantees that it exists after a clean reboot.
> > 
> > Unless I am confused, the fastmap design is over-simplified.
> 
> Fastmap is an optimization.  Maybe I'm missing something, but
> I'm not sure why, if the optimization stopped working, you
> would want to reduce the functionality of the file system.

That's *exactly* my point.
If fastmap is available - fine, we have fast boot - yay!
But if something really nasty happens we can safely fall back
to scanning mode.
And fastmap is designed to allow this.

Thanks,
//richard

Re: UBI fastmap updates

[Tim Bird]

On 08/02/2012 09:45 AM, Artem Bityutskiy wrote:
> Richard,
> 
> On Thu, 2012-08-02 at 18:32 +0200, Richard Weinberger wrote:
>>> This should not happen. Fastmap should _reserve_ enough of PEBs for it
>>> to operate. It should always find the PEB to write.
>>
>> What is the benefit?
>> IOW what is wrong with the current approach?
> 
> Several reasons. The main is: fastmap will start consuming PEBs reserved
> for volumes when the amount of available PEBs is just enough to map all
> LEBs. This will break UBI liability.

I'm don't understand what "UBI liability" is.  Can you please clarify?
What breaks if the PEBs get consumed?

> 
>> Why?
>> If everything goes wrong, fastmap makes sure that no fastmap is on
>> flash.
>> In case of a powercut we fall back to scanning mode.
>> R/O mode is overkill IMHO.
> 
> So can I interpret this the following way. Not only fastmap give no
> guarantees that it exists after an unclean reboot, it does not even give
> guarantees that it exists after a clean reboot.
> 
> Unless I am confused, the fastmap design is over-simplified.

Fastmap is an optimization.  Maybe I'm missing something, but
I'm not sure why, if the optimization stopped working, you
would want to reduce the functionality of the file system.

=============================
Tim Bird
Architecture Group Chair, CE Workgroup of the Linux Foundation
Senior Staff Engineer, Sony Network Entertainment
=============================

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 19:45:30 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> Richard,
> 
> On Thu, 2012-08-02 at 18:32 +0200, Richard Weinberger wrote:
> > > This should not happen. Fastmap should _reserve_ enough of PEBs
> > > for it to operate. It should always find the PEB to write.
> > 
> > What is the benefit?
> > IOW what is wrong with the current approach?
> 
> Several reasons. The main is: fastmap will start consuming PEBs
> reserved for volumes when the amount of available PEBs is just enough
> to map all LEBs. This will break UBI liability.

Fastmap is also just a volume.
But if you want I can reserve PEBs for it.

> > Why?
> > If everything goes wrong, fastmap makes sure that no fastmap is on
> > flash.
> > In case of a powercut we fall back to scanning mode.
> > R/O mode is overkill IMHO.
> 
> So can I interpret this the following way. Not only fastmap give no
> guarantees that it exists after an unclean reboot, it does not even
> give guarantees that it exists after a clean reboot.

As I said several times before, fastmap was designed to be able to
fall back to scanning mode.
And yes, if there is currently no fastmap on flash (because you
attached from an old UBI volume) and there are no free PEBs you'll
have no fastmap on flash.
In all other cases you'll have one. At detach time fastmap checks
whether a fastmap is installed or not and installs one if needed.

> Unless I am confused, the fastmap design is over-simplified.

KISS.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

Richard,

On Thu, 2012-08-02 at 18:32 +0200, Richard Weinberger wrote:
> > This should not happen. Fastmap should _reserve_ enough of PEBs for it
> > to operate. It should always find the PEB to write.
> 
> What is the benefit?
> IOW what is wrong with the current approach?

Several reasons. The main is: fastmap will start consuming PEBs reserved
for volumes when the amount of available PEBs is just enough to map all
LEBs. This will break UBI liability.

> Why?
> If everything goes wrong, fastmap makes sure that no fastmap is on
> flash.
> In case of a powercut we fall back to scanning mode.
> R/O mode is overkill IMHO.

So can I interpret this the following way. Not only fastmap give no
guarantees that it exists after an unclean reboot, it does not even give
guarantees that it exists after a clean reboot.

Unless I am confused, the fastmap design is over-simplified.


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 19:17:47 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Thu, 2012-08-02 at 16:51 +0200, Richard Weinberger wrote:
> > Every time fastmap writes a new fastmap to the flash it tries to
> > get a new PEB and returns the old one (used for the old fastmap)
> > back to the WL sub-system.
> 
> OK.
> 
> > If no free PEBs are available (E.g Volume is full or the erase
> > worker is too slow) ubi_wl_get_fm_peb() returns NULL and fastmap
> > reuses the currently used PEB.
> 
> This should not happen. Fastmap should _reserve_ enough of PEBs for it
> to operate. It should always find the PEB to write.

What is the benefit?
IOW what is wrong with the current approach?

> Just like if you create a volume maximum possible size, we guarantee
> that you can fill it with data, and UBI will find enough PEBs for
> that.
> 
> Just like we always have enough PEBs for the volume table.
> 
> The above things are UBI's liabilities.
> 
> In the situation when a lot of PEBs became bad, UBI will switch to R/O
> mode with a scary message if it notices that it does not have enough
> PEBs to satisfy all the liabilities.
> 
> And this is why we reserve 2% of PEBs for bad PEBs handling.

Of course fastmap could also do something like that, but I don't really
see a benefit in this.

> > In this situation ubi_wl_get_fm_peb() may trigger such an error
> > message. If think we should get rid of the message as this is not
> > an error condition. It's a well known execution path.
> 
> Unless I am confused, this should lead to switching to R/O mode
> instead, just like we do when we write to an LEB and do not find a
> PEB to map to.

Why?
If everything goes wrong, fastmap makes sure that no fastmap is on
flash.
In case of a powercut we fall back to scanning mode.
R/O mode is overkill IMHO.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

On Thu, 2012-08-02 at 16:51 +0200, Richard Weinberger wrote:
> Every time fastmap writes a new fastmap to the flash it tries to get a
> new PEB and returns the old one (used for the old fastmap) back to the
> WL sub-system.

OK.

> If no free PEBs are available (E.g Volume is full or the erase worker
> is too slow) ubi_wl_get_fm_peb() returns NULL and fastmap reuses the
> currently used PEB.

This should not happen. Fastmap should _reserve_ enough of PEBs for it
to operate. It should always find the PEB to write.

Just like if you create a volume maximum possible size, we guarantee
that you can fill it with data, and UBI will find enough PEBs for that.

Just like we always have enough PEBs for the volume table.

The above things are UBI's liabilities.

In the situation when a lot of PEBs became bad, UBI will switch to R/O
mode with a scary message if it notices that it does not have enough
PEBs to satisfy all the liabilities.

And this is why we reserve 2% of PEBs for bad PEBs handling.

> In this situation ubi_wl_get_fm_peb() may trigger such an error message.
> If think we should get rid of the message as this is not an error
> condition. It's a well known execution path.

Unless I am confused, this should lead to switching to R/O mode instead,
just like we do when we write to an LEB and do not find a PEB to map to.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 18:18:48 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> > Hmm, and without fastmap it works fine?
> 
> Yes.
> 
> > I don't see much fastmap related here.
> 
> It is related to your changes in attach.c.

Okay, I'll dig into the issue.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

On Thu, 2012-08-02 at 16:59 +0200, Richard Weinberger wrote:
> Am Thu, 02 Aug 2012 17:58:50 +0300
> schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:
> 
> > On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> > > This is the next round of UBI fastmap updates.
> > > It fixes all issues pointed out by Shmulik. :-)
> > 
> > Hi Richard,
> > 
> > when I try to attach mtdram (NOR flash), UBI fails:
> >
>  
> Hmm, and without fastmap it works fine?

Yes.

> I don't see much fastmap related here.

It is related to your changes in attach.c.

> Anyway, I'll try to reproduce.

Please, make sure that UBI tests work fine on mtdram as well.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 17:58:50 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> > This is the next round of UBI fastmap updates.
> > It fixes all issues pointed out by Shmulik. :-)
> 
> Hi Richard,
> 
> when I try to attach mtdram (NOR flash), UBI fails:
>
 
Hmm, and without fastmap it works fine?
I don't see much fastmap related here.
Anyway, I'll try to reproduce.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> It fixes all issues pointed out by Shmulik. :-)

Hi Richard,

when I try to attach mtdram (NOR flash), UBI fails:

[ 7106.353791] UBI assert failed in ubi_io_is_bad at 623 (pid 5411)
[ 7106.354253] Pid: 5411, comm: modprobe Not tainted 3.5.0+ #2
[ 7106.354255] Call Trace:
[ 7106.354264]  [<ffffffffa003c4c1>] ubi_io_is_bad+0xd1/0x100 [ubi]
[ 7106.354271]  [<ffffffffa0042bd9>] scan_peb+0x49/0x740 [ubi]
[ 7106.354287]  [<ffffffff8117e6e4>] ? __kmalloc+0x194/0x1e0
[ 7106.354293]  [<ffffffffa0047b7c>] ? ubi_debugging_init_dev+0x2c/0x60 [ubi]
[ 7106.354300]  [<ffffffffa0044276>] ? ubi_attach+0x66/0x380 [ubi]
[ 7106.354304]  [<ffffffffa00442f5>] ubi_attach+0xe5/0x380 [ubi]
[ 7106.354310]  [<ffffffffa0035f91>] ubi_attach_mtd_dev+0xa81/0x10e0 [ubi]
[ 7106.354316]  [<ffffffffa005c3e8>] ubi_init+0x22d/0xe45 [ubi]
[ 7106.354321]  [<ffffffffa005c1bb>] ? ubi_mtd_param_parse+0x1bb/0x1bb [ubi]
[ 7106.354328]  [<ffffffff8100203f>] do_one_initcall+0x3f/0x170
[ 7106.354335]  [<ffffffff810c69c6>] sys_init_module+0xa16/0x1d40
[ 7106.354345]  [<ffffffff8132c5a0>] ? ddebug_add_module+0x100/0x100
[ 7106.354351]  [<ffffffff815ffca9>] system_call_fastpath+0x16/0x1b
[ 7106.354353] UBI assert failed in ubi_io_read_ec_hdr at 749 (pid 5411)

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Am Thu, 02 Aug 2012 17:29:01 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Thu, 2012-08-02 at 16:15 +0200, Richard Weinberger wrote:
> > > If I understand correctly, it can be only because of a bug. If I
> > > am correct, could you please add a 'dump_stack()' to improve the
> > > error report?
> > > 
> > 
> > This can happen if all PEBs are used and fastmap is unable to find
> > (or produce) an empty one.
> 
> In which situations is this possible? Could you please give an
> example?
> 

Every time fastmap writes a new fastmap to the flash it tries to get a
new PEB and returns the old one (used for the old fastmap) back to the
WL sub-system.
If no free PEBs are available (E.g Volume is full or the erase worker
is too slow) ubi_wl_get_fm_peb() returns NULL and fastmap reuses the
currently used PEB.
In this situation ubi_wl_get_fm_peb() may trigger such an error message.
If think we should get rid of the message as this is not an error
condition. It's a well known execution path.
The only bad thing that happens in such a situation is that a PEB gets
reused.

BTW: Which version of fastmap are you testing?

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 446 bytes --]

On Thu, 2012-08-02 at 16:15 +0200, Richard Weinberger wrote:
> > If I understand correctly, it can be only because of a bug. If I am
> > correct, could you please add a 'dump_stack()' to improve the error
> > report?
> > 
> 
> This can happen if all PEBs are used and fastmap is unable to find (or
> produce) an empty one.

In which situations is this possible? Could you please give an example?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

Artem,

Am Thu, 02 Aug 2012 17:12:27 +0300
schrieb Artem Bityutskiy <artem.bityutskiy@linux.intel.com>:

> On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> > This is the next round of UBI fastmap updates.
> > It fixes all issues pointed out by Shmulik. :-)
> 
> I see the following errors when rung UBI tests on nandsim:
> 
> [ 3698.041511] UBI error: __wl_get_peb: no free eraseblocks
> [ 3698.041781] UBI error: ubi_wl_get_fm_peb: no free eraseblocks
> [ 3714.773064] UBI error: __wl_get_peb: no free eraseblocks
> [ 3714.773336] UBI error: ubi_wl_get_fm_peb: no free eraseblocks
> 
> How can this happen? I do not have any bad blocks.
> 
> If I understand correctly, it can be only because of a bug. If I am
> correct, could you please add a 'dump_stack()' to improve the error
> report?
> 

This can happen if all PEBs are used and fastmap is unable to find (or
produce) an empty one.
In this case fastmap takes care that no invalid or outdated fastmap is
on flash.

Thanks,
//richard

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 724 bytes --]

On Mon, 2012-07-09 at 14:18 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> It fixes all issues pointed out by Shmulik. :-)

I see the following errors when rung UBI tests on nandsim:

[ 3698.041511] UBI error: __wl_get_peb: no free eraseblocks
[ 3698.041781] UBI error: ubi_wl_get_fm_peb: no free eraseblocks
[ 3714.773064] UBI error: __wl_get_peb: no free eraseblocks
[ 3714.773336] UBI error: ubi_wl_get_fm_peb: no free eraseblocks

How can this happen? I do not have any bad blocks.

If I understand correctly, it can be only because of a bug. If I am
correct, could you please add a 'dump_stack()' to improve the error
report?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

UBI fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.
It fixes all issues pointed out by Shmulik. :-)

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v17

Enjoy!
//richard

[PATCH 1/7] UBI: Fastmap: Fix lock imbalance in case of an error
[PATCH 2/7] UBI: Fastmap: Get rid of find_fastmap switch
[PATCH 3/7] UBI: Fastmap: Fix memory leak in error path
[PATCH 4/7] UBI: Fastmap: Fix double free in error path
[PATCH 5/7] UBI: Fastmap: Kerneldoc fixes
[PATCH 6/7] UBI: Fastmap: Make sure that find_wl_entry() never
[PATCH 7/7] UBI: Fastmap: Make checkpatch.pl happy

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 760 bytes --]

Am 09.07.2012 09:37, schrieb Shmulik Ladkani:
> Hi Richard,
> 
> On Sun, 08 Jul 2012 14:07:41 +0200 Richard Weinberger <richard@nod.at> wrote:
>>> +			/* TODO: in the new locking scheme, produce_free_peb is
>>> +			 * called under wl_lock taken.
>>> +			 * so when returning, should reacquire the lock
>>> +			 */
>>
>> Which new locking scheme?
> 
> I am diffing linux-ubi fastmap HEAD against its fork point (vanilla
> ubi), that's 6b16351..d41a140 on linux-ubi.
> 
> Which gives the following diff in produce_free_pebs:

Ahh. _my_ new locking scheme. I feared someone else changed it meanwhile in mainline. ;)
Yes, the &ubi->wl_lock in produce_free_peb() is no  longer needed.
Again, thanks for pointing this out!

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Shmulik Ladkani]

Hi Richard,

On Sun, 08 Jul 2012 14:07:41 +0200 Richard Weinberger <richard@nod.at> wrote:
> > +			/* TODO: in the new locking scheme, produce_free_peb is
> > +			 * called under wl_lock taken.
> > +			 * so when returning, should reacquire the lock
> > +			 */
> 
> Which new locking scheme?

I am diffing linux-ubi fastmap HEAD against its fork point (vanilla
ubi), that's 6b16351..d41a140 on linux-ubi.

Which gives the following diff in produce_free_pebs:

@@ -261,7 +266,6 @@ static int produce_free_peb(struct ubi_device *ubi)
 {
 	int err;
 
-	spin_lock(&ubi->wl_lock);
 	while (!ubi->free.rb_node) {
 		spin_unlock(&ubi->wl_lock);
 
@@ -272,7 +276,6 @@ static int produce_free_peb(struct ubi_device *ubi)
 
 		spin_lock(&ubi->wl_lock);
 	}
-	spin_unlock(&ubi->wl_lock);
 
 	return 0;
 }

Which is probably okay, since you obtain the lock in the new
'ubi_refill_pools', which calls produce_free_peb:

+void ubi_refill_pools(struct ubi_device *ubi)
+{
+	spin_lock(&ubi->wl_lock);
+	refill_wl_pool(ubi);
+	refill_wl_user_pool(ubi);
+	spin_unlock(&ubi->wl_lock);
+}

However if 'do_work' fails within 'produce_free_peb', you return the
error but leave wl_lock unlocked - where it is expected to be locked
(otherwise, ubi_refill_pools will unlock it again):

static int produce_free_peb(struct ubi_device *ubi)
{
	int err;

	while (!ubi->free.rb_node) {
		spin_unlock(&ubi->wl_lock);

		dbg_wl("do one work synchronously");
		err = do_work(ubi);
		if (err)
			return err;

		spin_lock(&ubi->wl_lock);
	}

	return 0;
}

Regards,
Shmulik

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 885 bytes --]

Am 08.07.2012 14:07, schrieb Richard Weinberger:
> Hi Shmulik!
> 
> Am 08.07.2012 13:47, schrieb Shmulik Ladkani:
>> +
>> +		/* TODO: if find_fastmap==1, we do not enter this block at all.
>> +		 * shouldn't we? shouldn't we care of compatability of unknown
>> +		 * internal volumes OTHER than the fastmap ones, even if
>> +		 * find_fastmap==1?
>> +		 */
>> +
> 
> If find_fastmap=1 we scan only the first 64 PEBs (now by using scan_peb()).
> When using fastmap can do not care about compatibility of unknown internal volumes
> at all.
> Fastmap keeps only track of known (and compatible volumes).
> Taking care of unknown internal volumes would imply a full scan.

Please forget the above statement.
We have to think of the case where no fastmap was found and the
64 scanned PEBs get reused by scan_all().
Thanks for pointing this out!

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 3814 bytes --]

Hi Shmulik!

Am 08.07.2012 13:47, schrieb Shmulik Ladkani:
> +
> +		/* TODO: if find_fastmap==1, we do not enter this block at all.
> +		 * shouldn't we? shouldn't we care of compatability of unknown
> +		 * internal volumes OTHER than the fastmap ones, even if
> +		 * find_fastmap==1?
> +		 */
> +

If find_fastmap=1 we scan only the first 64 PEBs (now by using scan_peb()).
When using fastmap can do not care about compatibility of unknown internal volumes
at all.
Fastmap keeps only track of known (and compatible volumes).
Taking care of unknown internal volumes would imply a full scan.

		int lnum = be32_to_cpu(vidh->lnum);
>  
>  		/* Unsupported internal volume */
> @@ -1221,6 +1228,7 @@ static void destroy_ai(struct ubi_attach_info *ai)
>   * scan_all - scan entire MTD device.
>   * @ubi: UBI device description object
>   * @ai: attach info object
> + * TODO: document @start

Tomorrow I'll send another kernel-doc update.
There more tags missing.

>   * This function does full scanning of an MTD device and returns complete
>   * information about it in form of a "struct ubi_attach_info" object. In case
> @@ -1350,6 +1358,11 @@ out_vidh:
>  out_ech:
>  	kfree(ech);
>  out_ai:
> +	/* TODO: doesn't seem clean to destroy 'ai' as it was allocated by the
> +	 * caller, its his responsibility.
> +	 * also looks like it leads to double freee in case 'err' returned is
> +	 * negative
> +	 */

I have to look closer into this.
It looks like the call to destroy_ai() in scan_fast() has to go.

>  	destroy_ai(ai);
>  	return err;
>  }
> @@ -1441,6 +1454,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
>  
>  		err = scan_all(ubi, scan_ai, 0);
>  		if (err) {
> +			/* TODO: hmm... kfree or destroy_ai ? */

True. Must be desroy_ai().

>  			kfree(scan_ai);
>  			goto out_wl;
>  		}
> diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
> index 50b7590..f769c22 100644
> --- a/drivers/mtd/ubi/build.c
> +++ b/drivers/mtd/ubi/build.c
> @@ -1053,6 +1053,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
>  	ubi_notify_all(ubi, UBI_VOLUME_REMOVED, NULL);
>  	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
>  
> +	/* TODO: any action on failure? */

What do you expect on failure?
At this point we have either no fastmap or a valid fastmap on flash.
If ubi_update_fastmap() fails it will ensure that no invalid fastmap remains on flash.
See invalidate_fastmap().

> --- a/drivers/mtd/ubi/wl.c
> +++ b/drivers/mtd/ubi/wl.c
> @@ -272,6 +272,10 @@ static int produce_free_peb(struct ubi_device *ubi)
>  		dbg_wl("do one work synchronously");
>  		err = do_work(ubi);
>  		if (err)
> +			/* TODO: in the new locking scheme, produce_free_peb is
> +			 * called under wl_lock taken.
> +			 * so when returning, should reacquire the lock
> +			 */

Which new locking scheme?

>  			return err;
>  
>  		spin_lock(&ubi->wl_lock);
> @@ -377,6 +381,7 @@ static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi,
>  	 * as anchor PEB, hold it back and return the second best WL entry
>  	 * such that fastmap can use the anchor PEB later. */
>  	if (!ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START)
> +		/* TODO: do we have a risk returning NULL here? */

How?

>  		return prev_e;
>  
>  	return e;
> @@ -405,6 +410,7 @@ static struct ubi_wl_entry *find_mean_wl_entry(struct ubi_device *ubi,
>  		/* If no fastmap has been written and this WL entry can be used
>  		 * as anchor PEB, hold it back and return the second best
>  		 * WL entry such that fastmap can use the anchor PEB later. */
> +		/* TODO: why this is specific just to < WL_FREE_MAX_DIFF case? */

Because find_wl_entry() already takes care of this.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Shmulik Ladkani]

Hi Richard,

On Fri, 29 Jun 2012 17:14:18 +0200 Richard Weinberger <richard@nod.at> wrote:
> This is the next round of UBI fastmap updates.

Please examine some TODOs (and questions) I've spotted while diffing
against "vanilla" ubi.

This patch should apply to linux-ubi at d41a140

Sorry I couldn't review entirely, diff has gotten really enormous...
I'll try to pick it up again when you'll get to the final merge
submitting incremental patches.

Regards,
Shmulik

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index a343a41..dae9674 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -999,6 +999,13 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 
 	if (!find_fastmap &&
 	   (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID)) {
+
+		/* TODO: if find_fastmap==1, we do not enter this block at all.
+		 * shouldn't we? shouldn't we care of compatability of unknown
+		 * internal volumes OTHER than the fastmap ones, even if
+		 * find_fastmap==1?
+		 */
+
 		int lnum = be32_to_cpu(vidh->lnum);
 
 		/* Unsupported internal volume */
@@ -1221,6 +1228,7 @@ static void destroy_ai(struct ubi_attach_info *ai)
  * scan_all - scan entire MTD device.
  * @ubi: UBI device description object
  * @ai: attach info object
+ * TODO: document @start
  *
  * This function does full scanning of an MTD device and returns complete
  * information about it in form of a "struct ubi_attach_info" object. In case
@@ -1350,6 +1358,11 @@ out_vidh:
 out_ech:
 	kfree(ech);
 out_ai:
+	/* TODO: doesn't seem clean to destroy 'ai' as it was allocated by the
+	 * caller, its his responsibility.
+	 * also looks like it leads to double freee in case 'err' returned is
+	 * negative
+	 */
 	destroy_ai(ai);
 	return err;
 }
@@ -1441,6 +1454,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 
 		err = scan_all(ubi, scan_ai, 0);
 		if (err) {
+			/* TODO: hmm... kfree or destroy_ai ? */
 			kfree(scan_ai);
 			goto out_wl;
 		}
diff --git a/drivers/mtd/ubi/build.c b/drivers/mtd/ubi/build.c
index 50b7590..f769c22 100644
--- a/drivers/mtd/ubi/build.c
+++ b/drivers/mtd/ubi/build.c
@@ -1053,6 +1053,7 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
 	ubi_notify_all(ubi, UBI_VOLUME_REMOVED, NULL);
 	dbg_msg("detaching mtd%d from ubi%d", ubi->mtd->index, ubi_num);
 
+	/* TODO: any action on failure? */
 	ubi_update_fastmap(ubi);
 
 	/*
@@ -1070,7 +1071,6 @@ int ubi_detach_mtd_dev(int ubi_num, int anyway)
 
 	ubi_debugfs_exit_dev(ubi);
 	uif_close(ubi);
-
 	ubi_wl_close(ubi);
 	ubi_free_internal_volumes(ubi);
 	vfree(ubi->vtbl);
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 9f766ff..0b2d0cf 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -219,7 +219,7 @@ struct ubi_volume_desc;
  * @size: size of the fastmap in bytes
  * @used_blocks: number of used PEBs
  * @max_pool_size: maximal size of the user pool
- * @max_wl_pool_size: maximal size of the pooly used by the WL sub-system
+ * @max_wl_pool_size: maximal size of the pool used by the WL sub-system
  * @raw: the fastmap itself as byte array (only valid while attaching)
  */
 struct ubi_fastmap_layout {
@@ -242,7 +242,6 @@ struct ubi_fastmap_layout {
  * A pool gets filled with up to max_size.
  * If all PEBs within the pool are used a new fastmap will be written
  * to the flash and the pool gets refilled with empty PEBs.
- *
  */
 struct ubi_fm_pool {
 	int pebs[UBI_FM_MAX_POOL_SIZE];
diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 6c69017..688881b 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -272,6 +272,10 @@ static int produce_free_peb(struct ubi_device *ubi)
 		dbg_wl("do one work synchronously");
 		err = do_work(ubi);
 		if (err)
+			/* TODO: in the new locking scheme, produce_free_peb is
+			 * called under wl_lock taken.
+			 * so when returning, should reacquire the lock
+			 */
 			return err;
 
 		spin_lock(&ubi->wl_lock);
@@ -377,6 +381,7 @@ static struct ubi_wl_entry *find_wl_entry(struct ubi_device *ubi,
 	 * as anchor PEB, hold it back and return the second best WL entry
 	 * such that fastmap can use the anchor PEB later. */
 	if (!ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START)
+		/* TODO: do we have a risk returning NULL here? */
 		return prev_e;
 
 	return e;
@@ -405,6 +410,7 @@ static struct ubi_wl_entry *find_mean_wl_entry(struct ubi_device *ubi,
 		/* If no fastmap has been written and this WL entry can be used
 		 * as anchor PEB, hold it back and return the second best
 		 * WL entry such that fastmap can use the anchor PEB later. */
+		/* TODO: why this is specific just to < WL_FREE_MAX_DIFF case? */
 		if (e && !ubi->fm_disabled && !ubi->fm &&
 		    e->pnum < UBI_FM_MAX_START)
 			e = rb_entry(rb_next(root->rb_node),

UBI fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v16

Enjoy!
//richard

[PATCH 1/3] UBI: Fastmap: Document return codes.
[PATCH 2/3] UBI: Fastmap: Export fm_auto to sysfs
[PATCH 3/3] UBI: Fastmap: Simplify fastmap buffers

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 292 bytes --]

On Sat, 2012-06-30 at 12:53 +0200, Richard Weinberger wrote:
> So basically you want me to get rid of any vmalloc()'ed buffer?
> That's not easy.

Yeah, I guess it is difficult, we probably can let it be vmalloc() since
we already have plenty...

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 250 bytes --]

On Sat, 2012-06-30 at 12:53 +0200, Richard Weinberger wrote:
> So basically you want me to get rid of any vmalloc()'ed buffer?
> That's not easy.

How big is the buffer. What is its maximum possible size?

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 1343 bytes --]

Am 30.06.2012 12:43, schrieb Artem Bityutskiy:
> On Fri, 2012-06-29 at 17:14 +0200, Richard Weinberger wrote:
>> This is the next round of UBI fastmap updates.
>>
>> Fastmap is now disabled by default.
>> If you attach an image it will not automatically convert it
>> to the fastmap format.
>> UBI has a new module parameter "fm_auto".
>> If set to 1 UBI will create a fastmap automatically on your
>> flash device.
>> Please see commit "Add a module parameter to enable fastmap"
>> for more details.
> 
> One thing I've noticed is that you use vmalloc for some of the buffers
> you do I/O on (fm_raw). This is a problem for many ARM platforms because
> they cannot do DMA on such buffers. The drivers have to implement
> various workarouns for this: either fall back to memcopies or do an
> extra copy to a DMA-able buffers.
> 
> UBI and UBIFS already to I/O on vmalloc'ed buffers, an I am planning to
> fix this, which is not easy. Would be great to not add more of these.

There are only two buffers which use vmalloc() both are used to hold the raw fastmap and have
the same size.
If it helps I could preallocate a buffer in ubi-> and use it in both functions ubi_write_fastmap()
and ubi_scan_fastmap().

So basically you want me to get rid of any vmalloc()'ed buffer?
That's not easy.

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 959 bytes --]

On Fri, 2012-06-29 at 17:14 +0200, Richard Weinberger wrote:
> This is the next round of UBI fastmap updates.
> 
> Fastmap is now disabled by default.
> If you attach an image it will not automatically convert it
> to the fastmap format.
> UBI has a new module parameter "fm_auto".
> If set to 1 UBI will create a fastmap automatically on your
> flash device.
> Please see commit "Add a module parameter to enable fastmap"
> for more details.

One thing I've noticed is that you use vmalloc for some of the buffers
you do I/O on (fm_raw). This is a problem for many ARM platforms because
they cannot do DMA on such buffers. The drivers have to implement
various workarouns for this: either fall back to memcopies or do an
extra copy to a DMA-able buffers.

UBI and UBIFS already to I/O on vmalloc'ed buffers, an I am planning to
fix this, which is not easy. Would be great to not add more of these.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

UBI fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.

Fastmap is now disabled by default.
If you attach an image it will not automatically convert it
to the fastmap format.
UBI has a new module parameter "fm_auto".
If set to 1 UBI will create a fastmap automatically on your
flash device.
Please see commit "Add a module parameter to enable fastmap"
for more details.

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v15

Enjoy!
//richard

[PATCH 01/11] UBI: Fastmap: Fix mem leak in error path
[PATCH 02/11] UBI: Fastmap: Amend self_check_eba()
[PATCH 03/11] UBI: Fastmap: Remove forward declaration
[PATCH 04/11] UBI: Fastmap: Move fastmap specific code into
[PATCH 05/11] UBI: Fastmap: Kill TODO
[PATCH 06/11] UBI: Fastmap: Remove unused variable
[PATCH 07/11] UBI: Fastmap: Fix scan regression
[PATCH 08/11] ubi: fastmap: Remove 'ubi' parameter of 'destroy_ai()'
[PATCH 09/11] ubi: fastmap: Do not free 'ai' in 'scan_all()'
[PATCH 10/11] UBI: Fastmap: Disable fastmap per default
[PATCH 11/11] UBI: Fastmap: Add a module parameter to enable fastmap

Re: UBI fastmap updates

[Richard Weinberger]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

Am 27.06.2012 08:48, schrieb Nikita V. Youshchenko:
>> Would you infom me how much UBI attach time is improved with fastmap ?
> 
> I'm not Richard but still.
> 
> On board I'm currently working on, ubi attach time for 1G chip without 
> fastmap is more than 5 seconds. With fastmap, it is less than 250 
> milliseconds.

I can confirm these numbers.
On all of my boards the attach time dropped from a few seconds to
less than 500 milliseconds.
(The worst case I've seen was ~800 milliseconds)

Thanks,
//richard


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: UBI fastmap updates

[Nikita V. Youshchenko]

> Hi. Richard.
>
> Would you infom me how much UBI attach time is improved with fastmap ?

I'm not Richard but still.

On board I'm currently working on, ubi attach time for 1G chip without 
fastmap is more than 5 seconds. With fastmap, it is less than 250 
milliseconds.

Nikita

Re: UBI fastmap updates

[Namjae Jeon]

Hi. Richard.

Would you infom me how much UBI attach time is improved with fastmap ?

Thanks.

2012/6/23, Richard Weinberger <richard@nod.at>:
> This is the next round of UBI fastmap updates.
>
> The highlights are:
> 	- Fastmap produces anchor PEBs if none are available
> 	  (creating a fastmap on an image created by ubinize works now)
> 	- Fastmap is now able to reuse all PEBs if no empty PEBs are available
> 	- Minor fixes
>
> If you want to test fastmap you can use my git repo:
> git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v13
>
> Enjoy!
> //richard
>
> [PATCH 1/7] UBI: Fastmap: Modify the WL sub-system to prodcue a free
> [PATCH 2/7] UBI: Fastmap: Fix WARN_ON()
> [PATCH 3/7] UBI: Fastmap: Ensure that not all anchor PEBs go into a
> [PATCH 4/7] UBI: Fastmap: Fix ubi_assert()
> [PATCH 5/7] UBI: Fastmap: Kill max_pnum logic
> [PATCH 6/7] UBI: Fastmap: Reuse all fastmap PEB if no free PEBs are
> [PATCH 7/7] UBI: Fastmap: Get rid of fm_pool_mutex
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

UBI fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.

The highlights are:
	- Fastmap produces anchor PEBs if none are available
	  (creating a fastmap on an image created by ubinize works now)
	- Fastmap is now able to reuse all PEBs if no empty PEBs are available
	- Minor fixes

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v13

Enjoy!
//richard

[PATCH 1/7] UBI: Fastmap: Modify the WL sub-system to prodcue a free
[PATCH 2/7] UBI: Fastmap: Fix WARN_ON()
[PATCH 3/7] UBI: Fastmap: Ensure that not all anchor PEBs go into a
[PATCH 4/7] UBI: Fastmap: Fix ubi_assert()
[PATCH 5/7] UBI: Fastmap: Kill max_pnum logic
[PATCH 6/7] UBI: Fastmap: Reuse all fastmap PEB if no free PEBs are
[PATCH 7/7] UBI: Fastmap: Get rid of fm_pool_mutex

UBI Fastmap updates

[Richard Weinberger]

This is the next round of UBI fastmap updates.
It contains mostly fixes.

The highlights are:
- ubi_wl_flush() is no longer needed before creating the fastmap
- It passes all tests (ubi-tests, my own tests) with bit flit emulation and UBI self checks enabled.
- If UBI self checks are enabled UBI is able to proof the correctness of an EBA created from by fastmap.
- Endianness has been successfully tested (using qemu-x86 and qemu-ppc)

If you want to test fastmap you can use my git repo:
git://git.kernel.org/pub/scm/linux/kernel/git/rw/ubi2.git ubi2/v11

Please don't get confused with the release number, v11 is the internal release.

Fastmap has been tested on PPC, ARM, x86 on both real hardware and emulation using qemu.
To catch race conditions lots of tests have been done on a very fast x86 multi core machine.
So far nothing exploded. :-)
I'll send my fastmap torture script plus a paranoia patch in a separate mail.

Enjoy!
//richard

[PATCH 01/22] UBI: Fastmap: Add EBA selfcheck
[PATCH 02/22] UBI: Fastmap: Fix NULL pointer bug
[PATCH 03/22] UBI: Fastmap: Keep fastmap after attaching
[PATCH 04/22] UBI: Fastmap: Store scrub list in fastmap
[PATCH 05/22] UBI: Fastmap: Rework ubi_wl_put_fm_peb()
[PATCH 06/22] UBI: Fastmap: Make EBA table self check depend on
[PATCH 07/22] UBI: Fastmap: Fix build (a left over from the ai->fm
[PATCH 08/22] Revert "UBI: Fastmap: Check for duplicated PEBs in
[PATCH 09/22] UBI: Fastmap: Fix PEB count assert
[PATCH 10/22] UBI: Fastmap: Remove more useless new lines
[PATCH 11/22] UBI: Fastmap: Get rid of ubi_wl_flush() in
[PATCH 12/22] UBI: Fastmap: Locking fixes
[PATCH 13/22] UBI: Fastmap: Fix EC values
[PATCH 14/22] UBI: Fastmap: Fix copy&paste error
[PATCH 15/22] UBI: Fastmap: Kill old fastmap in case of a failure
[PATCH 16/22] UBI: Fastmap: Fix loglevel
[PATCH 17/22] UBI: Fastmap: Add comments to new functions
[PATCH 18/22] UBI: Fastmap: Rename "early PEB" to "anchor PEB".
[PATCH 19/22] UBI: Fastmap: Init fm_sem
[PATCH 20/22] UBI: Fastmap: Use good_peb_count in assert
[PATCH 21/22] UBI: Fastmap: Fix and explain duplicated PEBs
[PATCH 22/22] UBI: Fastmap: Replace crc32_be() with crc32()