Patchwork [v7,1/5] : mtdoops: avoid erasing already empty areas

login
register
mail settings
Submitter Simon Kagstrom
Date Oct. 15, 2009, 7:47 a.m.
Message ID <20091015094721.6c85cc48@marrow.netinsight.se>
Download mbox | patch
Permalink /patch/36080/
State New
Headers show

Comments

Simon Kagstrom - Oct. 15, 2009, 7:47 a.m.
After having scanned the entire mtdoops area, mtdoops will erase it if
there are no mtdoops headers in it. However, empty and already erased
areas (i.e., without mtdoops headers) will therefore also be erased at
each startup.

This patch counts the number of unclean pages (neither empty nor with
the mtdoops header) and only erases if no headers are found and the area
is still unclean.

Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>
---
 drivers/mtd/mtdoops.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)
Artem Bityutskiy - Oct. 23, 2009, 3:57 a.m.
On Thu, 2009-10-15 at 09:47 +0200, Simon Kagstrom wrote:
> After having scanned the entire mtdoops area, mtdoops will erase it if
> there are no mtdoops headers in it. However, empty and already erased
> areas (i.e., without mtdoops headers) will therefore also be erased at
> each startup.
> 
> This patch counts the number of unclean pages (neither empty nor with
> the mtdoops header) and only erases if no headers are found and the area
> is still unclean.
> 
> Signed-off-by: Simon Kagstrom <simon.kagstrom@netinsight.net>

Actually, it is safer to erase the partition. This is exactly why JFFS2
has erase markers. On NOR flash, if you interrupt an erase operation
(e.g., by re-booting), you may end up with unstable bits, which are read
as '1' sometimes, but sometimes they may be read as '1'. Or you may end
up with 0xFF's at the beginning, and garbage at the end.

When you erase the partition before starting using it, you make sure you
start with eraseblocks in normal state.

In practice, we observed stuff like that only on NOR, so if this is a
performance issue for you, you can make the erasure to be mandatory only
on NOR, but this won't be very nice.

BTW, we could also add some limit to the partition size of MTD oops. If
users use mtdoops with 256MiB partition, e.g., by mistake, it does not
make sense and we may just refuse it, instead of erasing.

-----
Side note, not a request to implement, but if you add a comment to the
code about this, it may be nice
-----

BTW, this suggests that before erasing eraseblocks in MTD oops, you need
to "invalidate" them by writing, say zeroes, to the second byte of the
eraseblock, if this is NOR. This is what we added to UBI recently,
because otherwise unclean reboots made it fail horribly on NOR, If you
are interested, you may take a look at the 'nor_erase_prepare()'
function in drivers/mtd/ubi/io.c

But again, this is NOR-specific.

Patch

diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
index 18c6c96..c785e1a 100644
--- a/drivers/mtd/mtdoops.c
+++ b/drivers/mtd/mtdoops.c
@@ -225,7 +225,7 @@  static void mtdoops_workfunc_write(struct work_struct *work)
 static void find_next_position(struct mtdoops_context *cxt)
 {
 	struct mtd_info *mtd = cxt->mtd;
-	int ret, page, maxpos = 0;
+	int ret, page, maxpos = 0, unclean_pages = 0;
 	u32 count[2], maxcount = 0xffffffff;
 	size_t retlen;
 
@@ -237,10 +237,13 @@  static void find_next_position(struct mtdoops_context *cxt)
 			continue;
 		}
 
-		if (count[1] != MTDOOPS_KERNMSG_MAGIC)
-			continue;
 		if (count[0] == 0xffffffff)
 			continue;
+		if (count[1] != MTDOOPS_KERNMSG_MAGIC) {
+			/* Page is neither clean nor empty */
+			unclean_pages++;
+			continue;
+		}
 		if (maxcount == 0xffffffff) {
 			maxcount = count[0];
 			maxpos = page;
@@ -259,7 +262,14 @@  static void find_next_position(struct mtdoops_context *cxt)
 	if (maxcount == 0xffffffff) {
 		cxt->nextpage = 0;
 		cxt->nextcount = 1;
-		schedule_work(&cxt->work_erase);
+		if (unclean_pages != 0) {
+			printk(KERN_INFO "mtdoops: cleaning area\n");
+			schedule_work(&cxt->work_erase);
+		} else {
+			printk(KERN_DEBUG "mtdoops: ready %d, %d (clean)\n",
+			       cxt->nextpage, cxt->nextcount);
+			cxt->ready = 1;
+		}
 		return;
 	}