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