Patchwork [RFC,/] mtd/ubi: flush wl before clearing update marker

login
register
mail settings
Submitter Sebastian Siewior
Date Nov. 29, 2009, 6:46 p.m.
Message ID <20091129184602.GB30617@Chamillionaire.breakpoint.cc>
Download mbox | patch
Permalink /patch/39736/
State RFC
Headers show

Comments

Sebastian Siewior - Nov. 29, 2009, 6:46 p.m.
ubiupdatevol -t does the following:
- ubi_start_update()
  - set_update_marker()
  - for all LEBs ubi_eba_unmap_leb()
  - clear_update_marker()
  - ubi_wl_flush()

ubi_wl_flush() physically erases all PEB, once it returns all PEBs are
empty. clear_update_marker() has the update marker written after return.
If there is a power cut between the last two functions then the UBI
volume has no longer the "update" marker set and may have some valid
LEBs while some of them may be gone.
If that volume in question happens to be a UBIFS volume, then mount
will fail with

|UBIFS error (pid 1361): ubifs_read_node: bad node type (255 but expected 6)
|UBIFS error (pid 1361): ubifs_read_node: bad node at LEB 0:0
|Not a node, first 24 bytes:
|00000000: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff

if there is at least one valid LEB and the wear-leveling worker managed
to clear LEB 0.

The patch waits for the wl worker to finish prior clearing the "update"
marker on flash. The two new LEB which are scheduled for erasing after
clear_update_marker() should not matter because they are only visible to
UBI.

Signed-off-by: Sebastian Andrzej Siewior <sebastian@breakpoint.cc>
---
Artem, I got a faulty image which shows this error message on mount. I
The image had no super block any more, not even an older version in a
Artem Bityutskiy - Nov. 30, 2009, 12:50 p.m.
On Sun, 2009-11-29 at 19:46 +0100, Sebastian Andrzej Siewior wrote:
> ubiupdatevol -t does the following:
> - ubi_start_update()
>   - set_update_marker()
>   - for all LEBs ubi_eba_unmap_leb()
>   - clear_update_marker()
>   - ubi_wl_flush()
> 
> ubi_wl_flush() physically erases all PEB, once it returns all PEBs are
> empty. clear_update_marker() has the update marker written after return.
> If there is a power cut between the last two functions then the UBI
> volume has no longer the "update" marker set and may have some valid
> LEBs while some of them may be gone.
> If that volume in question happens to be a UBIFS volume, then mount
> will fail with

Very good catch, thanks. Pushed to the ubi-2.6.git tree, although I've

[snip]

>  	if (bytes == 0) {
> +
Removed this junk newline
> +		err = ubi_wl_flush(ubi);
> +		if (err)
> +			return err;

[snip]

>  	vol->upd_buf = vmalloc(ubi->leb_size);
> @@ -362,16 +365,17 @@ int ubi_more_update_data(struct ubi_device *ubi, struct ubi_volume *vol,
>  
>  	ubi_assert(vol->upd_received <= vol->upd_bytes);
>  	if (vol->upd_received == vol->upd_bytes) {
> +
And this.
> +		err = ubi_wl_flush(ubi);
> +		if (err)
> +			return err;

[snip]
Artem Bityutskiy - Nov. 30, 2009, 12:51 p.m.
On Sun, 2009-11-29 at 19:46 +0100, Sebastian Andrzej Siewior wrote:
> ubiupdatevol -t does the following:
> - ubi_start_update()
>   - set_update_marker()
>   - for all LEBs ubi_eba_unmap_leb()
>   - clear_update_marker()
>   - ubi_wl_flush()
> 
> ubi_wl_flush() physically erases all PEB, once it returns all PEBs are
> empty. clear_update_marker() has the update marker written after return.
> If there is a power cut between the last two functions then the UBI
> volume has no longer the "update" marker set and may have some valid
> LEBs while some of them may be gone.
> If that volume in question happens to be a UBIFS volume, then mount
> will fail with

This bug fix deserves being sent to Linus now, but it is now so late
that I'm really not sure I should do this, because if this causes any
regression, it will be too late to fix it for 2.6.32. So I think I'll
merge it to 2.6.33, is this OK with you?
Sebastian Siewior - Dec. 1, 2009, 11:30 p.m.
* Artem Bityutskiy | 2009-11-30 14:51:37 [+0200]:

>This bug fix deserves being sent to Linus now, but it is now so late
>that I'm really not sure I should do this, because if this causes any
>regression, it will be too late to fix it for 2.6.32. So I think I'll
>merge it to 2.6.33, is this OK with you?
Sure. This bug is there since the first release of UBI so it
shouldn't hurt anyone if the fix comes one release later.
It would be nice if you would push it -stable and pick it up in your
ubifs trees once you decided that it should not create any regressions
:)

Sebastian
Artem Bityutskiy - Dec. 2, 2009, 6:36 a.m.
On Wed, 2009-12-02 at 00:30 +0100, Sebastian Andrzej Siewior wrote:
> * Artem Bityutskiy | 2009-11-30 14:51:37 [+0200]:
> 
> >This bug fix deserves being sent to Linus now, but it is now so late
> >that I'm really not sure I should do this, because if this causes any
> >regression, it will be too late to fix it for 2.6.32. So I think I'll
> >merge it to 2.6.33, is this OK with you?
> Sure. This bug is there since the first release of UBI so it
> shouldn't hurt anyone if the fix comes one release later.
> It would be nice if you would push it -stable and pick it up in your
> ubifs trees once you decided that it should not create any regressions
> :)

I'll do this, thanks.

Patch

different PEB. So I came to the conclusion described above. Is this
possible or did miss a detail?
I've made the same fix in ubi_more_update_data() because there is the
same pattern however I'm not 100% sure if it is required / correct.

 drivers/mtd/ubi/upd.c |   22 +++++++++++++---------
 1 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/ubi/upd.c b/drivers/mtd/ubi/upd.c
index 74fdc40..312d0ec 100644
--- a/drivers/mtd/ubi/upd.c
+++ b/drivers/mtd/ubi/upd.c
@@ -147,12 +147,15 @@  int ubi_start_update(struct ubi_device *ubi, struct ubi_volume *vol,
 	}
 
 	if (bytes == 0) {
+
+		err = ubi_wl_flush(ubi);
+		if (err)
+			return err;
+
 		err = clear_update_marker(ubi, vol, 0);
 		if (err)
 			return err;
-		err = ubi_wl_flush(ubi);
-		if (!err)
-			vol->updating = 0;
+		vol->updating = 0;
 	}
 
 	vol->upd_buf = vmalloc(ubi->leb_size);
@@ -362,16 +365,17 @@  int ubi_more_update_data(struct ubi_device *ubi, struct ubi_volume *vol,
 
 	ubi_assert(vol->upd_received <= vol->upd_bytes);
 	if (vol->upd_received == vol->upd_bytes) {
+
+		err = ubi_wl_flush(ubi);
+		if (err)
+			return err;
 		/* The update is finished, clear the update marker */
 		err = clear_update_marker(ubi, vol, vol->upd_bytes);
 		if (err)
 			return err;
-		err = ubi_wl_flush(ubi);
-		if (err == 0) {
-			vol->updating = 0;
-			err = to_write;
-			vfree(vol->upd_buf);
-		}
+		vol->updating = 0;
+		err = to_write;
+		vfree(vol->upd_buf);
 	}
 
 	return err;