[PATCH] block2mtd oops in erase function.

[PATCH] block2mtd oops in erase function.

[Felix Fietkau]

Hi, I have a small fix for a crash that happened when I was using jffs2
in combination with block2mtd.c

In the erase function when checking the block to see if it's already
erased, the limit is to be set to the page_address(page) + PAGE_SIZE,
but because the variable has the type (ulong *), it gets set to
PAGE_SIZE*sizeof(ulong), which makes the kernel oops when the page is
very close to the end of RAM.

Signed-off-by: Felix Fietkau <nbd@openwrt.org>

--- linux.dev/drivers/mtd/devices/block2mtd.c.old	2007-02-18 14:08:59.519952312 +0100
+++ linux.dev/drivers/mtd/devices/block2mtd.c	2007-02-18 14:09:04.219237912 +0100
@@ -111,7 +111,7 @@
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		max = (u_long*)page_address(page) + PAGE_SIZE;
+		max = (u_long*) ((u8 *) page_address(page) + PAGE_SIZE);
 		for (p=(u_long*)page_address(page); p<max; p++)
 			if (*p != -1UL) {
 				lock_page(page);

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Mon, 19 February 2007 22:29:39 +0100, Felix Fietkau wrote:
> 
> Hi, I have a small fix for a crash that happened when I was using jffs2
> in combination with block2mtd.c
> 
> In the erase function when checking the block to see if it's already
> erased, the limit is to be set to the page_address(page) + PAGE_SIZE,
> but because the variable has the type (ulong *), it gets set to
> PAGE_SIZE*sizeof(ulong), which makes the kernel oops when the page is
> very close to the end of RAM.

Good catch!  What a twisted little piece of code you've found.

> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> 
> --- linux.dev/drivers/mtd/devices/block2mtd.c.old	2007-02-18 14:08:59.519952312 +0100
> +++ linux.dev/drivers/mtd/devices/block2mtd.c	2007-02-18 14:09:04.219237912 +0100
> @@ -111,7 +111,7 @@
>  		if (IS_ERR(page))
>  			return PTR_ERR(page);
>  
> -		max = (u_long*)page_address(page) + PAGE_SIZE;
> +		max = (u_long*) ((u8 *) page_address(page) + PAGE_SIZE);
>  		for (p=(u_long*)page_address(page); p<max; p++)
>  			if (*p != -1UL) {
>  				lock_page(page);

The proper fix would be to remove the cast instead of adding yet
another.  I wonder when those got added.

Jörn

-- 
He who knows others is wise.
He who knows himself is enlightened.
-- Lao Tsu

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Mon, 19 February 2007 22:20:41 +0000, Jörn Engel wrote:
> 
> The proper fix would be to remove the cast instead of adding yet
> another.  I wonder when those got added.

On December 21st 2004 by none other than me.  How embarrassing!
http://lists.infradead.org/pipermail/linux-mtd/2004-December/011236.html

Can you test this patch?

diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
index f9f2ce7..6a9fb80 100644
--- a/drivers/mtd/devices/block2mtd.c
+++ b/drivers/mtd/devices/block2mtd.c
@@ -111,8 +111,8 @@ static int _block2mtd_erase(struct block
 		if (IS_ERR(page))
 			return PTR_ERR(page);
 
-		max = (u_long*)page_address(page) + PAGE_SIZE;
-		for (p=(u_long*)page_address(page); p<max; p++)
+		max = page_address(page) + PAGE_SIZE;
+		for (p=page_address(page); p<max; p++)
 			if (*p != -1UL) {
 				lock_page(page);
 				memset(page_address(page), 0xff, PAGE_SIZE);

Jörn

-- 
"[One] doesn't need to know [...] how to cause a headache in order
to take an aspirin."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001

Re: [PATCH] block2mtd oops in erase function.

[Felix Fietkau]

On Mon, 2007-02-19 at 22:30 +0000, Jörn Engel wrote:
> On December 21st 2004 by none other than me.  How embarrassing!
> http://lists.infradead.org/pipermail/linux-mtd/2004-December/011236.html
> 
> Can you test this patch?
> 
> diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> [...]

Works :)

- Felix

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Tue, 20 February 2007 03:27:09 +0100, Felix Fietkau wrote:
> On Mon, 2007-02-19 at 22:30 +0000, Jörn Engel wrote:
> > On December 21st 2004 by none other than me.  How embarrassing!
> > http://lists.infradead.org/pipermail/linux-mtd/2004-December/011236.html
> > 
> > Can you test this patch?
> > 
> > diff --git a/drivers/mtd/devices/block2mtd.c b/drivers/mtd/devices/block2mtd.c
> > [...]
> 
> Works :)

Cool!  Would you mind also trying two more patches?  One of them just
removes a compile warning if CONFIG_MODULE=n, the other rips out the
readahead code, which should give a nice performance boost on USB
sticks, MMC/SD, etc.  Mounting JFFS2 in particular should be faster.

Jörn

-- 
It does not matter how slowly you go, so long as you do not stop.
-- Confucius

Re: [PATCH] block2mtd oops in erase function.

[Jason Lunz]

On Tue, Feb 20, 2007 at 10:53:07AM +0000, Jörn Engel wrote:
> Cool!  Would you mind also trying two more patches?  One of them just
> removes a compile warning if CONFIG_MODULE=n, the other rips out the
> readahead code, which should give a nice performance boost on USB
> sticks, MMC/SD, etc.  Mounting JFFS2 in particular should be faster.

I'm using jffs2 on block2mtd. I'd be happy to test any patches you have.

The readahead-disable one in particular is interesting - i ended up
patching the source to get rid of the warning about readahead past
end-of-disk. And mounting jffs2-on-block2mtd-on-ide-flash is quite slow.

Jason

---
 drivers/mtd/devices/block2mtd.c |    1 -
 1 file changed, 1 deletion(-)

Index: linux-2.6.20-rc5-uml/drivers/mtd/devices/block2mtd.c
===================================================================
--- linux-2.6.20-rc5-uml.orig/drivers/mtd/devices/block2mtd.c
+++ linux-2.6.20-rc5-uml/drivers/mtd/devices/block2mtd.c
@@ -63,7 +63,6 @@
 	for (i = 0; i < PAGE_READAHEAD; i++) {
 		pagei = index + i;
 		if (pagei > end_index) {
-			INFO("Overrun end of disk in cache readahead\n");
 			break;
 		}
 		page = radix_tree_lookup(&mapping->page_tree, pagei);

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Tue, 20 February 2007 16:35:29 -0500, Jason Lunz wrote:
> 
> I'm using jffs2 on block2mtd. I'd be happy to test any patches you have.

Is it ok if I just pass this URL?
http://kernel.org/git/?p=linux/kernel/git/joern/misc.git

The top three patches are test-worthy.

> The readahead-disable one in particular is interesting - i ended up
> patching the source to get rid of the warning about readahead past
> end-of-disk. And mounting jffs2-on-block2mtd-on-ide-flash is quite slow.

If it works for you and improves mount time, that would be good results.

Jörn

-- 
I've never met a human being who would want to read 17,000 pages of
documentation, and if there was, I'd kill him to get him out of the
gene pool.
-- Joseph Costello

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Tue, 20 February 2007 22:02:54 -0500, Jason Lunz wrote:
> On Tue, Feb 20, 2007 at 09:55:13PM +0000, Jörn Engel wrote:
> > Is it ok if I just pass this URL?
> > http://kernel.org/git/?p=linux/kernel/git/joern/misc.git
> > 
> > The top three patches are test-worthy.
> 
> I'm running with all three. everything seems fine.

Great!

> Mount time of a ~56M jffs2 partition went from ~72s to ~54s.

Ouch!  While this is a significant improvement, 54s is still quite bad.
The 512MiB partition on the OLPC is mounting in ~3.4s with JFFS2 and
~60ms with LogFS.  Imo 3.4s is already bad, and 54s is way beyond.

Now where is time lost?  One candidate is erase block summary, enabling
it usually gives roughly a 6x performance improvement.  That would get
you down to ~9s.

Another next thing I suspect is the erase size.  The default is 4KiB,
which is rather small.  Can you try setting it to 64KiB or maybe even
128KiB?

Instead of passing "block2mtd=/dev/foo", you can pass
"block2mtd=/dev/foo,64KiB" if my memory doesn't fail me.

Changing the erase size to a multiple of the previous erase size should
be fine with JFFS2 (a smaller erase size could lead to data loss), but
you likely won't get better performance unless you recreate the
filesystem.  Maybe something like

$ mount /dev/mtdX /mnt -t jffs2
$ (cd /mnt && tar cvpf /tmp/foo.tgz .)
$ umount /mnt
$ flash_eraseall /dev/mtdX
$ mount /dev/mtdX /mnt -t jffs2
$ (cd /mnt && tar vpf /tmp/foo.tgz)
$ umount /dev/mtdX

Unless you are happy with the current mount time, of course.  Noone is
forcing you to change anything.

Jörn

-- 
Do not stop an army on its way home.
-- Sun Tzu

Re: [PATCH] block2mtd oops in erase function.

[Jason Lunz]

On Wed, Feb 21, 2007 at 02:47:54PM +0000, Jörn Engel wrote:
> > Mount time of a ~56M jffs2 partition went from ~72s to ~54s.
> 
> Ouch!  While this is a significant improvement, 54s is still quite bad.
> The 512MiB partition on the OLPC is mounting in ~3.4s with JFFS2 and
> ~60ms with LogFS.  Imo 3.4s is already bad, and 54s is way beyond.
> 
> Now where is time lost?  One candidate is erase block summary, enabling
> it usually gives roughly a 6x performance improvement.  That would get
> you down to ~9s.

How do I do that?

> Another next thing I suspect is the erase size.  The default is 4KiB,
> which is rather small.  Can you try setting it to 64KiB or maybe even
> 128KiB?

Those timings are already using a 128k erase size.

A raw read of the underlying block device takes 54s, so jffs2 isn't
adding any significant overhead anymore. It was when readahead was
in use.

The only way to increase performance at this point is to make it so that
jffs2 doesn't need to read the entire device at mount time. Is this what
erase block summaries do?

-- 
Jason Lunz | Senior Developer | Reflex Security, Inc.
lunz@reflexsecurity.com

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Wed, 21 February 2007 10:42:49 -0500, Jason Lunz wrote:
> 
> A raw read of the underlying block device takes 54s, so jffs2 isn't
> adding any significant overhead anymore. It was when readahead was
> in use.

That is a bit surprising.  JFFS2 has some optimizations to skip empty
blocks after some bytes.  This is where readahead really hurt - JFFS2
would skip and readahead would read the skipped data anyway.  Now I can
see that JFFS2 has some overhead over plain reading the device, but it
is unusual that overhead and optimizations cancel each other out so
perfectly.

> The only way to increase performance at this point is to make it so that
> jffs2 doesn't need to read the entire device at mount time. Is this what
> erase block summaries do?

Yes.  The idea is to only read the summary at the end of an erase block.
If the summary does not exist, it will fall back to linear scanning of
the block, afaik.

If you enable CONFIG_JFFS2_SUMMARY, it should work for you.  Of course,
your filesystem needs the summary, so you either have to do the
backup&restore procedure or apply sumtool (is that its name?) to your
images.

Jörn

-- 
Joern's library part 3:
http://inst.eecs.berkeley.edu/~cs152/fa05/handouts/clark-test.pdf

Re: [PATCH] block2mtd oops in erase function.

[Jason Lunz]

On Wed, Feb 21, 2007 at 04:02:18PM +0000, Jörn Engel wrote:
> If you enable CONFIG_JFFS2_SUMMARY, it should work for you.  Of course,
> your filesystem needs the summary, so you either have to do the
> backup&restore procedure or apply sumtool (is that its name?) to your
> images.

I thought I'd enabled CONFIG_JFFS2_SUMMARY, but it turns out not.
I've enabled it now, but I still don't seem to see any speedup.

I'm unclear on how to make the filesystem use it. I create the jffs2
filesystem by simply doing:

	flash_eraseall -j /dev/mtd0
	mount -t jffs2 mtd0 /state

Will this create a summary automatically, or is there an extra step I
need to take? 

...ah, it seems removing the -j does the trick. If I don't use it, mount
goes from >50s to <4s.

thanks!

Jason

Re: [PATCH] block2mtd oops in erase function.

[Jörn Engel]

On Wed, 21 February 2007 15:26:51 -0500, Jason Lunz wrote:
> 
> ...ah, it seems removing the -j does the trick. If I don't use it, mount
> goes from >50s to <4s.

That's sounds better.  Thank you for testing the patches.

Jörn

-- 
Time? What's that? Time is only worth what you do with it.
-- Theo de Raadt