UBI fastmap updates

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

[PATCH 1/7] UBI: Fastmap: Fix lock imbalance in case of an error

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 6c69017..79e3257 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -271,10 +271,10 @@ static int produce_free_peb(struct ubi_device *ubi)
 
 		dbg_wl("do one work synchronously");
 		err = do_work(ubi);
-		if (err)
-			return err;
 
 		spin_lock(&ubi->wl_lock);
+		if (err)
+			return err;
 	}
 
 	return 0;
-- 
1.7.6.5

[PATCH 2/7] UBI: Fastmap: Get rid of find_fastmap switch

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 106aa01..02505ff 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -817,8 +817,7 @@ out_unlock:
  * successfully handled and a negative error code in case of failure.
  */
 static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
-		    int pnum, int *vid, unsigned long long *sqnum,
-		    int find_fastmap)
+		    int pnum, int *vid, unsigned long long *sqnum)
 {
 	long long uninitialized_var(ec);
 	int err, bitflips = 0, vol_id = -1, ec_err = 0;
@@ -997,15 +996,18 @@ static int scan_peb(struct ubi_device *ubi, struct ubi_attach_info *ai,
 	if (sqnum)
 		*sqnum = be64_to_cpu(vidh->sqnum);
 
-	if (!find_fastmap &&
-	   (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID)) {
+	if (vol_id > UBI_MAX_VOLUMES && vol_id != UBI_LAYOUT_VOLUME_ID) {
 		int lnum = be32_to_cpu(vidh->lnum);
 
 		/* Unsupported internal volume */
 		switch (vidh->compat) {
 		case UBI_COMPAT_DELETE:
-			ubi_msg("\"delete\" compatible internal volume %d:%d"
-				" found, will remove it", vol_id, lnum);
+			if (vol_id != UBI_FM_SB_VOLUME_ID
+			    && vol_id != UBI_FM_DATA_VOLUME_ID) {
+				ubi_msg("\"delete\" compatible internal volume"
+					" %d:%d found, will remove it",
+					vol_id, lnum);
+			}
 			err = add_to_list(ai, pnum, vol_id, lnum,
 					  ec, 1, &ai->erase);
 			if (err)
@@ -1247,7 +1249,7 @@ static int scan_all(struct ubi_device *ubi, struct ubi_attach_info *ai, int star
 		cond_resched();
 
 		dbg_gen("process PEB %d", pnum);
-		err = scan_peb(ubi, ai, pnum, NULL, NULL, 0);
+		err = scan_peb(ubi, ai, pnum, NULL, NULL);
 		if (err < 0)
 			goto out_vidh;
 	}
@@ -1332,7 +1334,7 @@ static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info *ai)
 		cond_resched();
 
 		dbg_gen("process PEB %d", pnum);
-		err = scan_peb(ubi, ai, pnum, &vol_id, &sqnum, 1);
+		err = scan_peb(ubi, ai, pnum, &vol_id, &sqnum);
 		if (err < 0)
 			goto out_vidh;
 
-- 
1.7.6.5

[PATCH 3/7] UBI: Fastmap: Fix memory leak in error path

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 02505ff..5d61ba7 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1448,7 +1448,7 @@ int ubi_attach(struct ubi_device *ubi, int force_scan)
 
 		err = scan_all(ubi, scan_ai, 0);
 		if (err) {
-			kfree(scan_ai);
+			destroy_ai(scan_ai);
 			goto out_wl;
 		}
 
-- 
1.7.6.5

[PATCH 4/7] UBI: Fastmap: Fix double free in error path

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 5d61ba7..85c9e0c 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -1322,7 +1322,7 @@ static int scan_fast(struct ubi_device *ubi, struct ubi_attach_info *ai)
 
 	ech = kzalloc(ubi->ec_hdr_alsize, GFP_KERNEL);
 	if (!ech)
-		goto out_ai;
+		goto out;
 
 	vidh = ubi_zalloc_vid_hdr(ubi, GFP_KERNEL);
 	if (!vidh)
@@ -1356,8 +1356,7 @@ out_vidh:
 	ubi_free_vid_hdr(ubi, vidh);
 out_ech:
 	kfree(ech);
-out_ai:
-	destroy_ai(ai);
+out:
 	return err;
 }
 static struct ubi_attach_info *alloc_ai(void)
-- 
1.7.6.5

[PATCH 5/7] UBI: Fastmap: Kerneldoc fixes

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/attach.c b/drivers/mtd/ubi/attach.c
index 85c9e0c..14d2580 100644
--- a/drivers/mtd/ubi/attach.c
+++ b/drivers/mtd/ubi/attach.c
@@ -810,6 +810,8 @@ out_unlock:
  * @ubi: UBI device description object
  * @ai: attaching information
  * @pnum: the physical eraseblock number
+ * @vid: The volume ID of the found volume will be stored in this pointer
+ * @sqnum: The sqnum of the found volume will be stored in this pointer
  *
  * This function reads UBI headers of PEB @pnum, checks them, and adds
  * information about this PEB to the corresponding list or RB-tree in the
@@ -1223,6 +1225,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
+ * @start: start scanning at this PEB
  *
  * 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
diff --git a/drivers/mtd/ubi/ubi.h b/drivers/mtd/ubi/ubi.h
index 56e6be0..a25d59a 100644
--- a/drivers/mtd/ubi/ubi.h
+++ b/drivers/mtd/ubi/ubi.h
@@ -218,7 +218,7 @@ struct ubi_volume_desc;
  * @to_be_tortured: if non-zero tortured this PEB
  * @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
  */
 struct ubi_fastmap_layout {
 	struct ubi_wl_entry *e[UBI_FM_MAX_BLOCKS];
-- 
1.7.6.5

[PATCH 6/7] UBI: Fastmap: Make sure that find_wl_entry() never returns NULL

[Richard Weinberger]

If the rbtree contains only one entry and we don't have fastmap
on flash find_wl_entry() might return NULL.
This must not happen.

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

diff --git a/drivers/mtd/ubi/wl.c b/drivers/mtd/ubi/wl.c
index 79e3257..d61672d 100644
--- a/drivers/mtd/ubi/wl.c
+++ b/drivers/mtd/ubi/wl.c
@@ -376,7 +376,8 @@ static struct ubi_wl_entry *find_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. */
-	if (!ubi->fm_disabled && !ubi->fm && e->pnum < UBI_FM_MAX_START)
+	if (prev_e && !ubi->fm_disabled &&
+	    !ubi->fm && e->pnum < UBI_FM_MAX_START)
 		return prev_e;
 
 	return e;
-- 
1.7.6.5

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

[Richard Weinberger]

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

diff --git a/drivers/mtd/ubi/fastmap.c b/drivers/mtd/ubi/fastmap.c
index 44b95ce..be73a1d 100644
--- a/drivers/mtd/ubi/fastmap.c
+++ b/drivers/mtd/ubi/fastmap.c
@@ -912,7 +912,8 @@ int ubi_scan_fastmap(struct ubi_device *ubi, struct ubi_attach_info *ai,
 
 	fm_size = ubi->leb_size * used_blocks;
 	if (fm_size != ubi->fm_size) {
-		ubi_err("bad fastmap size: %zi, expected: %zi", fm_size, ubi->fm_size);
+		ubi_err("bad fastmap size: %zi, expected: %zi", fm_size,
+			ubi->fm_size);
 		ret = UBI_BAD_FASTMAP;
 		kfree(fmsb);
 		kfree(fm);
-- 
1.7.6.5

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 --]

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: 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]

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: 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 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: 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: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

[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, 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

[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, 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: 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

[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: 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

[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]

[-- 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

[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

[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: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: 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 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

[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

[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: 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: 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

[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: 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: 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

[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: 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

[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: 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: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: 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 --]

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]

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 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

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

[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 --]

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: 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

[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

[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),

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

[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

[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: 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 --]

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

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

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/
>

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

[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 --]

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()