[PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[赵 豆豆]

[-- Attachment #1: Type: text/plain, Size: 868 bytes --]

Hi, 

Current jffs2_check_oob_empty() assumes that only OOB of first
page has data content -- the clean marker. OOB of other pages should
be all 0xFF.
But if we decide to store other kind of meta-data in OOB of the
second,third or forth page, this will cause compatibility issue.
For example, if we store some data in OOB of second page in the
future, the "old" JFFS2 will recognize all the erase blocks in
the "new" JFFS2 image as ALL_DIRTY.
So this patch solve this compatibility problem by extending existent
node type compat_bits to OOB area. In particular, I think
JFFS2_FEATURE_RWCOMPAT_COPY has no meaning for OOB, so it's
handled the same way as JFFS2_FEATURE_RWCOMPAT_DELETE.

Your comments are welcome!

Thanks,
Forrest

_________________________________________________________________
享用世界上最大的电子邮件系统― MSN Hotmail。  http://www.hotmail.com  

[-- Attachment #2: oob_compat.patch --]
[-- Type: application/octet-stream, Size: 2509 bytes --]

--- wbuf.c.bak	2005-07-21 20:50:42.000000000 +0800
+++ wbuf.c	2005-07-21 22:03:53.589883720 +0800
@@ -933,9 +933,10 @@
 {
 	unsigned char *buf;
 	int 	ret = 0;
-	int	i,len,page;
+	int	i,len,page,current_oob;
 	size_t  retlen;
 	int	oob_size;
+	struct jffs2_unknown_node *node;
 
 	/* allocate a buffer for all oob data in this sector */
 	oob_size = c->mtd->oobsize;
@@ -947,7 +948,7 @@
 	}
 	/* 
 	 * if mode = 0, we scan for a total empty oob area, else we have
-	 * to take care of the cleanmarker in the first page of the block
+	 * to take care of the data content in the subsequent pages of the block
 	*/
 	ret = jffs2_flash_read_oob(c, jeb->offset, len , &retlen, buf);
 	if (ret) {
@@ -961,28 +962,51 @@
 		ret = -EIO;
 		goto out;
 	}
-	
-	/* Special check for first page */
-	for(i = 0; i < oob_size ; i++) {
-		/* Yeah, we know about the cleanmarker. */
-		if (mode && i >= c->fsdata_pos && 
-		    i < c->fsdata_pos + c->fsdata_len)
-			continue;
-
-		if (buf[i] != 0xFF) {
-			D2(printk(KERN_DEBUG "Found %02x at %x in OOB for %08x\n",
-				  buf[i], i, jeb->offset));
-			ret = 1; 
-			goto out;
+
+
+	if (!mode) {
+		for (page = 0; page < len; page += sizeof(long)) {
+			unsigned long dat = *(unsigned long *)(&buf[page]);
+			if (dat != -1) {
+				ret = 1;
+				goto out;
+			}
 		}
-	}
+	} else {
+		for (current_oob = 0; current_oob < len; current_oob += oob_size) {
+			node = (struct jffs2_unknown_node *)&buf[current_oob + c->fsdata_pos];
+			if (je16_to_cpu(node->magic) == JFFS2_MAGIC_BITMASK) {
+				switch (je16_to_cpu(node->nodetype) & JFFS2_COMPAT_MASK) {
+				case JFFS2_FEATURE_ROCOMPAT:
+					c->flags |= JFFS2_SB_FLAG_RO;
+					if (!(jffs2_is_readonly(c)))
+						return -EROFS;
+
+				case JFFS2_FEATURE_RWCOMPAT_DELETE:
+				case JFFS2_FEATURE_RWCOMPAT_COPY:
+					for (i = current_oob; i < current_oob + oob_size; i++) {
+						if (i >= current_oob + c->fsdata_pos &&
+						    i < current_oob + c->fsdata_pos + c->fsdata_len)
+							continue;
+
+						if (buf[i] != 0xFF) {
+							ret = 1;
+							goto out;
+						}
+					}
+					break;
 
-	/* we know, we are aligned :) */	
-	for (page = oob_size; page < len; page += sizeof(long)) {
-		unsigned long dat = *(unsigned long *)(&buf[page]);
-		if(dat != -1) {
-			ret = 1; 
-			goto out;
+				case JFFS2_FEATURE_INCOMPAT:
+					return -EINVAL;
+				}
+			} else {
+				for (i = current_oob; i < current_oob + oob_size; i++) {
+					if (buf[i] != 0xFF) {
+						ret = 1;
+						goto out;
+					}
+				}
+			}
 		}
 	}
 

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

赵 豆豆 wrote:
> Your comments are welcome!
Could you please use -auNrp diff options?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Thu, 21 July 2005 15:22:24 +0800, ?? ???? wrote:
> 
> Current jffs2_check_oob_empty() assumes that only OOB of first
> page has data content -- the clean marker. OOB of other pages should
> be all 0xFF.
> But if we decide to store other kind of meta-data in OOB of the
> second,third or forth page, this will cause compatibility issue.
> For example, if we store some data in OOB of second page in the
> future, the "old" JFFS2 will recognize all the erase blocks in
> the "new" JFFS2 image as ALL_DIRTY.
> So this patch solve this compatibility problem by extending existent
> node type compat_bits to OOB area. In particular, I think
> JFFS2_FEATURE_RWCOMPAT_COPY has no meaning for OOB, so it's
> handled the same way as JFFS2_FEATURE_RWCOMPAT_DELETE.
> 
> Your comments are welcome!

I'm pretty clueless wrt. oob data, so I leave the hard stuff for
others to comment on.

> --- wbuf.c.bak	2005-07-21 20:50:42.000000000 +0800
> +++ wbuf.c	2005-07-21 22:03:53.589883720 +0800
> @@ -933,9 +933,10 @@
>  {
>  	unsigned char *buf;
>  	int 	ret = 0;
> -	int	i,len,page;
> +	int	i,len,page,current_oob;
>  	size_t  retlen;
>  	int	oob_size;
> +	struct jffs2_unknown_node *node;
>  
>  	/* allocate a buffer for all oob data in this sector */
>  	oob_size = c->mtd->oobsize;
> @@ -947,7 +948,7 @@
>  	}
>  	/* 
>  	 * if mode = 0, we scan for a total empty oob area, else we have
> -	 * to take care of the cleanmarker in the first page of the block
> +	 * to take care of the data content in the subsequent pages of the block
>  	*/
>  	ret = jffs2_flash_read_oob(c, jeb->offset, len , &retlen, buf);
>  	if (ret) {
> @@ -961,28 +962,51 @@
>  		ret = -EIO;
>  		goto out;
>  	}
> -	
> -	/* Special check for first page */
> -	for(i = 0; i < oob_size ; i++) {
> -		/* Yeah, we know about the cleanmarker. */
> -		if (mode && i >= c->fsdata_pos && 
> -		    i < c->fsdata_pos + c->fsdata_len)
> -			continue;
> -
> -		if (buf[i] != 0xFF) {
> -			D2(printk(KERN_DEBUG "Found %02x at %x in OOB for %08x\n",
> -				  buf[i], i, jeb->offset));
> -			ret = 1; 
> -			goto out;
> +
> +
> +	if (!mode) {
> +		for (page = 0; page < len; page += sizeof(long)) {
> +			unsigned long dat = *(unsigned long *)(&buf[page]);
> +			if (dat != -1) {
> +				ret = 1;
> +				goto out;
> +			}
>  		}
> -	}
> +	} else {
> +		for (current_oob = 0; current_oob < len; current_oob += oob_size) {
> +			node = (struct jffs2_unknown_node *)&buf[current_oob + c->fsdata_pos];
> +			if (je16_to_cpu(node->magic) == JFFS2_MAGIC_BITMASK) {
> +				switch (je16_to_cpu(node->nodetype) & JFFS2_COMPAT_MASK) {
> +				case JFFS2_FEATURE_ROCOMPAT:
> +					c->flags |= JFFS2_SB_FLAG_RO;
> +					if (!(jffs2_is_readonly(c)))
> +						return -EROFS;
> +
> +				case JFFS2_FEATURE_RWCOMPAT_DELETE:
> +				case JFFS2_FEATURE_RWCOMPAT_COPY:
> +					for (i = current_oob; i < current_oob + oob_size; i++) {
> +						if (i >= current_oob + c->fsdata_pos &&
> +						    i < current_oob + c->fsdata_pos + c->fsdata_len)
> +							continue;
> +
> +						if (buf[i] != 0xFF) {
> +							ret = 1;
> +							goto out;

Sevenfold indentation is not what I call good coding style.  Try to
make this stuff simpler.  Use "if (!...) continue" and similar
constructs.

> +						}
> +					}
> +					break;
>  
> -	/* we know, we are aligned :) */	
> -	for (page = oob_size; page < len; page += sizeof(long)) {
> -		unsigned long dat = *(unsigned long *)(&buf[page]);
> -		if(dat != -1) {
> -			ret = 1; 
> -			goto out;
> +				case JFFS2_FEATURE_INCOMPAT:
> +					return -EINVAL;
> +				}
> +			} else {
> +				for (i = current_oob; i < current_oob + oob_size; i++) {
> +					if (buf[i] != 0xFF) {
> +						ret = 1;
> +						goto out;

Same here.

> +					}
> +				}
> +			}
>  		}
>  	}
 

Jörn

-- 
A surrounded army must be given a way out.
-- Sun Tzu

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> 
> Sevenfold indentation is not what I call good coding style.  Try to
> make this stuff simpler.  Use "if (!...) continue" and similar
> constructs.
> 
Agree, although it is how JFFS2 is written. Let's not worsen it, but 
instead, make it tidier. Please, if you don't mind, reformat the whole 
function, you may split it on shorter sub-functions, etc.

BTW, IMO the Summary patch of Ferenc Havasi is not yet in CVS (even 
though it greatly helps with mount time) because he didn't bothered 
making it cute. He just took messy JFFS2 code, and made it even messier. 
I'd be glad to see Ferenc's patch nice and in CVS.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Fri, 22 July 2005 16:12:56 +0400, Artem B. Bityuckiy wrote:
> Jörn Engel wrote:
> >
> >Sevenfold indentation is not what I call good coding style.  Try to
> >make this stuff simpler.  Use "if (!...) continue" and similar
> >constructs.
> >
> Agree, although it is how JFFS2 is written. Let's not worsen it, but 
> instead, make it tidier. Please, if you don't mind, reformat the whole 
> function, you may split it on shorter sub-functions, etc.

Yep.  You should send two (or more) seperate patches then.  First for
cleanup of existing function, second for actual changes.

> BTW, IMO the Summary patch of Ferenc Havasi is not yet in CVS (even 
> though it greatly helps with mount time) because he didn't bothered 
> making it cute. He just took messy JFFS2 code, and made it even messier. 
> I'd be glad to see Ferenc's patch nice and in CVS.

Same here.

Jörn

-- 
If System.PrivateProfileString("",
"HKEY_CURRENT_USER\Software\Microsoft\Office\9.0\Word\Security", "Level") <>
"" Then  CommandBars("Macro").Controls("Security...").Enabled = False
-- from the Melissa-source

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Artem B. Bityuckiy wrote:
> BTW, IMO the Summary patch of Ferenc Havasi is not yet in CVS (even 
> though it greatly helps with mount time) because he didn't bothered 
> making it cute. He just took messy JFFS2 code, and made it even messier. 
> I'd be glad to see Ferenc's patch nice and in CVS.

We also would be happy if it summary patch would be the part of the 
official JFFS2 (at least the erase block summary).

If I remember well David (dwmw2) said that it should be the part of 
JFFS3 only, because he would like to see only bugfixes in JFFS2. He did 
not mentioned that the reason is that it is messy. If it is true we will 
clean it (or partially rewrite if necessery) just please say some words 
about exactly why and where it is messy.

Thanks,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 26 July 2005 09:36:54 +0200, Ferenc Havasi wrote:
> Artem B. Bityuckiy wrote:
> >BTW, IMO the Summary patch of Ferenc Havasi is not yet in CVS (even 
> >though it greatly helps with mount time) because he didn't bothered 
> >making it cute. He just took messy JFFS2 code, and made it even messier. 
> >I'd be glad to see Ferenc's patch nice and in CVS.
> 
> We also would be happy if it summary patch would be the part of the 
> official JFFS2 (at least the erase block summary).
> 
> If I remember well David (dwmw2) said that it should be the part of 
> JFFS3 only, because he would like to see only bugfixes in JFFS2. He did 
> not mentioned that the reason is that it is messy.

All of which appeared to make sense back then.  By now, I guess,
most people stopped caring about jffs3.  The successor of jffs2 should
no longer be log-structured.  It's just that noone cared enough to
remove jffs3 from cvs yet.

> If it is true we will 
> clean it (or partially rewrite if necessery) just please say some words 
> about exactly why and where it is messy.

Can you post a patch or a URL to it?

Jörn

-- 
Fools ignore complexity.  Pragmatists suffer it.
Some can avoid it.  Geniuses remove it.
-- Perlis's Programming Proverb #58, SIGPLAN Notices, Sept.  1982

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Jörn Engel wrote:
>>If it is true we will 
>>clean it (or partially rewrite if necessery) just please say some words 
>>about exactly why and where it is messy.
> 
> Can you post a patch or a URL to it?

You can download from here:

http://www.inf.u-szeged.hu/jffs2/mount.php

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Steven Scholz]

Ferenc,

>>> If it is true we will clean it (or partially rewrite if necessery) 
>>> just please say some words about exactly why and where it is messy.
>>
>>
>> Can you post a patch or a URL to it?
> 
> 
> You can download from here:
> 
> http://www.inf.u-szeged.hu/jffs2/mount.php

Is there any chance to extend the code so that EBS will use the first mount to 
generate a summary instead having the need of a user space tool to create an 
JFFS2 image with EBS?


--
Steven

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 26 July 2005 10:29:40 +0200, Steven Scholz wrote:
> To: linux-mtd@lists.infradead.org

Please keep everyone on the Cc: list.

> Is there any chance to extend the code so that EBS will use the first mount 
> to generate a summary instead having the need of a user space tool to 
> create an JFFS2 image with EBS?

That's a lot of code for something inherently inefficient.  EBS should
create summaries for *new* blocks that are written, sure.  But if you
try to GC every single block without summary, your mount time will be
well beyond it would have been without EBS.

Jörn

-- 
The story so far:
In the beginning the Universe was created.  This has made a lot
of people very angry and been widely regarded as a bad move.
-- Douglas Adams

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Jörn Engel wrote:
>>Is there any chance to extend the code so that EBS will use the first mount 
>>to generate a summary instead having the need of a user space tool to 
>>create an JFFS2 image with EBS?
> 
> That's a lot of code for something inherently inefficient.  EBS should
> create summaries for *new* blocks that are written, sure.  But if you
> try to GC every single block without summary, your mount time will be
> well beyond it would have been without EBS.

I agree. It would be technically difficult to implement and would make 
the first mount time very-very long.

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> 
> I agree. It would be technically difficult to implement and would make 
> the first mount time very-very long.
> 

Hi Ferenc,

I have no time to look at your patch right now, but I recall there was 
many things like this:

#ifdef SUMMARY
Large block of code. Code is not tidier then the original JFFS2's code. 
If you are in scan.c, you wrote the code using scan.c's style...
#endif

In scan.c for example.

Also I saw large blocks of copy-pasted messy JFFS2 code which is 
slightly changed, and is embraced by #ifdefs. That's not neat.

I'll review your patch later and will be more specific.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Steven Scholz]

Ferenc Havasi wrote:

> Jörn Engel wrote:
> 
>>> Is there any chance to extend the code so that EBS will use the first 
>>> mount to generate a summary instead having the need of a user space 
>>> tool to create an JFFS2 image with EBS?
>>
>>
>> That's a lot of code for something inherently inefficient.  EBS should
>> create summaries for *new* blocks that are written, sure.  But if you
>> try to GC every single block without summary, your mount time will be
>> well beyond it would have been without EBS.
> 
> 
> I agree. It would be technically difficult to implement and would make 
> the first mount time very-very long.

Ah. I see. I did not know it would cretae that much hassle.

But then: what is the status of CS? For NOR?

--
Steven

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 26 July 2005 12:51:09 +0200, Steven Scholz wrote:
> 
> But then: what is the status of CS? For NOR?

What is CS?

Jörn

-- 
...one more straw can't possibly matter...
-- Kirby Bakken

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Steven Scholz]

Jörn Engel wrote:

> On Tue, 26 July 2005 12:51:09 +0200, Steven Scholz wrote:
> 
>>But then: what is the status of CS? For NOR?
> 
> 
> What is CS?

Centralized Summary. The other technique mentioned on Ferenc website.

--
Steven

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Steven!

> But then: what is the status of CS? For NOR?

Now it seems to be stable enough (at least the version which uses the
first erase block seems to store reference pointer) thanks to an active
user (realmsys). It is even more faster than EBS, but now it is still
working with NAND. And I think there are also things that should make
nicer in our code similar to EBS.

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 26 July 2005 09:57:25 +0200, Ferenc Havasi wrote:
> 
> You can download from here:
> 
> http://www.inf.u-szeged.hu/jffs2/mount.php

I don't care about pretty web pages, sorry.  Can you send me the patch
or a URL _directly_ to the patch?

Sorry, if this is rude, but efficiency has priority over politeness.

Jörn

-- 
/* Keep these two variables together */
int bar;

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 26 July 2005 11:32:15 +0200, Jörn Engel wrote:
> 
> I don't care about pretty web pages, sorry.  Can you send me the patch
> or a URL _directly_ to the patch?

Found it.  Still, for the future...

Review is focused on the simple stuff.  Most of the comments can be
repeated over and over, so just go through it yourself.  You should
focus on:
o Documentation/CodingStyle
o empty whitespace at the end of lines
o 80 columns (see CodingStyle)

When most of that stuff is done, resend the patch for the next round
of review.

PS: Sorry for being impolite.  This is one of those days...

> diff --unified --new-file --recursive Mtd-orig/fs/Kconfig mtd/fs/Kconfig
> --- Mtd-orig/fs/Kconfig	2005-05-09 10:16:08.000000000 +0200
> +++ mtd/fs/Kconfig	2005-07-18 15:42:05.000000000 +0200
> @@ -64,6 +64,19 @@
>  	    - NOR flash with transparent ECC
>  	    - DataFlash
>  
> +config JFFS2_SUMMARY
> +        bool "JFFS2 summary support (EXPERIMENTAL)" 
> +        depends on JFFS2_FS

Should depend on EXPERIMENTAL as well, judging by your own
description.

> +        default n
> +        help 
> +          This feature makes it possible to use summary information
> +          for faster filesystem mount - specially on NAND.
> +
> +          The summary information can be inserted into a filesystem image
> +          by the utility 'sumtool'.
> +
> +          If unsure, say 'N'.
> +
>  config JFFS2_COMPRESSION_OPTIONS
>  	bool "Advanced compression options for JFFS2"
>  	default n
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/Makefile.common mtd/fs/jffs2/Makefile.common
> --- Mtd-orig/fs/jffs2/Makefile.common	2005-07-17 08:56:20.000000000 +0200
> +++ mtd/fs/jffs2/Makefile.common	2005-07-18 15:42:05.000000000 +0200
> @@ -15,3 +15,4 @@
>  jffs2-$(CONFIG_JFFS2_RUBIN)	+= compr_rubin.o
>  jffs2-$(CONFIG_JFFS2_RTIME)	+= compr_rtime.o
>  jffs2-$(CONFIG_JFFS2_ZLIB)	+= compr_zlib.o
> +jffs2-$(CONFIG_JFFS2_SUMMARY)   += summary.o
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c

diff -Naurp is a bit nicer.  You can argue about -a, but -p definitely
helps when reading.

> --- Mtd-orig/fs/jffs2/build.c	2005-07-17 14:01:42.000000000 +0200
> +++ mtd/fs/jffs2/build.c	2005-07-18 15:42:05.000000000 +0200
> @@ -334,6 +334,10 @@
>  		c->blocks[i].first_node = NULL;
>  		c->blocks[i].last_node = NULL;
>  		c->blocks[i].bad_count = 0;
> + #ifdef CONFIG_JFFS2_SUMMARY	
> + 		c->blocks[i].sum_collected = NULL;
> + #endif
> +

Long-term the #ifdefs should go.

>  	}
>  
>  	INIT_LIST_HEAD(&c->clean_list);
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/dir.c mtd/fs/jffs2/dir.c
> --- Mtd-orig/fs/jffs2/dir.c	2005-07-17 13:13:46.000000000 +0200
> +++ mtd/fs/jffs2/dir.c	2005-07-18 15:42:05.000000000 +0200
> @@ -304,7 +304,7 @@
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);

JFFS2 generally suffers from this problem, but a line break after 80
columns (or less) would be nice.

>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -364,7 +364,7 @@
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> @@ -449,7 +449,7 @@
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -492,7 +492,7 @@
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> @@ -601,7 +601,7 @@
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -646,7 +646,7 @@
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> @@ -800,4 +800,3 @@
>  
>  	return 0;
>  }
> -
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/erase.c mtd/fs/jffs2/erase.c
> --- Mtd-orig/fs/jffs2/erase.c	2005-07-17 08:56:20.000000000 +0200
> +++ mtd/fs/jffs2/erase.c	2005-07-18 15:42:05.000000000 +0200
> @@ -423,7 +423,17 @@
>  		jeb->dirty_size = 0;
>  		jeb->wasted_size = 0;
>  	}
> -
> +	

Whitespace pollution.

> +#ifdef CONFIG_JFFS2_SUMMARY
> +	
> +	if (jeb->sum_collected) {
> +	

Pointless empty line.

> +		jffs2_sum_clean_collected(jeb);
> +		jeb->sum_collected->sum_size = 0;
> +		jeb->sum_collected->sum_padded = 0;
> +	}
> +#endif
> +	
>  	spin_lock(&c->erase_completion_lock);
>  	c->erasing_size -= c->sector_size;
>  	c->free_size += jeb->free_size;
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/file.c mtd/fs/jffs2/file.c
> --- Mtd-orig/fs/jffs2/file.c	2005-07-06 14:13:09.000000000 +0200
> +++ mtd/fs/jffs2/file.c	2005-07-18 15:42:05.000000000 +0200
> @@ -21,6 +21,7 @@
>  #include <linux/jffs2.h>
>  #include "nodelist.h"
>  
> +
>  extern int generic_file_open(struct inode *, struct file *) __attribute__((weak));
>  extern loff_t generic_file_llseek(struct file *file, loff_t offset, int origin) __attribute__((weak));
>  

Remove the newline.  If you believe the extra newline makes sense,
create a seperate cleanup patch for it.

> @@ -137,7 +138,7 @@
>  		D1(printk(KERN_DEBUG "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
>  			  (unsigned int)inode->i_size, pageofs));
>  
> -		ret = jffs2_reserve_space(c, sizeof(ri), &phys_ofs, &alloc_len, ALLOC_NORMAL);
> +		ret = jffs2_reserve_space(c, sizeof(ri), &phys_ofs, &alloc_len, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  		if (ret)
>  			return ret;
>  
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/fs.c mtd/fs/jffs2/fs.c
> --- Mtd-orig/fs/jffs2/fs.c	2005-07-17 14:01:42.000000000 +0200
> +++ mtd/fs/jffs2/fs.c	2005-07-18 15:42:05.000000000 +0200
> @@ -74,7 +74,7 @@
>  		return -ENOMEM;
>  	}
>  		
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + mdatalen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + mdatalen, &phys_ofs, &alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
>  		if (S_ISLNK(inode->i_mode & S_IFMT))
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/gc.c mtd/fs/jffs2/gc.c
> --- Mtd-orig/fs/jffs2/gc.c	2005-07-17 14:01:43.000000000 +0200
> +++ mtd/fs/jffs2/gc.c	2005-07-18 15:42:05.000000000 +0200
> @@ -505,7 +505,8 @@
>  	uint32_t phys_ofs, alloclen;
>  	uint32_t crc, rawlen;
>  	int retried = 0;
> -
> +	struct kvec vecs[1];
> +	

Whitespace pollution.

>  	D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
>  
>  	rawlen = ref_totlen(c, c->gcblock, raw);
> @@ -514,7 +515,7 @@
>  	   don't want to force wastage of the end of a block if splitting would
>  	   work. */
>  	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
> -					      rawlen), &phys_ofs, &alloclen);
> +					      rawlen), &phys_ofs, &alloclen, rawlen); /* this is not optimal yet */
>  	if (ret)
>  		return ret;
>  
> @@ -593,9 +594,12 @@
>  	nraw->flash_offset = phys_ofs;
>  	nraw->__totlen = rawlen;
>  	nraw->next_phys = NULL;
> +	

Whitespace pollution.

> +	vecs[0].iov_base = (unsigned char *) node;
> +	vecs[0].iov_len = rawlen;
>  
> -	ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
> -
> +	ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);
> +	

Whitespace pollution.

>  	if (ret || (retlen != rawlen)) {
>  		printk(KERN_NOTICE "Write of %d bytes at 0x%08x failed. returned %d, retlen %zd\n",
>                         rawlen, phys_ofs, ret, retlen);
> @@ -622,7 +626,7 @@
>  			jffs2_dbg_acct_sanity_check(c,jeb);
>  			jffs2_dbg_acct_paranoia_check(c, jeb);
>  
> -			ret = jffs2_reserve_space_gc(c, rawlen, &phys_ofs, &dummy);
> +			ret = jffs2_reserve_space_gc(c, rawlen, &phys_ofs, &dummy, rawlen); /* this is not optimal yet */
>  
>  			if (!ret) {
>  				D1(printk(KERN_DEBUG "Allocated space at 0x%08x to retry failed write.\n", phys_ofs));
> @@ -701,7 +705,7 @@
>  
>  	}
>  	
> -	ret = jffs2_reserve_space_gc(c, sizeof(ri) + mdatalen, &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(ri) + mdatalen, &phys_ofs, &alloclen, JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_metadata failed: %d\n",
>  		       sizeof(ri)+ mdatalen, ret);
> @@ -776,7 +780,7 @@
>  	rd.node_crc = cpu_to_je32(crc32(0, &rd, sizeof(rd)-8));
>  	rd.name_crc = cpu_to_je32(crc32(0, fd->name, rd.nsize));
>  	
> -	ret = jffs2_reserve_space_gc(c, sizeof(rd)+rd.nsize, &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(rd)+rd.nsize, &phys_ofs, &alloclen, JFFS2_SUMMARY_DIRENT_SIZE(rd.nsize));
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_dirent failed: %d\n",
>  		       sizeof(rd)+rd.nsize, ret);
> @@ -986,7 +990,7 @@
>  	ri.data_crc = cpu_to_je32(0);
>  	ri.node_crc = cpu_to_je32(crc32(0, &ri, sizeof(ri)-8));
>  
> -	ret = jffs2_reserve_space_gc(c, sizeof(ri), &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(ri), &phys_ofs, &alloclen, JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_hole failed: %d\n",
>  		       sizeof(ri), ret);
> @@ -1211,7 +1215,7 @@
>  		uint32_t cdatalen;
>  		uint16_t comprtype = JFFS2_COMPR_NONE;
>  
> -		ret = jffs2_reserve_space_gc(c, sizeof(ri) + JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen);
> +		ret = jffs2_reserve_space_gc(c, sizeof(ri) + JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen, JFFS2_SUMMARY_INODE_SIZE);
>  
>  		if (ret) {
>  			printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_dnode failed: %d\n",
> @@ -1268,4 +1272,3 @@
>  	jffs2_gc_release_page(c, pg_ptr, &pg);
>  	return ret;
>  }
> -
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/nodelist.h mtd/fs/jffs2/nodelist.h
> --- Mtd-orig/fs/jffs2/nodelist.h	2005-07-17 08:56:21.000000000 +0200
> +++ mtd/fs/jffs2/nodelist.h	2005-07-18 15:42:05.000000000 +0200
> @@ -20,6 +20,7 @@
>  #include <linux/jffs2.h>
>  #include <linux/jffs2_fs_sb.h>
>  #include <linux/jffs2_fs_i.h>
> +#include "summary.h"
>  
>  #ifdef __ECOS
>  #include "os-ecos.h"
> @@ -179,6 +180,10 @@
>  	int bad_count;
>  	uint32_t offset;		/* of this block in the MTD */
>  
> +#ifdef CONFIG_JFFS2_SUMMARY	
> +	struct jffs2_sum_info *sum_collected;
> +#endif
> +	
>  	uint32_t unchecked_size;
>  	uint32_t used_size;
>  	uint32_t dirty_size;
> @@ -316,8 +321,8 @@
>  
>  /* nodemgmt.c */
>  int jffs2_thread_should_wake(struct jffs2_sb_info *c);
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio);
> -int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio, uint32_t sumsize);
> +int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize);
>  int jffs2_add_physical_node_ref(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *new);
>  void jffs2_complete_reservation(struct jffs2_sb_info *c);
>  void jffs2_mark_node_obsolete(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *raw);
> @@ -378,6 +383,10 @@
>  /* scan.c */
>  int jffs2_scan_medium(struct jffs2_sb_info *c);
>  void jffs2_rotate_lists(struct jffs2_sb_info *c);
> +int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
> +				uint32_t ofs, uint32_t len);
> +struct jffs2_inode_cache *jffs2_scan_make_ino_cache(struct jffs2_sb_info *c, uint32_t ino);
> +
>  
>  /* build.c */
>  int jffs2_do_mount_fs(struct jffs2_sb_info *c);
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/nodemgmt.c mtd/fs/jffs2/nodemgmt.c
> --- Mtd-orig/fs/jffs2/nodemgmt.c	2005-07-17 08:56:21.000000000 +0200
> +++ mtd/fs/jffs2/nodemgmt.c	2005-07-18 15:42:05.000000000 +0200
> @@ -38,9 +38,9 @@
>   *	for the requested allocation.
>   */
>  
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize);
>  
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio)
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio, uint32_t sumsize)
>  {
>  	int ret = -EAGAIN;
>  	int blocksneeded = c->resv_blocks_write;
> @@ -129,7 +129,7 @@
>  			spin_lock(&c->erase_completion_lock);
>  		}
>  
> -		ret = jffs2_do_reserve_space(c, minsize, ofs, len);
> +		ret = jffs2_do_reserve_space(c, minsize, ofs, len, sumsize);
>  		if (ret) {
>  			D1(printk(KERN_DEBUG "jffs2_reserve_space: ret is %d\n", ret));
>  		}
> @@ -140,7 +140,7 @@
>  	return ret;
>  }
>  
> -int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len)
> +int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>  {
>  	int ret = -EAGAIN;
>  	minsize = PAD(minsize);
> @@ -149,7 +149,7 @@
>  
>  	spin_lock(&c->erase_completion_lock);
>  	while(ret == -EAGAIN) {
> -		ret = jffs2_do_reserve_space(c, minsize, ofs, len);
> +		ret = jffs2_do_reserve_space(c, minsize, ofs, len, sumsize);
>  		if (ret) {
>  		        D1(printk(KERN_DEBUG "jffs2_reserve_space_gc: looping, ret is %d\n", ret));
>  		}
> @@ -159,50 +159,112 @@
>  }
>  
>  /* Called with alloc sem _and_ erase_completion_lock */
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len)
> +static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>  {
>  	struct jffs2_eraseblock *jeb = c->nextblock;
> +	uint32_t nofree_size;
>  	
>   restart:
> -	if (jeb && minsize > jeb->free_size) {
> -		/* Skip the end of this block and file it as having some dirty space */
> -		/* If there's a pending write to it, flush now */
> -		if (jffs2_wbuf_dirty(c)) {
> -			spin_unlock(&c->erase_completion_lock);
> -			D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));			    
> -			jffs2_flush_wbuf_pad(c);
> -			spin_lock(&c->erase_completion_lock);
> -			jeb = c->nextblock;
> -			goto restart;
> -		}
> -		c->wasted_size += jeb->free_size;
> -		c->free_size -= jeb->free_size;
> -		jeb->wasted_size += jeb->free_size;
> -		jeb->free_size = 0;
> +	nofree_size = 0;
> +	
> +#ifdef CONFIG_JFFS2_SUMMARY
> +
> +	if (sumsize != JFFS2_SUMMARY_NOSUM_SIZE) {
> +		int ret;
> +                if (jeb) {
> +                        if ((ret=jffs2_sum_care_sum_collected(jeb))) return ret;
> +                        nofree_size = PAD(sumsize + jeb->sum_collected->sum_size + JFFS2_SUMMARY_FRAME_SIZE);
> +                }
>  		
> -		/* Check, if we have a dirty block now, or if it was dirty already */
> -		if (ISDIRTY (jeb->wasted_size + jeb->dirty_size)) {
> -			c->dirty_size += jeb->wasted_size;
> -			c->wasted_size -= jeb->wasted_size;
> -			jeb->dirty_size += jeb->wasted_size;
> -			jeb->wasted_size = 0;
> -			if (VERYDIRTY(c, jeb->dirty_size)) {
> -				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to very_dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +		D1(if (jeb) printk(KERN_DEBUG "JFFS2: minsize %d , jeb->free(%d) , sum_collected->size(%d) , sumsize(%d)\n",minsize,jeb->free_size,jeb->sum_collected->sum_size,sumsize));

This wraps even on my 169 column xterm.  Can you line-wrap at 80
columns before I have to buy a new monitor?

> +		
> +		if (jeb && (PAD(minsize) + PAD(jeb->sum_collected->sum_size + sumsize + JFFS2_SUMMARY_FRAME_SIZE) > jeb->free_size)) {
> +			D1(printk(KERN_DEBUG "JFFS2: generating summary for 0x%08x.\n", jeb->offset));
> +			if (jeb->sum_collected->sum_size == JFFS2_SUMMARY_NOSUM_SIZE) {
> +				sumsize = JFFS2_SUMMARY_NOSUM_SIZE;
> +				jffs2_sum_clean_collected(jeb);
> +				goto restart;
> +			}
> +			
> +			ret = jffs2_sum_write_sumnode(c);
> +			
> +			if (ret)
> +				return ret;
> +			
> +			if (jeb->sum_collected->sum_size == JFFS2_SUMMARY_NOSUM_SIZE) { //jffs2_write_sumnode can't write out the summary information
> +				sumsize = JFFS2_SUMMARY_NOSUM_SIZE;
> +				jffs2_sum_clean_collected(jeb);
> +				goto restart;
> +			}
> +			
> +			/* Check, if we have a dirty block now, or if it was dirty already */
> +			if (ISDIRTY (jeb->wasted_size + jeb->dirty_size)) {
> +				c->dirty_size += jeb->wasted_size;
> +				c->wasted_size -= jeb->wasted_size;
> +				jeb->dirty_size += jeb->wasted_size;
> +				jeb->wasted_size = 0;
> +				if (VERYDIRTY(c, jeb->dirty_size)) {
> +					D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to very_dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +					  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> +					list_add_tail(&jeb->list, &c->very_dirty_list);

Five levels of indentation are generally a sign that you should
rethink your code.

> +				} else {
> +					D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +					  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> +					list_add_tail(&jeb->list, &c->dirty_list);
> +				}
> +			} else { 
> +				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to clean_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
>  				  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -				list_add_tail(&jeb->list, &c->very_dirty_list);
> -			} else {
> -				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +				list_add_tail(&jeb->list, &c->clean_list);
> +			}
> +			c->nextblock = jeb = NULL;
> +		}
> +	}
> +	else {	

Keep the above two on the same line and remove extra whitespace at the
end of the line.

> +#endif			
> +		if (jeb && minsize > jeb->free_size) {
> +			/* Skip the end of this block and file it as having some dirty space */
> +			/* If there's a pending write to it, flush now */
> +			
> +			if (jffs2_wbuf_dirty(c)) {
> +				spin_unlock(&c->erase_completion_lock);
> +				D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));			    
> +				jffs2_flush_wbuf_pad(c);
> +				spin_lock(&c->erase_completion_lock);
> +				jeb = c->nextblock;
> +				goto restart;
> +			}
> +			
> +			c->wasted_size += jeb->free_size;
> +			c->free_size -= jeb->free_size;
> +			jeb->wasted_size += jeb->free_size;
> +			jeb->free_size = 0;
> +			
> +			/* Check, if we have a dirty block now, or if it was dirty already */
> +			if (ISDIRTY (jeb->wasted_size + jeb->dirty_size)) {
> +				c->dirty_size += jeb->wasted_size;
> +				c->wasted_size -= jeb->wasted_size;
> +				jeb->dirty_size += jeb->wasted_size;
> +				jeb->wasted_size = 0;
> +				if (VERYDIRTY(c, jeb->dirty_size)) {
> +					D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to very_dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +					  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> +					list_add_tail(&jeb->list, &c->very_dirty_list);
> +				} else {
> +					D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +					  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> +					list_add_tail(&jeb->list, &c->dirty_list);
> +				}
> +			} else { 
> +				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to clean_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
>  				  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -				list_add_tail(&jeb->list, &c->dirty_list);
> +				list_add_tail(&jeb->list, &c->clean_list);
>  			}
> -		} else { 
> -			D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to clean_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> -			  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -			list_add_tail(&jeb->list, &c->clean_list);
> +			c->nextblock = jeb = NULL;
>  		}
> -		c->nextblock = jeb = NULL;
> +#ifdef CONFIG_JFFS2_SUMMARY
>  	}
> -	
> +#endif	
>  	if (!jeb) {
>  		struct list_head *next;
>  		/* Take the next block off the 'free' list */
> @@ -266,7 +328,7 @@
>  	/* OK, jeb (==c->nextblock) is now pointing at a block which definitely has
>  	   enough space */
>  	*ofs = jeb->offset + (c->sector_size - jeb->free_size);
> -	*len = jeb->free_size;
> +	*len = jeb->free_size - nofree_size;
>  
>  	if (c->cleanmarker_size && jeb->used_size == c->cleanmarker_size &&
>  	    !jeb->first_node->next_in_ino) {
> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/os-linux.h mtd/fs/jffs2/os-linux.h
> --- Mtd-orig/fs/jffs2/os-linux.h	2005-07-17 13:13:46.000000000 +0200
> +++ mtd/fs/jffs2/os-linux.h	2005-07-18 15:42:05.000000000 +0200
> @@ -67,7 +67,13 @@
>  
>  #ifndef CONFIG_JFFS2_FS_WRITEBUFFER
>  #define SECTOR_ADDR(x) ( ((unsigned long)(x) & ~(c->sector_size-1)) )
> +
> +#ifndef CONFIG_JFFS2_SUMMARY

This bit confused me, until I noticed the "n" in ifndef.  But since
the whole conditional compilation stuff should go later on, don't
worry about it.

>  #define jffs2_can_mark_obsolete(c) (1)
> +#else
> +#define jffs2_can_mark_obsolete(c) (0)
> +#endif
> +
>  #define jffs2_is_writebuffered(c) (0)
>  #define jffs2_cleanmarker_oob(c) (0)
>  #define jffs2_write_nand_cleanmarker(c,jeb) (-EIO)
> @@ -94,7 +100,12 @@
>  
>  #define jffs2_is_writebuffered(c) (c->wbuf != NULL)
>  #define SECTOR_ADDR(x) ( ((unsigned long)(x) / (unsigned long)(c->sector_size)) * c->sector_size )
> +#ifndef CONFIG_JFFS2_SUMMARY
>  #define jffs2_can_mark_obsolete(c) ((c->mtd->type == MTD_NORFLASH && !(c->mtd->flags & MTD_ECC)) || c->mtd->type == MTD_RAM)
> +#else
> +#define jffs2_can_mark_obsolete(c) (0)
> +#endif
> +	

Whitespace pollution.

>  #define jffs2_cleanmarker_oob(c) (c->mtd->type == MTD_NANDFLASH)
>  
>  #define jffs2_flash_write_oob(c, ofs, len, retlen, buf) ((c)->mtd->write_oob((c)->mtd, ofs, len, retlen, buf))
> @@ -186,5 +197,3 @@
>  
>  
>  #endif /* __JFFS2_OS_LINUX_H__ */
> -
> -

Cleanup patch (or just remove).

> diff --unified --new-file --recursive Mtd-orig/fs/jffs2/scan.c mtd/fs/jffs2/scan.c
> --- Mtd-orig/fs/jffs2/scan.c	2005-07-17 08:56:21.000000000 +0200
> +++ mtd/fs/jffs2/scan.c	2005-07-18 15:42:05.000000000 +0200
> @@ -18,22 +18,10 @@
>  #include <linux/crc32.h>
>  #include <linux/compiler.h>
>  #include "nodelist.h"
> +#include "summary.h"
>  
>  #define DEFAULT_EMPTY_SCAN_SIZE 1024
>  
> -#define DIRTY_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->dirty_size += _x; \
> -		jeb->free_size -= _x ; jeb->dirty_size += _x; \
> -		}while(0)
> -#define USED_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->used_size += _x; \
> -		jeb->free_size -= _x ; jeb->used_size += _x; \
> -		}while(0)
> -#define UNCHECKED_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->unchecked_size += _x; \
> -		jeb->free_size -= _x ; jeb->unchecked_size += _x; \
> -		}while(0)
> -
>  #define noisy_printk(noise, args...) do { \
>  	if (*(noise)) { \
>  		printk(KERN_NOTICE args); \
> @@ -58,13 +46,6 @@
>  static int jffs2_scan_dirent_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>  				 struct jffs2_raw_dirent *rd, uint32_t ofs);
>  
> -#define BLK_STATE_ALLFF		0
> -#define BLK_STATE_CLEAN		1
> -#define BLK_STATE_PARTDIRTY	2
> -#define BLK_STATE_CLEANMARKER	3
> -#define BLK_STATE_ALLDIRTY	4
> -#define BLK_STATE_BADBLOCK	5
> -
>  static inline int min_free(struct jffs2_sb_info *c)
>  {
>  	uint32_t min = 2 * sizeof(struct jffs2_raw_inode);
> @@ -265,7 +246,7 @@
>  	return ret;
>  }
>  
> -static int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
> +int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
>  				uint32_t ofs, uint32_t len)
>  {
>  	int ret;
> @@ -294,6 +275,11 @@
>  	uint32_t hdr_crc, buf_ofs, buf_len;
>  	int err;
>  	int noise = 0;
> +
> +#ifdef CONFIG_JFFS2_SUMMARY
> +	struct jffs2_sum_marker *sm;
> +#endif	
> +
>  #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>  	int cleanmarkerfound = 0;
>  #endif
> @@ -319,10 +305,54 @@
>  		}
>  	}
>  #endif
> +	
> +#ifdef CONFIG_JFFS2_SUMMARY	
> +	sm = (struct jffs2_sum_marker *)kmalloc(sizeof(struct jffs2_sum_marker), GFP_KERNEL);
> +	if (!sm) {
> +	    return -ENOMEM;
> +	}

Kernel coding style is 1 tab for indentation and no extra braces
unless required.

> +	
> +	err = jffs2_fill_scan_buf(c, (unsigned char *) sm, jeb->offset + c->sector_size - sizeof(struct jffs2_sum_marker), sizeof(struct jffs2_sum_marker));
> +	
> +	if (err) {
> +		kfree(sm);
> +	        return err;
> +	}
> +	

Whitespace pollution.

> +	if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
> +
> +		if(je32_to_cpu(sm->erase_size) == c->sector_size) {
> +			int ret = jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);

Why introduce a new variable?  Can't err and ret be merged into one?

> +			
> +			if (ret) {
> +				kfree(sm);
> +				return ret;
> +			}
> +		}
> +		
> +		printk(KERN_WARNING "FS erase_block_size != JFFS2 erase_block_size => skipping summary information\n");
> +		
> +	}
> +	
> +	kfree(sm);
> +	
> +	ofs = jeb->offset;
> +	prevofs = jeb->offset - 1;
> +	

Jörn

-- 
Everything should be made as simple as possible, but not simpler.
-- Albert Einstein

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[David Woodhouse]

On Tue, 2005-07-26 at 12:03 +0200, Jörn Engel wrote:
> o 80 columns (see CodingStyle)

Go easy on that one. I just rejected a patch from Artem because he went
overboard with the 80-column thing. Sometimes it's better to have longer
lines than that.

-- 
dwmw2

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Wed, 27 July 2005 23:08:03 +0100, David Woodhouse wrote:
> On Tue, 2005-07-26 at 12:03 +0200, Jörn Engel wrote:
> > o 80 columns (see CodingStyle)
> 
> Go easy on that one. I just rejected a patch from Artem because he went
> overboard with the 80-column thing. Sometimes it's better to have longer
> lines than that.

Did I hear someone volunteer for the next review? ;)

Jörn

-- 
Debugging is twice as hard as writing the code in the first place.
Therefore, if you write the code as cleverly as possible, you are,
by definition, not smart enough to debug it.
-- Brian W. Kernighan

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Havasi Ferenc]

Hi Jörn,

>Review is focused on the simple stuff.  Most of the comments can be
>repeated over and over, so just go through it yourself.  You should
>focus on:
>o Documentation/CodingStyle
>o empty whitespace at the end of lines
>o 80 columns (see CodingStyle)
>
>When most of that stuff is done, resend the patch for the next round
>of review.
>  
>
Thanks for the review, the modified patch is at
http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch.

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 1 August 2005 11:50:04 +0200, Havasi Ferenc wrote:
> >
> Thanks for the review, the modified patch is at
> http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch.

Not Found
The requested URL /jffs2/jffs2-summary-20050729.patch. was not found on this server.

Am I too quick?

Jörn

-- 
Data dominates. If you've chosen the right data structures and organized
things well, the algorithms will almost always be self-evident. Data
structures, not algorithms, are central to programming.
-- Rob Pike

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Havasi Ferenc]

Jörn Engel wrote:

>On Mon, 1 August 2005 11:50:04 +0200, Havasi Ferenc wrote:
>  
>
>>Thanks for the review, the modified patch is at
>>http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch.
>>    
>>
>
>Not Found
>The requested URL /jffs2/jffs2-summary-20050729.patch. was not found on this server.
>  
>
Sorry, the "." was the end of the sentence, not the part of the URL. So
it is:
http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 1 August 2005 12:07:44 +0200, Havasi Ferenc wrote:
> Jörn Engel wrote:
> >On Mon, 1 August 2005 11:50:04 +0200, Havasi Ferenc wrote:
> >
> >>Thanks for the review, the modified patch is at
> >>http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch.
> >
> >Not Found
> >The requested URL /jffs2/jffs2-summary-20050729.patch. was not found on this server.
> >
> Sorry, the "." was the end of the sentence, not the part of the URL. So
> it is:
> http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050729.patch

A classical problem.  :)


> diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
> --- Mtd-orig/fs/jffs2/build.c	2005-07-22 12:32:07.000000000 +0200
> +++ mtd/fs/jffs2/build.c	2005-07-29 18:43:16.000000000 +0200
> @@ -334,6 +334,9 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
>  		c->blocks[i].first_node = NULL;
>  		c->blocks[i].last_node = NULL;
>  		c->blocks[i].bad_count = 0;
> +#ifdef CONFIG_JFFS2_SUMMARY
> +		c->blocks[i].sum_collected = NULL;
> +#endif

I guess we can start removing some of the #ifdef ugliness already.
The extra field per eraseblock can be added unconditionally, as can
this line.

> diff -Narup Mtd-orig/fs/jffs2/erase.c mtd/fs/jffs2/erase.c
> --- Mtd-orig/fs/jffs2/erase.c	2005-07-22 12:32:08.000000000 +0200
> +++ mtd/fs/jffs2/erase.c	2005-07-29 18:43:16.000000000 +0200
> @@ -425,6 +425,14 @@ static void jffs2_mark_erased_block(stru
>  		jeb->wasted_size = 0;
>  	}
>  
> +#ifdef CONFIG_JFFS2_SUMMARY
> +	if (jeb->sum_collected) {
> +		jffs2_sum_clean_collected(jeb);
> +		jeb->sum_collected->sum_size = 0;
> +		jeb->sum_collected->sum_padded = 0;
> +	}
> +#endif
> +

This part could be packaged in a small function.  With a bit of header
magic like this:

#ifdef CONFIG_JFFS2_SUMMARY
void jffs2_sum_clean_collected(struct jffs2_eraseblock *jeb);
#else
void inline jffs2_sum_clean_collected(struct jffs2_eraseblock *jeb) {}
#endif

You can call that function unconditionally here.  No ifdefs in the
already ugly function.

> diff -Narup Mtd-orig/fs/jffs2/gc.c mtd/fs/jffs2/gc.c
> --- Mtd-orig/fs/jffs2/gc.c	2005-07-24 17:14:14.000000000 +0200
> +++ mtd/fs/jffs2/gc.c	2005-07-29 18:43:16.000000000 +0200
> @@ -505,6 +505,7 @@ static int jffs2_garbage_collect_pristin
>  	uint32_t phys_ofs, alloclen;
>  	uint32_t crc, rawlen;
>  	int retried = 0;
> +	struct kvec vecs[1];

Funky.

>  	D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
>  
> @@ -513,8 +514,9 @@ static int jffs2_garbage_collect_pristin
>  	/* Ask for a small amount of space (or the totlen if smaller) because we
>  	   don't want to force wastage of the end of a block if splitting would
>  	   work. */
> -	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
> -					      rawlen), &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
> +				JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
> +				/* this is not optimal yet */

Can you add some more explanation to the comment?

>  	if (ret)
>  		return ret;
>  
> @@ -594,7 +596,10 @@ static int jffs2_garbage_collect_pristin
>  	nraw->__totlen = rawlen;
>  	nraw->next_phys = NULL;
>  
> -	ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
> +	vecs[0].iov_base = (unsigned char *) node;
> +	vecs[0].iov_len = rawlen;
> +
> +	ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);

Why this change?  If it's required, it might make sense to split it
out into a preparatory patch.  But my gut feeling is that dropping
this part would be better.

> diff -Narup Mtd-orig/fs/jffs2/nodelist.h mtd/fs/jffs2/nodelist.h
> --- Mtd-orig/fs/jffs2/nodelist.h	2005-07-27 16:46:11.000000000 +0200
> +++ mtd/fs/jffs2/nodelist.h	2005-07-29 18:43:16.000000000 +0200
> @@ -20,6 +20,7 @@
>  #include <linux/jffs2.h>
>  #include <linux/jffs2_fs_sb.h>
>  #include <linux/jffs2_fs_i.h>
> +#include "summary.h"
>  
>  #ifdef __ECOS
>  #include "os-ecos.h"
> @@ -179,6 +180,10 @@ struct jffs2_eraseblock
>  	int bad_count;
>  	uint32_t offset;		/* of this block in the MTD */
>  
> +#ifdef CONFIG_JFFS2_SUMMARY	
> +	struct jffs2_sum_info *sum_collected;
> +#endif
> +
>  	uint32_t unchecked_size;
>  	uint32_t used_size;
>  	uint32_t dirty_size;

Make it unconditional (see above).

> @@ -313,8 +318,8 @@ int jffs2_add_full_dnode_to_inode(struct
>  
>  /* nodemgmt.c */
>  int jffs2_thread_should_wake(struct jffs2_sb_info *c);
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio);
> -int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio, uint32_t sumsize);
> +int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize);

128 columns. *grumble*

Dwmw2 has a different opinion on this matter (which he didn't bother
to explain, so far.) *grumble*

>  int jffs2_add_physical_node_ref(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *new);
>  void jffs2_complete_reservation(struct jffs2_sb_info *c);
>  void jffs2_mark_node_obsolete(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *raw);
> @@ -374,6 +379,10 @@ char *jffs2_getlink(struct jffs2_sb_info
>  /* scan.c */
>  int jffs2_scan_medium(struct jffs2_sb_info *c);
>  void jffs2_rotate_lists(struct jffs2_sb_info *c);
> +int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
                          ^
This space can be removed.
> +				uint32_t ofs, uint32_t len);
> +struct jffs2_inode_cache *jffs2_scan_make_ino_cache(struct jffs2_sb_info *c, uint32_t ino);
> +int jffs2_scan_classify_jeb(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
>  
>  /* build.c */
>  int jffs2_do_mount_fs(struct jffs2_sb_info *c);
> diff -Narup Mtd-orig/fs/jffs2/nodemgmt.c mtd/fs/jffs2/nodemgmt.c
> --- Mtd-orig/fs/jffs2/nodemgmt.c	2005-07-20 17:32:28.000000000 +0200
> +++ mtd/fs/jffs2/nodemgmt.c	2005-07-29 18:43:16.000000000 +0200
> @@ -18,6 +18,12 @@
>  #include <linux/sched.h> /* For cond_resched() */
>  #include "nodelist.h"
>  
> +#ifdef CONFIG_JFFS2_SUMMARY
> +#define jffs2_do_reserve_space(a,b,c,d,e) jffs2_do_reserve_space_sum(a,b,c,d,e)
> +#else
> +#define jffs2_do_reserve_space(a,b,c,d,e) jffs2_do_reserve_space_normal(a,b,c,d,e)
> +#endif
> +

Yuck!  Normally, this should be moved into some header.  I do see that
both functions are static, though, so that doesn't help.

This needs to be changed somehow.  I don't have a good idea yet, so
maybe you can come up with something.  If necessary, do a preparatory
cleanup patch for nodemgmt.c.

>  /**
>   *	jffs2_reserve_space - request physical space to write nodes to flash
>   *	@c: superblock info
> @@ -38,9 +44,9 @@
>   *	for the requested allocation.
>   */
>  
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize);
>  
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio)
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio, uint32_t sumsize)

*grumble*

> @@ -158,52 +164,36 @@ int jffs2_reserve_space_gc(struct jffs2_
>  	return ret;
>  }
>  
> -/* Called with alloc sem _and_ erase_completion_lock */
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len)
> +
> +static void jffs2_close_nextblock(struct jffs2_sb_info *c, struct jffs2_eraseblock **jeb)
>  {

Not reviewed yet - this still need another round anyway and lunch is
waiting...

> +#ifdef CONFIG_JFFS2_SUMMARY
> +
> +/* Called with alloc sem _and_ erase_completion_lock */
> +static int jffs2_do_reserve_space_sum(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> +{
[...]
> +#else
> +
> +/* Called with alloc sem _and_ erase_completion_lock */
> +static int jffs2_do_reserve_space_normal(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> +{

It doesn't take too much creativity to combine this and the above
#ifdef into a single one.  That will still be ugly, but it's a step in
the right direction.

Still didn't review the whole patch, but I'm getting hungry.  Should
be enough to keep you busy for a moment.

Jörn

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

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Jörn Engel wrote:

>>diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
>>--- Mtd-orig/fs/jffs2/build.c	2005-07-22 12:32:07.000000000 +0200
>>+++ mtd/fs/jffs2/build.c	2005-07-29 18:43:16.000000000 +0200
>>@@ -334,6 +334,9 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
>> 		c->blocks[i].first_node = NULL;
>> 		c->blocks[i].last_node = NULL;
>> 		c->blocks[i].bad_count = 0;
>>+#ifdef CONFIG_JFFS2_SUMMARY
>>+		c->blocks[i].sum_collected = NULL;
>>+#endif
>>    
>>
>
>I guess we can start removing some of the #ifdef ugliness already.
>The extra field per eraseblock can be added unconditionally, as can
>this line.
>
>  
>
OK, we will eliminate all of these kind of #endifs.

>> 	D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
>> 
>>@@ -513,8 +514,9 @@ static int jffs2_garbage_collect_pristin
>> 	/* Ask for a small amount of space (or the totlen if smaller) because we
>> 	   don't want to force wastage of the end of a block if splitting would
>> 	   work. */
>>-	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
>>-					      rawlen), &phys_ofs, &alloclen);
>>+	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
>>+				JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
>>+				/* this is not optimal yet */
>>    
>>
>
>Can you add some more explanation to the comment?
>  
>
OK, we will.

The reason of "this is not optimal" is the following: for
jffs2_reserve_space_gc() we have to specify an argument called sumsize.
It is the summary size of the requested node. It needs it, because it is
necessary to fit not only the node but also its summary representation
into the erase block. In all other cases we know exactly the summary
size of all nodes except in this case, so we made an upper-estimation here.

>> 	if (ret)
>> 		return ret;
>> 
>>@@ -594,7 +596,10 @@ static int jffs2_garbage_collect_pristin
>> 	nraw->__totlen = rawlen;
>> 	nraw->next_phys = NULL;
>> 
>>-	ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
>>+	vecs[0].iov_base = (unsigned char *) node;
>>+	vecs[0].iov_len = rawlen;
>>+
>>+	ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);
>>    
>>
>
>Why this change?  If it's required, it might make sense to split it
>out into a preparatory patch.  But my gut feeling is that dropping
>this part would be better.
>  
>
Jffs2_flash_write() simply write directly the data onto the flash, and
does not collect the summary information. Jffs2_flash_writev() does.

>>+#ifdef CONFIG_JFFS2_SUMMARY
>>+
>>+/* Called with alloc sem _and_ erase_completion_lock */
>>+static int jffs2_do_reserve_space_sum(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>>+{
>>    
>>
>[...]
>  
>
>>+#else
>>+
>>+/* Called with alloc sem _and_ erase_completion_lock */
>>+static int jffs2_do_reserve_space_normal(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
>>+{
>>    
>>
>
>It doesn't take too much creativity to combine this and the above
>#ifdef into a single one.  That will still be ugly, but it's a step in
>the right direction.
>  
>
In the previous version it was combined, but we afraid of that it is
hard to see what exectly happen. But we can tranform it back.

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 1 August 2005 16:02:32 +0200, Ferenc Havasi wrote:
> Jörn Engel wrote:
> 
> >> 	D1(printk(KERN_DEBUG "Going to GC REF_PRISTINE node at 0x%08x\n", ref_offset(raw)));
> >> 
> >>@@ -513,8 +514,9 @@ static int jffs2_garbage_collect_pristin
> >> 	/* Ask for a small amount of space (or the totlen if smaller) because we
> >> 	   don't want to force wastage of the end of a block if splitting would
> >> 	   work. */
> >>-	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
> >>-					      rawlen), &phys_ofs, &alloclen);
> >>+	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
> >>+				JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
> >>+				/* this is not optimal yet */
> >
> >Can you add some more explanation to the comment?
> >
> OK, we will.
> 
> The reason of "this is not optimal" is the following: for
> jffs2_reserve_space_gc() we have to specify an argument called sumsize.
> It is the summary size of the requested node. It needs it, because it is
> necessary to fit not only the node but also its summary representation
> into the erase block. In all other cases we know exactly the summary
> size of all nodes except in this case, so we made an upper-estimation here.

Makes sense.

> >> 	if (ret)
> >> 		return ret;
> >> 
> >>@@ -594,7 +596,10 @@ static int jffs2_garbage_collect_pristin
> >> 	nraw->__totlen = rawlen;
> >> 	nraw->next_phys = NULL;
> >> 
> >>-	ret = jffs2_flash_write(c, phys_ofs, rawlen, &retlen, (char *)node);
> >>+	vecs[0].iov_base = (unsigned char *) node;
> >>+	vecs[0].iov_len = rawlen;
> >>+
> >>+	ret = jffs2_flash_writev(c, vecs, 1, phys_ofs, &retlen, 0);
> >
> >Why this change?  If it's required, it might make sense to split it
> >out into a preparatory patch.  But my gut feeling is that dropping
> >this part would be better.
> >
> Jffs2_flash_write() simply write directly the data onto the flash, and
> does not collect the summary information. Jffs2_flash_writev() does.

Then why can't you just re-implement jffs2_flash_write() using
Jffs2_flash_writev()?  That way, you keep the strange one-element
array logic confined in a small and simple function.  Here, we have an
already large and complicated one getting larger and more complicated.

Ok, maybe I'm missing something, so please tell me.

> >>+#ifdef CONFIG_JFFS2_SUMMARY
> >>+
> >>+/* Called with alloc sem _and_ erase_completion_lock */
> >>+static int jffs2_do_reserve_space_sum(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> >>+{
> >>
> >[...]
> >  
> >
> >>+#else
> >>+
> >>+/* Called with alloc sem _and_ erase_completion_lock */
> >>+static int jffs2_do_reserve_space_normal(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> >>+{
> >
> >It doesn't take too much creativity to combine this and the above
> >#ifdef into a single one.  That will still be ugly, but it's a step in
> >the right direction.
> >
> In the previous version it was combined, but we afraid of that it is
> hard to see what exectly happen. But we can tranform it back.

Hmm.  Either way, I'm not happy yet.  What I'd like to see is a patch
containing three types of changes:
o condition new code in fs/jffs2/summary.c,
o unconditional changes in existing source files and
o no-too-ugly #ifdefs in headers.

Nothing else.  I guess the converged variant is a better basis to work
towards that goal, but I could be wrong.  This one clearly needs more
thinking.

Jörn

-- 
Simplicity is prerequisite for reliability.
-- Edsger W. Dijkstra

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Jörn,

Jörn Engel wrote:

>Hmm.  Either way, I'm not happy yet.  What I'd like to see is a patch
>containing three types of changes:
>  
>
Here is our new patch: 
http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050811-v2.patch

We did all of the asked transformation - hopefully.

Thanks: Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> Here is our new patch: 
> http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050811-v2.patch
> 
> We did all of the asked transformation - hopefully.
> 

Hello Ferenc,

I wonder, did you try your summaries with my last fragtree changes? Does 
they work well ?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Artem,

> I wonder, did you try your summaries with my last fragtree changes? 
> Does they work well ?

We tried the latest snapshot with summary and without summary and it 
reported CRC errors. We tried it with older JFFS2 snapshot and it worked 
well.

Our plan was - even if Jörn and David say our patch is OK -, to wait 
commiting our patch to keep the picture clear, and try to help you to 
find the bug in your improvements (which has a nice result). But 
becuseof our and your holidays it may will be later...

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

On Thu, 2005-08-11 at 17:59 +0100, Ferenc Havasi wrote:
> Hi Artem,
> 
> > I wonder, did you try your summaries with my last fragtree changes? 
> > Does they work well ?
> 
> We tried the latest snapshot with summary and without summary and it 
> reported CRC errors. We tried it with older JFFS2 snapshot and it worked 
> well.
Which CRC errors?
Ferenc, please, if you've met some problems, don't keeps silence. You
know the procedure - logs, debug enable, etc. Thanks.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Artem!

>>We tried the latest snapshot with summary and without summary and it 
>>reported CRC errors. We tried it with older JFFS2 snapshot and it worked 
>>well.
>>    
>>
>Which CRC errors?
>Ferenc, please, if you've met some problems, don't keeps silence. You
>know the procedure - logs, debug enable, etc. Thanks.
>  
>
I remember a letter you wrote to mtd list about "don't use recent JFFS2 
- try earlier". I thought it means that you already know about some 
bugs, and our problem is related to them.

Zoltan (my college) will send you more information soon.

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> I remember a letter you wrote to mtd list about "don't use recent JFFS2 
> - try earlier". I thought it means that you already know about some 
> bugs, and our problem is related to them.
> 
Hmm, I couldn't have say this, if I commit something this is assumed to 
be usable...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Artem B. Bityuckiy írta:

> Ferenc Havasi wrote:
>
>> I remember a letter you wrote to mtd list about "don't use recent 
>> JFFS2 - try earlier". I thought it means that you already know about 
>> some bugs, and our problem is related to them.
>>
> Hmm, I couldn't have say this, if I commit something this is assumed 
> to be usable...

I misunderstood you, sorry.

We analized the problem more deeply, and with mtdram it still works 
correctly. So it seems that the problem is not in the recent JFFS2 but 
in nandsim.

It writes

[nandsim] errorr: write_byte: chip is disabled, ignore write

with its default configuration. It worked well earlier. Maybe there were 
changes in mtd...

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> It writes
> 
> [nandsim] errorr: write_byte: chip is disabled, ignore write
> 
> with its default configuration. It worked well earlier. Maybe there were 
> changes in mtd...
> 
Hmm, I also see this message. But nandsim still works just fine..

Thomas, nandsim issues a warning that one sends command to nand while it 
is disabled. Not sure, but isn't this patch should be applied to 
nand_base.c:

--- nand_base.c 2005-08-05 14:44:30.000000000 +0400
+++ nand_base_fixed.c   2005-08-16 17:13:41.620785252 +0400
@@ -2652,6 +2652,9 @@ int nand_scan (struct mtd_info *mtd, int

         mtd->owner = THIS_MODULE;

+       /* Select the device */
+       this->select_chip(mtd, 0);
+
         /* Reset the chip */
         this->cmdfunc (mtd, NAND_CMD_RESET, -1, -1);

At line 2621 we did:

         /* De-select the device */
         this->select_chip(mtd, -1);

so it seems logical to enable it before sending the reset command, isn't 
it ?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Thu, 11 August 2005 17:03:10 +0200, Ferenc Havasi wrote:
> Jörn Engel wrote:
> 
> >Hmm.  Either way, I'm not happy yet.  What I'd like to see is a patch
> >containing three types of changes:
> > 
> >
> Here is our new patch: 
> http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050811-v2.patch
> 
> We did all of the asked transformation - hopefully.

I'll take a closer look over the weekend - which happens to start in
about five minutes.  And just in case you'd get bored without
feedback, you still don't comply to the codingstyle.  Things like 
+	}
+	else {
should completely go away, not just where I complained before.

But it's getting better.  Good.

Jörn

-- 
Admonish your friends privately, but praise them openly.
-- Publilius Syrus 

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Thu, 11 August 2005 17:03:10 +0200, Ferenc Havasi wrote:
>
> Here is our new patch: 
> http://www.inf.u-szeged.hu/jffs2/jffs2-summary-20050811-v2.patch

Next round of comments below.

Jörn

-- 
To my face you have the audacity to advise me to become a thief - the worst
kind of thief that is conceivable, a thief of spiritual things, a thief of
ideas! It is insufferable, intolerable!
-- M. Binet in Scarabouche

There are still quite a few non-conformances to coding style.  I
suggest you run Lindent over the new code.  Or use this one instead,
as it ignored long lines:
indent -kr -i8 -ts8 -sob -l0 -ss -ncs "$@"


My comments are not strictly in order, as I jumped back and forth
through the code.  I hope you still get their meaning.

One big issue for this round is the number of explicit casts.  Most of
them are plain superfluous.  The rest can be removed by changing types
from uchar* to void*.  If there are any problematic ones you're not
sure about, I'll take a look in the next review round.

> diff -Narup Mtd-orig/fs/Kconfig mtd/fs/Kconfig
> --- Mtd-orig/fs/Kconfig	2005-05-09 10:16:08.000000000 +0200
> +++ mtd/fs/Kconfig	2005-08-11 15:35:06.000000000 +0200
> @@ -64,6 +64,19 @@ config JFFS2_FS_WRITEBUFFER
>  	    - NOR flash with transparent ECC
>  	    - DataFlash
>  
> +config JFFS2_SUMMARY
> +        bool "JFFS2 summary support (EXPERIMENTAL)"
> +        depends on JFFS2_FS && EXPERIMENTAL
> +        default n
> +        help 
> +          This feature makes it possible to use summary information
> +          for faster filesystem mount - specially on NAND.
> +
> +          The summary information can be inserted into a filesystem image
> +          by the utility 'sumtool'.
> +
> +          If unsure, say 'N'.
> +
>  config JFFS2_COMPRESSION_OPTIONS
>  	bool "Advanced compression options for JFFS2"
>  	default n
> diff -Narup Mtd-orig/fs/jffs2/Makefile.common mtd/fs/jffs2/Makefile.common
> --- Mtd-orig/fs/jffs2/Makefile.common	2005-07-17 08:56:20.000000000 +0200
> +++ mtd/fs/jffs2/Makefile.common	2005-08-11 15:17:41.000000000 +0200
> @@ -15,3 +15,4 @@ jffs2-$(CONFIG_JFFS2_FS_WRITEBUFFER)	+= 
>  jffs2-$(CONFIG_JFFS2_RUBIN)	+= compr_rubin.o
>  jffs2-$(CONFIG_JFFS2_RTIME)	+= compr_rtime.o
>  jffs2-$(CONFIG_JFFS2_ZLIB)	+= compr_zlib.o
> +jffs2-$(CONFIG_JFFS2_SUMMARY)   += summary.o
> diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
> --- Mtd-orig/fs/jffs2/build.c	2005-07-30 17:29:27.000000000 +0200
> +++ mtd/fs/jffs2/build.c	2005-08-11 15:17:41.000000000 +0200
> @@ -336,6 +336,7 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
>  		c->blocks[i].first_node = NULL;
>  		c->blocks[i].last_node = NULL;
>  		c->blocks[i].bad_count = 0;
> +		c->blocks[i].sum_collected = NULL;
>  	}
>  
>  	INIT_LIST_HEAD(&c->clean_list);
> diff -Narup Mtd-orig/fs/jffs2/dir.c mtd/fs/jffs2/dir.c
> --- Mtd-orig/fs/jffs2/dir.c	2005-07-17 13:13:46.000000000 +0200
> +++ mtd/fs/jffs2/dir.c	2005-08-11 15:17:41.000000000 +0200
> @@ -304,7 +304,8 @@ static int jffs2_symlink (struct inode *
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + targetlen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -364,7 +365,8 @@ static int jffs2_symlink (struct inode *
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> @@ -449,7 +451,8 @@ static int jffs2_mkdir (struct inode *di
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL,
> +				JFFS2_SUMMARY_INODE_SIZE);
>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -492,7 +495,8 @@ static int jffs2_mkdir (struct inode *di
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> @@ -601,7 +605,8 @@ static int jffs2_mknod (struct inode *di
>  	 * Just the node will do for now, though 
>  	 */
>  	namelen = dentry->d_name.len;
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + devlen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
> @@ -646,7 +651,8 @@ static int jffs2_mknod (struct inode *di
>  	up(&f->sem);
>  
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		/* Eep. */
>  		jffs2_clear_inode(inode);
> diff -Narup Mtd-orig/fs/jffs2/erase.c mtd/fs/jffs2/erase.c
> --- Mtd-orig/fs/jffs2/erase.c	2005-07-22 12:32:08.000000000 +0200
> +++ mtd/fs/jffs2/erase.c	2005-08-11 15:17:41.000000000 +0200
> @@ -425,6 +425,8 @@ static void jffs2_mark_erased_block(stru
>  		jeb->wasted_size = 0;
>  	}
>  
> +	jffs2_sum_reset_collected(jeb);
> +
>  	spin_lock(&c->erase_completion_lock);
>  	c->erasing_size -= c->sector_size;
>  	c->free_size += jeb->free_size;
> diff -Narup Mtd-orig/fs/jffs2/file.c mtd/fs/jffs2/file.c
> --- Mtd-orig/fs/jffs2/file.c	2005-07-06 14:13:09.000000000 +0200
> +++ mtd/fs/jffs2/file.c	2005-08-11 15:17:41.000000000 +0200
> @@ -137,7 +137,8 @@ static int jffs2_prepare_write (struct f
>  		D1(printk(KERN_DEBUG "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
>  			  (unsigned int)inode->i_size, pageofs));
>  
> -		ret = jffs2_reserve_space(c, sizeof(ri), &phys_ofs, &alloc_len, ALLOC_NORMAL);
> +		ret = jffs2_reserve_space(c, sizeof(ri), &phys_ofs, &alloc_len,
> +					ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  		if (ret)
>  			return ret;
>  
> diff -Narup Mtd-orig/fs/jffs2/fs.c mtd/fs/jffs2/fs.c
> --- Mtd-orig/fs/jffs2/fs.c	2005-08-06 06:51:30.000000000 +0200
> +++ mtd/fs/jffs2/fs.c	2005-08-11 15:17:41.000000000 +0200
> @@ -74,7 +74,8 @@ static int jffs2_do_setattr (struct inod
>  		return -ENOMEM;
>  	}
>  		
> -	ret = jffs2_reserve_space(c, sizeof(*ri) + mdatalen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri) + mdatalen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		jffs2_free_raw_inode(ri);
>  		if (S_ISLNK(inode->i_mode & S_IFMT))
> diff -Narup Mtd-orig/fs/jffs2/gc.c mtd/fs/jffs2/gc.c
> --- Mtd-orig/fs/jffs2/gc.c	2005-07-24 17:14:14.000000000 +0200
> +++ mtd/fs/jffs2/gc.c	2005-08-11 15:17:41.000000000 +0200
> @@ -513,8 +513,11 @@ static int jffs2_garbage_collect_pristin
>  	/* Ask for a small amount of space (or the totlen if smaller) because we
>  	   don't want to force wastage of the end of a block if splitting would
>  	   work. */
> -	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) + JFFS2_MIN_DATA_LEN, 
> -					      rawlen), &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, min_t(uint32_t, sizeof(struct jffs2_raw_inode) +
> +				JFFS2_MIN_DATA_LEN, rawlen), &phys_ofs, &alloclen, rawlen);
> +				/* this is not the exact summary size of it,
> +					it is only an upper estimation */
> +
>  	if (ret)
>  		return ret;
>  
> @@ -622,7 +625,9 @@ static int jffs2_garbage_collect_pristin
>  			jffs2_dbg_acct_sanity_check(c,jeb);
>  			jffs2_dbg_acct_paranoia_check(c, jeb);
>  
> -			ret = jffs2_reserve_space_gc(c, rawlen, &phys_ofs, &dummy);
> +			ret = jffs2_reserve_space_gc(c, rawlen, &phys_ofs, &dummy, rawlen);
> +						/* this is not the exact summary size of it,
> +							it is only an upper estimation */
>  
>  			if (!ret) {
>  				D1(printk(KERN_DEBUG "Allocated space at 0x%08x to retry failed write.\n", phys_ofs));
> @@ -701,7 +706,8 @@ static int jffs2_garbage_collect_metadat
>  
>  	}
>  	
> -	ret = jffs2_reserve_space_gc(c, sizeof(ri) + mdatalen, &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(ri) + mdatalen, &phys_ofs, &alloclen,
> +				JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_metadata failed: %d\n",
>  		       sizeof(ri)+ mdatalen, ret);
> @@ -776,7 +782,8 @@ static int jffs2_garbage_collect_dirent(
>  	rd.node_crc = cpu_to_je32(crc32(0, &rd, sizeof(rd)-8));
>  	rd.name_crc = cpu_to_je32(crc32(0, fd->name, rd.nsize));
>  	
> -	ret = jffs2_reserve_space_gc(c, sizeof(rd)+rd.nsize, &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(rd)+rd.nsize, &phys_ofs, &alloclen,
> +				JFFS2_SUMMARY_DIRENT_SIZE(rd.nsize));
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_dirent failed: %d\n",
>  		       sizeof(rd)+rd.nsize, ret);
> @@ -986,7 +993,8 @@ static int jffs2_garbage_collect_hole(st
>  	ri.data_crc = cpu_to_je32(0);
>  	ri.node_crc = cpu_to_je32(crc32(0, &ri, sizeof(ri)-8));
>  
> -	ret = jffs2_reserve_space_gc(c, sizeof(ri), &phys_ofs, &alloclen);
> +	ret = jffs2_reserve_space_gc(c, sizeof(ri), &phys_ofs, &alloclen,
> +				JFFS2_SUMMARY_INODE_SIZE);
>  	if (ret) {
>  		printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_hole failed: %d\n",
>  		       sizeof(ri), ret);
> @@ -1211,7 +1219,8 @@ static int jffs2_garbage_collect_dnode(s
>  		uint32_t cdatalen;
>  		uint16_t comprtype = JFFS2_COMPR_NONE;
>  
> -		ret = jffs2_reserve_space_gc(c, sizeof(ri) + JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen);
> +		ret = jffs2_reserve_space_gc(c, sizeof(ri) + JFFS2_MIN_DATA_LEN, &phys_ofs,
> +					&alloclen, JFFS2_SUMMARY_INODE_SIZE);
>  
>  		if (ret) {
>  			printk(KERN_WARNING "jffs2_reserve_space_gc of %zd bytes for garbage_collect_dnode failed: %d\n",
> @@ -1268,4 +1277,3 @@ static int jffs2_garbage_collect_dnode(s
>  	jffs2_gc_release_page(c, pg_ptr, &pg);
>  	return ret;
>  }
> -
> diff -Narup Mtd-orig/fs/jffs2/nodelist.h mtd/fs/jffs2/nodelist.h
> --- Mtd-orig/fs/jffs2/nodelist.h	2005-08-01 14:05:19.000000000 +0200
> +++ mtd/fs/jffs2/nodelist.h	2005-08-11 15:17:41.000000000 +0200
> @@ -20,6 +20,7 @@
>  #include <linux/jffs2.h>
>  #include <linux/jffs2_fs_sb.h>
>  #include <linux/jffs2_fs_i.h>
> +#include "summary.h"
>  
>  #ifdef __ECOS
>  #include "os-ecos.h"
> @@ -195,6 +196,7 @@ struct jffs2_eraseblock
>  	struct jffs2_raw_node_ref *last_node;
>  
>  	struct jffs2_raw_node_ref *gc_node;	/* Next node to be garbage collected */
> +	struct jffs2_sum_info *sum_collected;
>  };
>  
>  /* Calculate totlen from surrounding nodes or eraseblock */
> @@ -321,8 +323,10 @@ int jffs2_add_older_frag_to_fragtree(str
>  
>  /* nodemgmt.c */
>  int jffs2_thread_should_wake(struct jffs2_sb_info *c);
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio);
> -int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs,
> +			uint32_t *len, int prio, uint32_t sumsize);
> +int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs,
> +			uint32_t *len, uint32_t sumsize);
>  int jffs2_add_physical_node_ref(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *new);
>  void jffs2_complete_reservation(struct jffs2_sb_info *c);
>  void jffs2_mark_node_obsolete(struct jffs2_sb_info *c, struct jffs2_raw_node_ref *raw);
> @@ -381,6 +385,10 @@ char *jffs2_getlink(struct jffs2_sb_info
>  /* scan.c */
>  int jffs2_scan_medium(struct jffs2_sb_info *c);
>  void jffs2_rotate_lists(struct jffs2_sb_info *c);
> +int jffs2_fill_scan_buf(struct jffs2_sb_info *c, unsigned char *buf,
> +				uint32_t ofs, uint32_t len);
> +struct jffs2_inode_cache *jffs2_scan_make_ino_cache(struct jffs2_sb_info *c, uint32_t ino);
> +int jffs2_scan_classify_jeb(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb);
>  
>  /* build.c */
>  int jffs2_do_mount_fs(struct jffs2_sb_info *c);
> diff -Narup Mtd-orig/fs/jffs2/nodemgmt.c mtd/fs/jffs2/nodemgmt.c
> --- Mtd-orig/fs/jffs2/nodemgmt.c	2005-07-20 17:32:28.000000000 +0200
> +++ mtd/fs/jffs2/nodemgmt.c	2005-08-11 15:46:11.000000000 +0200
> @@ -38,9 +38,11 @@
>   *	for the requested allocation.
>   */
>  
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len);
> +static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize,
> +					uint32_t *ofs, uint32_t *len, uint32_t sumsize);
>  
> -int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len, int prio)
> +int jffs2_reserve_space(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs,
> +			uint32_t *len, int prio, uint32_t sumsize)
>  {
>  	int ret = -EAGAIN;
>  	int blocksneeded = c->resv_blocks_write;
> @@ -129,7 +131,7 @@ int jffs2_reserve_space(struct jffs2_sb_
>  			spin_lock(&c->erase_completion_lock);
>  		}
>  
> -		ret = jffs2_do_reserve_space(c, minsize, ofs, len);
> +		ret = jffs2_do_reserve_space(c, minsize, ofs, len, sumsize);
>  		if (ret) {
>  			D1(printk(KERN_DEBUG "jffs2_reserve_space: ret is %d\n", ret));
>  		}
> @@ -140,7 +142,8 @@ int jffs2_reserve_space(struct jffs2_sb_
>  	return ret;
>  }
>  
> -int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs, uint32_t *len)
> +int jffs2_reserve_space_gc(struct jffs2_sb_info *c, uint32_t minsize, uint32_t *ofs,
> +			uint32_t *len, uint32_t sumsize)
>  {
>  	int ret = -EAGAIN;
>  	minsize = PAD(minsize);
> @@ -149,7 +152,7 @@ int jffs2_reserve_space_gc(struct jffs2_
>  
>  	spin_lock(&c->erase_completion_lock);
>  	while(ret == -EAGAIN) {
> -		ret = jffs2_do_reserve_space(c, minsize, ofs, len);
> +		ret = jffs2_do_reserve_space(c, minsize, ofs, len, sumsize);
>  		if (ret) {
>  		        D1(printk(KERN_DEBUG "jffs2_reserve_space_gc: looping, ret is %d\n", ret));
>  		}
> @@ -158,105 +161,168 @@ int jffs2_reserve_space_gc(struct jffs2_
>  	return ret;
>  }
>  
> -/* Called with alloc sem _and_ erase_completion_lock */
> -static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len)
> +static void jffs2_close_nextblock(struct jffs2_sb_info *c, struct jffs2_eraseblock **jeb)
>  {
> -	struct jffs2_eraseblock *jeb = c->nextblock;
> -	
> - restart:
> -	if (jeb && minsize > jeb->free_size) {
> -		/* Skip the end of this block and file it as having some dirty space */
> -		/* If there's a pending write to it, flush now */
> -		if (jffs2_wbuf_dirty(c)) {
> +
> +	/* Check, if we have a dirty block now, or if it was dirty already */
> +	if (ISDIRTY ((*jeb)->wasted_size + (*jeb)->dirty_size)) {
> +		c->dirty_size += (*jeb)->wasted_size;
> +		c->wasted_size -= (*jeb)->wasted_size;
> +		(*jeb)->dirty_size += (*jeb)->wasted_size;
> +		(*jeb)->wasted_size = 0;
> +		if (VERYDIRTY(c, (*jeb)->dirty_size)) {
> +			D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to very_dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +			  (*jeb)->offset, (*jeb)->free_size, (*jeb)->dirty_size, (*jeb)->used_size));
> +			list_add_tail(&(*jeb)->list, &c->very_dirty_list);
> +		} else {
> +			D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +			  (*jeb)->offset, (*jeb)->free_size, (*jeb)->dirty_size, (*jeb)->used_size));
> +			list_add_tail(&(*jeb)->list, &c->dirty_list);
> +		}
> +	} else { 
> +		D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to clean_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> +		  (*jeb)->offset, (*jeb)->free_size, (*jeb)->dirty_size, (*jeb)->used_size));
> +		list_add_tail(&(*jeb)->list, &c->clean_list);
> +	}
> +	c->nextblock = *jeb = NULL;
> +}
> +
> +
> +static int jffs2_find_nextblock(struct jffs2_sb_info *c)
> +{
> +	struct list_head *next;
> +	/* Take the next block off the 'free' list */
> +
> +	if (list_empty(&c->free_list)) {
> +
> +		if (!c->nr_erasing_blocks &&
> +			!list_empty(&c->erasable_list)) {
> +			struct jffs2_eraseblock *ejeb;
> +
> +			ejeb = list_entry(c->erasable_list.next, struct jffs2_eraseblock, list);
> +			list_del(&ejeb->list);
> +			list_add_tail(&ejeb->list, &c->erase_pending_list);
> +			c->nr_erasing_blocks++;
> +			jffs2_erase_pending_trigger(c);
> +			D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Triggering erase of erasable block at 0x%08x\n",
> +				  ejeb->offset));
> +		}
> +
> +		if (!c->nr_erasing_blocks &&
> +			!list_empty(&c->erasable_pending_wbuf_list)) {
> +			D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));
> +			/* c->nextblock is NULL, no update to c->nextblock allowed */			    
>  			spin_unlock(&c->erase_completion_lock);
> -			D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));			    
>  			jffs2_flush_wbuf_pad(c);
>  			spin_lock(&c->erase_completion_lock);
> -			jeb = c->nextblock;
> -			goto restart;
> +			/* Have another go. It'll be on the erasable_list now */
> +			return -EAGAIN;
>  		}
> -		c->wasted_size += jeb->free_size;
> -		c->free_size -= jeb->free_size;
> -		jeb->wasted_size += jeb->free_size;
> -		jeb->free_size = 0;
> -		
> -		/* Check, if we have a dirty block now, or if it was dirty already */
> -		if (ISDIRTY (jeb->wasted_size + jeb->dirty_size)) {
> -			c->dirty_size += jeb->wasted_size;
> -			c->wasted_size -= jeb->wasted_size;
> -			jeb->dirty_size += jeb->wasted_size;
> -			jeb->wasted_size = 0;
> -			if (VERYDIRTY(c, jeb->dirty_size)) {
> -				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to very_dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> -				  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -				list_add_tail(&jeb->list, &c->very_dirty_list);
> -			} else {
> -				D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to dirty_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> -				  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -				list_add_tail(&jeb->list, &c->dirty_list);
> -			}
> -		} else { 
> -			D1(printk(KERN_DEBUG "Adding full erase block at 0x%08x to clean_list (free 0x%08x, dirty 0x%08x, used 0x%08x\n",
> -			  jeb->offset, jeb->free_size, jeb->dirty_size, jeb->used_size));
> -			list_add_tail(&jeb->list, &c->clean_list);
> +
> +		if (!c->nr_erasing_blocks) {
> +			/* Ouch. We're in GC, or we wouldn't have got here.
> +			   And there's no space left. At all. */
> +			printk(KERN_CRIT "Argh. No free space left for GC. nr_erasing_blocks is %d. nr_free_blocks is %d. (erasableempty: %s, erasingempty: %s, erasependingempty: %s)\n", 
> +				   c->nr_erasing_blocks, c->nr_free_blocks, list_empty(&c->erasable_list)?"yes":"no", 
> +				   list_empty(&c->erasing_list)?"yes":"no", list_empty(&c->erase_pending_list)?"yes":"no");
> +			return -ENOSPC;
>  		}
> -		c->nextblock = jeb = NULL;
> +
> +		spin_unlock(&c->erase_completion_lock);
> +		/* Don't wait for it; just erase one right now */
> +		jffs2_erase_pending_blocks(c, 1);
> +		spin_lock(&c->erase_completion_lock);
> +
> +		/* An erase may have failed, decreasing the
> +		   amount of free space available. So we must
> +		   restart from the beginning */
> +		return -EAGAIN;
>  	}
> -	
> -	if (!jeb) {
> -		struct list_head *next;
> -		/* Take the next block off the 'free' list */
>  
> -		if (list_empty(&c->free_list)) {
> +	next = c->free_list.next;
> +	list_del(next);
> +	c->nextblock = list_entry(next, struct jffs2_eraseblock, list);
> +	c->nr_free_blocks--;
>  
> -			if (!c->nr_erasing_blocks && 
> -			    !list_empty(&c->erasable_list)) {
> -				struct jffs2_eraseblock *ejeb;
> -
> -				ejeb = list_entry(c->erasable_list.next, struct jffs2_eraseblock, list);
> -				list_del(&ejeb->list);
> -				list_add_tail(&ejeb->list, &c->erase_pending_list);
> -				c->nr_erasing_blocks++;
> -				jffs2_erase_pending_trigger(c);
> -				D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Triggering erase of erasable block at 0x%08x\n",
> -					  ejeb->offset));
> +	return 0;
> +}
> +
> +/* Called with alloc sem _and_ erase_completion_lock */
> +static int jffs2_do_reserve_space(struct jffs2_sb_info *c,  uint32_t minsize, uint32_t *ofs, uint32_t *len, uint32_t sumsize)
> +{
> +	struct jffs2_eraseblock *jeb = c->nextblock;
> +	uint32_t nofree_size;
> +	int ret;
> +
> + restart:
> +	nofree_size = 0;
> +
> +	if (jffs2_sum_active() && (sumsize != JFFS2_SUMMARY_NOSUM_SIZE)) {
> +							/* NOSUM_SIZE means not to generate summary*/
> +
> +		if (jeb) {
> +			if ((ret = jffs2_sum_care_sum_collected(jeb))) return ret;
> +			nofree_size = PAD(sumsize + jeb->sum_collected->sum_size + JFFS2_SUMMARY_FRAME_SIZE);
> +		}
> +
> +		D1(if (jeb) printk(KERN_DEBUG "JFFS2: minsize %d , jeb->free(%d) ,"
> +						"sum_collected->size(%d) , sumsize(%d)\n", minsize,
> +						jeb->free_size, jeb->sum_collected->sum_size, sumsize));
> +
> +		if (jeb && (PAD(minsize) + PAD(jeb->sum_collected->sum_size + sumsize +
> +					JFFS2_SUMMARY_FRAME_SIZE) > jeb->free_size)) {
> +
> +			D1(printk(KERN_DEBUG "JFFS2: generating summary for 0x%08x.\n", jeb->offset));
> +			if (jeb->sum_collected->sum_size == JFFS2_SUMMARY_NOSUM_SIZE) {
> +				sumsize = JFFS2_SUMMARY_NOSUM_SIZE;
> +				jffs2_sum_clean_collected(jeb);
> +				goto restart;
>  			}
>  
> -			if (!c->nr_erasing_blocks && 
> -			    !list_empty(&c->erasable_pending_wbuf_list)) {
> -				D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));
> -				/* c->nextblock is NULL, no update to c->nextblock allowed */			    
> +			ret = jffs2_sum_write_sumnode(c);
> +
> +			if (ret)
> +				return ret;
> +
> +			if (jeb->sum_collected->sum_size == JFFS2_SUMMARY_NOSUM_SIZE) {
> +				/* jffs2_write_sumnode can't write out the summary information */
> +				sumsize = JFFS2_SUMMARY_NOSUM_SIZE;
> +				jffs2_sum_clean_collected(jeb);
> +				goto restart;
> +			}
> +
> +			jffs2_close_nextblock(c, &jeb);
> +		}
> +	}
> +	else {
> +		if (jeb && minsize > jeb->free_size) {
> +			/* Skip the end of this block and file it as having some dirty space */
> +			/* If there's a pending write to it, flush now */
> +
> +			if (jffs2_wbuf_dirty(c)) {
>  				spin_unlock(&c->erase_completion_lock);
> +				D1(printk(KERN_DEBUG "jffs2_do_reserve_space: Flushing write buffer\n"));
>  				jffs2_flush_wbuf_pad(c);
>  				spin_lock(&c->erase_completion_lock);
> -				/* Have another go. It'll be on the erasable_list now */
> -				return -EAGAIN;
> -			}
> -
> -			if (!c->nr_erasing_blocks) {
> -				/* Ouch. We're in GC, or we wouldn't have got here.
> -				   And there's no space left. At all. */
> -				printk(KERN_CRIT "Argh. No free space left for GC. nr_erasing_blocks is %d. nr_free_blocks is %d. (erasableempty: %s, erasingempty: %s, erasependingempty: %s)\n", 
> -				       c->nr_erasing_blocks, c->nr_free_blocks, list_empty(&c->erasable_list)?"yes":"no", 
> -				       list_empty(&c->erasing_list)?"yes":"no", list_empty(&c->erase_pending_list)?"yes":"no");
> -				return -ENOSPC;
> +				jeb = c->nextblock;
> +				goto restart;
>  			}
>  
> -			spin_unlock(&c->erase_completion_lock);
> -			/* Don't wait for it; just erase one right now */
> -			jffs2_erase_pending_blocks(c, 1);
> -			spin_lock(&c->erase_completion_lock);
> +			c->wasted_size += jeb->free_size;
> +			c->free_size -= jeb->free_size;
> +			jeb->wasted_size += jeb->free_size;
> +			jeb->free_size = 0;
>  
> -			/* An erase may have failed, decreasing the
> -			   amount of free space available. So we must
> -			   restart from the beginning */
> -			return -EAGAIN;
> +			jffs2_close_nextblock(c, &jeb);
>  		}
> +	}
> +	if (!jeb) {
> +
> +		ret = jffs2_find_nextblock(c);
> +		if (ret)
> +			return ret;
>  
> -		next = c->free_list.next;
> -		list_del(next);
> -		c->nextblock = jeb = list_entry(next, struct jffs2_eraseblock, list);
> -		c->nr_free_blocks--;
> +		jeb = c->nextblock;
>  
>  		if (jeb->free_size != c->sector_size - c->cleanmarker_size) {
>  			printk(KERN_WARNING "Eep. Block 0x%08x taken from free_list had free_size of 0x%08x!!\n", jeb->offset, jeb->free_size);
> @@ -266,7 +332,7 @@ static int jffs2_do_reserve_space(struct
>  	/* OK, jeb (==c->nextblock) is now pointing at a block which definitely has
>  	   enough space */
>  	*ofs = jeb->offset + (c->sector_size - jeb->free_size);
> -	*len = jeb->free_size;
> +	*len = jeb->free_size - nofree_size;
>  
>  	if (c->cleanmarker_size && jeb->used_size == c->cleanmarker_size &&
>  	    !jeb->first_node->next_in_ino) {
> diff -Narup Mtd-orig/fs/jffs2/os-linux.h mtd/fs/jffs2/os-linux.h
> --- Mtd-orig/fs/jffs2/os-linux.h	2005-08-06 06:51:30.000000000 +0200
> +++ mtd/fs/jffs2/os-linux.h	2005-08-11 15:17:41.000000000 +0200
> @@ -67,12 +67,18 @@ static inline void jffs2_init_inode_info
>  
>  #ifndef CONFIG_JFFS2_FS_WRITEBUFFER
>  #define SECTOR_ADDR(x) ( ((unsigned long)(x) & ~(c->sector_size-1)) )
> +
> +#ifdef CONFIG_JFFS2_SUMMARY
> +#define jffs2_can_mark_obsolete(c) (0)
> +#else
>  #define jffs2_can_mark_obsolete(c) (1)
> +#endif
> +
>  #define jffs2_is_writebuffered(c) (0)
>  #define jffs2_cleanmarker_oob(c) (0)
>  #define jffs2_write_nand_cleanmarker(c,jeb) (-EIO)
>  
> -#define jffs2_flash_write(c, ofs, len, retlen, buf) ((c)->mtd->write((c)->mtd, ofs, len, retlen, buf))
> +#define jffs2_flash_write(c, ofs, len, retlen, buf) jffs2_flash_direct_write(c, ofs, len, retlen, buf)
>  #define jffs2_flash_read(c, ofs, len, retlen, buf) ((c)->mtd->read((c)->mtd, ofs, len, retlen, buf))
>  #define jffs2_flush_wbuf_pad(c) ({ (void)(c), 0; })
>  #define jffs2_flush_wbuf_gc(c, i) ({ (void)(c), (void) i, 0; })
> @@ -97,9 +103,15 @@ static inline void jffs2_init_inode_info
>  
>  #define jffs2_is_writebuffered(c) (c->wbuf != NULL)
>  #define SECTOR_ADDR(x) ( ((unsigned long)(x) / (unsigned long)(c->sector_size)) * c->sector_size )
> +
> +#ifdef CONFIG_JFFS2_SUMMARY
> +#define jffs2_can_mark_obsolete(c) (0)
> +#else
>  #define jffs2_can_mark_obsolete(c) \
>    ((c->mtd->type == MTD_NORFLASH && !(c->mtd->flags & (MTD_ECC|MTD_PROGRAM_REGIONS))) || \
>     c->mtd->type == MTD_RAM)
> +#endif
> +
>  #define jffs2_cleanmarker_oob(c) (c->mtd->type == MTD_NANDFLASH)
>  
>  #define jffs2_flash_write_oob(c, ofs, len, retlen, buf) ((c)->mtd->write_oob((c)->mtd, ofs, len, retlen, buf))
> @@ -192,7 +204,8 @@ void jffs2_flash_cleanup(struct jffs2_sb
>  /* writev.c */
>  int jffs2_flash_direct_writev(struct jffs2_sb_info *c, const struct kvec *vecs, 
>  		       unsigned long count, loff_t to, size_t *retlen);
> -
> +int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t ofs, size_t len,
> +			size_t *retlen, const u_char *buf);
>  
>  #endif /* __JFFS2_OS_LINUX_H__ */
>  
> diff -Narup Mtd-orig/fs/jffs2/scan.c mtd/fs/jffs2/scan.c
> --- Mtd-orig/fs/jffs2/scan.c	2005-07-20 17:32:28.000000000 +0200
> +++ mtd/fs/jffs2/scan.c	2005-08-11 16:37:16.000000000 +0200
> @@ -18,22 +18,10 @@
>  #include <linux/crc32.h>
>  #include <linux/compiler.h>
>  #include "nodelist.h"
> +#include "summary.h"
>  
>  #define DEFAULT_EMPTY_SCAN_SIZE 1024
>  
> -#define DIRTY_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->dirty_size += _x; \
> -		jeb->free_size -= _x ; jeb->dirty_size += _x; \
> -		}while(0)
> -#define USED_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->used_size += _x; \
> -		jeb->free_size -= _x ; jeb->used_size += _x; \
> -		}while(0)
> -#define UNCHECKED_SPACE(x) do { typeof(x) _x = (x); \
> -		c->free_size -= _x; c->unchecked_size += _x; \
> -		jeb->free_size -= _x ; jeb->unchecked_size += _x; \
> -		}while(0)
> -
>  #define noisy_printk(noise, args...) do { \
>  	if (*(noise)) { \
>  		printk(KERN_NOTICE args); \
> @@ -58,13 +46,6 @@ static int jffs2_scan_inode_node(struct 
>  static int jffs2_scan_dirent_node(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>  				 struct jffs2_raw_dirent *rd, uint32_t ofs);
>  
> -#define BLK_STATE_ALLFF		0
> -#define BLK_STATE_CLEAN		1
> -#define BLK_STATE_PARTDIRTY	2
> -#define BLK_STATE_CLEANMARKER	3
> -#define BLK_STATE_ALLDIRTY	4
> -#define BLK_STATE_BADBLOCK	5
> -
>  static inline int min_free(struct jffs2_sb_info *c)
>  {
>  	uint32_t min = 2 * sizeof(struct jffs2_raw_inode);
> @@ -265,7 +246,7 @@ int jffs2_scan_medium(struct jffs2_sb_in
>  	return ret;
>  }
>  
> -static int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
> +int jffs2_fill_scan_buf (struct jffs2_sb_info *c, unsigned char *buf,
>  				uint32_t ofs, uint32_t len)
>  {
>  	int ret;
> @@ -286,14 +267,36 @@ static int jffs2_fill_scan_buf (struct j
>  	return 0;
>  }
>  
> +int jffs2_scan_classify_jeb(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
> +{
> +	if ((jeb->used_size + jeb->unchecked_size) == PAD(c->cleanmarker_size) && !jeb->dirty_size
> +		&& (!jeb->first_node || !jeb->first_node->next_phys) )
> +		return BLK_STATE_CLEANMARKER;
> +
> +	/* move blocks with max 4 byte dirty space to cleanlist */
> +	else if (!ISDIRTY(c->sector_size - (jeb->used_size + jeb->unchecked_size))) {
> +		c->dirty_size -= jeb->dirty_size;
> +		c->wasted_size += jeb->dirty_size;
> +		jeb->wasted_size += jeb->dirty_size;
> +		jeb->dirty_size = 0;
> +		return BLK_STATE_CLEAN;
> +	} else if (jeb->used_size || jeb->unchecked_size)
> +		return BLK_STATE_PARTDIRTY;
> +	else
> +		return BLK_STATE_ALLDIRTY;
> +}
> +
>  static int jffs2_scan_eraseblock (struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
>  				  unsigned char *buf, uint32_t buf_size) {
>  	struct jffs2_unknown_node *node;
>  	struct jffs2_unknown_node crcnode;
> +	struct jffs2_sum_marker *sm;
>  	uint32_t ofs, prevofs;
>  	uint32_t hdr_crc, buf_ofs, buf_len;
>  	int err;
>  	int noise = 0;
> +
> +
>  #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>  	int cleanmarkerfound = 0;
>  #endif
> @@ -319,10 +322,52 @@ static int jffs2_scan_eraseblock (struct
>  		}
>  	}
>  #endif
> +
> +	if (jffs2_sum_active()) {
> +		sm = (struct jffs2_sum_marker *)kmalloc(sizeof(struct jffs2_sum_marker), GFP_KERNEL);

Remove cast.

> +		if (!sm) {
> +			return -ENOMEM;
> +		}
> +
> +		err = jffs2_fill_scan_buf(c, (unsigned char *) sm, jeb->offset + c->sector_size -
> +					sizeof(struct jffs2_sum_marker), sizeof(struct jffs2_sum_marker));
> +
> +		if (err) {
> +			kfree(sm);
> +			return err;
> +		}
> +
> +		if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
> +
> +			if(je32_to_cpu(sm->erase_size) == c->sector_size) {
> +				err = jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);
> +
> +				if (err) {
> +					kfree(sm);
> +					return err;
> +				}
> +			}
> +			printk(KERN_WARNING "FS erase_block_size != JFFS2 erase_block_size => skipping summary information\n");
> +		}
> +
> +		kfree(sm);
> +
> +		ofs = jeb->offset;
> +		prevofs = jeb->offset - 1;
> +	}
> +
>  	buf_ofs = jeb->offset;
>  
>  	if (!buf_size) {
>  		buf_len = c->sector_size;
> +
> +		if (jffs2_sum_active()) {
> +			/* must reread because of summary test */
> +			err = jffs2_fill_scan_buf(c, buf, buf_ofs, buf_len);
> +			if (err)
> +				return err;
> +		}
> +
>  	} else {
>  		buf_len = EMPTY_SCAN_SIZE(c->sector_size);
>  		err = jffs2_fill_scan_buf(c, buf, buf_ofs, buf_len);
> @@ -367,6 +412,8 @@ static int jffs2_scan_eraseblock (struct
>  
>  	noise = 10;
>  
> +	D1(printk(KERN_DEBUG "JFFS2: no summary found in jeb 0x%08x. Apply original scan.\n",jeb->offset));
> +
>  scan_more:	
>  	while(ofs < jeb->offset + c->sector_size) {
>  
> @@ -582,6 +629,8 @@ scan_more:	
>  			break;
>  
>  		case JFFS2_NODETYPE_PADDING:
> +			if (jffs2_sum_active())
> +				jffs2_sum_add_padding_mem(jeb,je32_to_cpu(node->totlen));
>  			DIRTY_SPACE(PAD(je32_to_cpu(node->totlen)));
>  			ofs += PAD(je32_to_cpu(node->totlen));
>  			break;
> @@ -616,6 +665,20 @@ scan_more:	
>  		}
>  	}
>  
> +	if (jffs2_sum_active()) {
> +		if (jeb->sum_collected) {
> +			if (PAD(jeb->sum_collected->sum_size + JFFS2_SUMMARY_FRAME_SIZE) > jeb->free_size) {
> +				D1(printk(KERN_WARNING "JFFS2 SUMMARY: There is not enough space for "
> +					"summary information, freeing up summary info!\n"));
> +				jffs2_sum_clean_collected(jeb);
> +				/* don't try to write out summary for this node */
> +				jeb->sum_collected->sum_size = JFFS2_SUMMARY_NOSUM_SIZE;
> +			}
> +		}
> +		else {
> +			D1(printk(KERN_WARNING "JFFS2 SUMMARY: Empty summary info found\n"));
> +		}
> +	}
>  
>  	D1(printk(KERN_DEBUG "Block at 0x%08x: free 0x%08x, dirty 0x%08x, unchecked 0x%08x, used 0x%08x\n", jeb->offset, 
>  		  jeb->free_size, jeb->dirty_size, jeb->unchecked_size, jeb->used_size));
> @@ -628,24 +691,10 @@ scan_more:	
>  		jeb->wasted_size = 0;
>  	}
>  
> -	if ((jeb->used_size + jeb->unchecked_size) == PAD(c->cleanmarker_size) && !jeb->dirty_size 
> -		&& (!jeb->first_node || !jeb->first_node->next_phys) )
> -		return BLK_STATE_CLEANMARKER;
> -		
> -	/* move blocks with max 4 byte dirty space to cleanlist */	
> -	else if (!ISDIRTY(c->sector_size - (jeb->used_size + jeb->unchecked_size))) {
> -		c->dirty_size -= jeb->dirty_size;
> -		c->wasted_size += jeb->dirty_size; 
> -		jeb->wasted_size += jeb->dirty_size;
> -		jeb->dirty_size = 0;
> -		return BLK_STATE_CLEAN;
> -	} else if (jeb->used_size || jeb->unchecked_size)
> -		return BLK_STATE_PARTDIRTY;
> -	else
> -		return BLK_STATE_ALLDIRTY;
> +	return jffs2_scan_classify_jeb(c, jeb);
>  }
>  
> -static struct jffs2_inode_cache *jffs2_scan_make_ino_cache(struct jffs2_sb_info *c, uint32_t ino)
> +struct jffs2_inode_cache *jffs2_scan_make_ino_cache(struct jffs2_sb_info *c, uint32_t ino)
>  {
>  	struct jffs2_inode_cache *ic;
>  
> @@ -739,6 +788,10 @@ static int jffs2_scan_inode_node(struct 
>  	pseudo_random += je32_to_cpu(ri->version);
>  
>  	UNCHECKED_SPACE(PAD(je32_to_cpu(ri->totlen)));
> +
> +	if (jffs2_sum_active())
> +		jffs2_sum_add_inode_mem(jeb, ri, ofs);
> +
>  	return 0;
>  }
>  
> @@ -790,6 +843,7 @@ static int jffs2_scan_dirent_node(struct
>  		printk(KERN_NOTICE "jffs2_scan_dirent_node(): allocation of node reference failed\n");
>  		return -ENOMEM;
>  	}
> +
>  	ic = jffs2_scan_make_ino_cache(c, je32_to_cpu(rd->pino));
>  	if (!ic) {
>  		jffs2_free_full_dirent(fd);
> @@ -817,6 +871,9 @@ static int jffs2_scan_dirent_node(struct
>  	USED_SPACE(PAD(je32_to_cpu(rd->totlen)));
>  	jffs2_add_fd_to_list(c, fd, &ic->scan_dents);
>  
> +	if (jffs2_sum_active())
> +		jffs2_sum_add_dirent_mem(jeb,rd,ofs);
> +
>  	return 0;
>  }
>  
> diff -Narup Mtd-orig/fs/jffs2/summary.c mtd/fs/jffs2/summary.c
> --- Mtd-orig/fs/jffs2/summary.c	1970-01-01 01:00:00.000000000 +0100
> +++ mtd/fs/jffs2/summary.c	2005-08-11 16:13:06.000000000 +0200
> @@ -0,0 +1,732 @@
> +/*
> + * JFFS2 -- Journalling Flash File System, Version 2.
> + *
> + * Copyright (C) 2004  Ferenc Havasi <havasi@inf.u-szeged.hu>,
> + *                     Zoltan Sogor <weth@inf.u-szeged.hu>,
> + *                     Patrik Kluba <pajko@halom.u-szeged.hu>,
> + *                     University of Szeged, Hungary
> + *
> + * For licensing information, see the file 'LICENCE' in this directory.
> + *
> + * $Id$
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/pagemap.h>
> +#include <linux/crc32.h>
> +#include <linux/compiler.h>
> +#include <linux/vmalloc.h>
> +#include "nodelist.h"
> +
> +int jffs2_sum_init(struct jffs2_sb_info *c)
> +{
> +	c->summary_buf = (jint32_t *) vmalloc(c->sector_size);

Remove cast.

> +	if (!c->summary_buf) {
> +		printk(KERN_WARNING "JFFS2: can't allocate memory to dump summary information!\n");
> +		return 1;
> +	}
> +	return 0;
> +}
> +
> +void jffs2_sum_exit(struct jffs2_sb_info *c) 
> +{
> +	if (c->summary_buf) {
> +		vfree(c->summary_buf);
> +		c->summary_buf = NULL;
> +	}

Remove condition.  vfree() already checks for NULL pointers.

> +}
> +
> +int jffs2_sum_care_sum_collected(struct jffs2_eraseblock *jeb)
> +{
> +	if (!jeb->sum_collected) {
> +		jeb->sum_collected = (struct jffs2_sum_info *) kmalloc(sizeof(struct jffs2_sum_info), GFP_KERNEL);

Remove cast.

> +		if (!jeb->sum_collected)
> +			return -ENOMEM;
> +
> +		jeb->sum_collected->sum_list = NULL;
> +		jeb->sum_collected->sum_num = 0;
> +		jeb->sum_collected->sum_size = 0;
> +		jeb->sum_collected->sum_padded = 0;
> +	}
> +	return 0;
> +}
> +
> +static int jffs2_sum_add_mem(struct jffs2_eraseblock *jeb, union jffs2_sum_mem *item)
> +{
> +
> +	union jffs2_sum_mem *walk;
> +	int ret;
> +
> +	if ((ret=jffs2_sum_care_sum_collected(jeb))) return ret;

Condition and conditional code on the same line are only obfuscating
the code.  Unless you have to hide something, put them onto seperate
lines.

I personally also dislike having assignments inside the condition.
Gcc is right to warn about it, but the suggestion to use double
brackets doesn't help.

Ergo:

	ret = jffs2_sum_care_sum_collected(jeb);
	if (ret)
		return ret;

> +	if (!jeb->sum_collected->sum_list) {
> +		jeb->sum_collected->sum_list = (union jffs2_sum_mem *) item;
> +	}
> +	else {
> +		walk = jeb->sum_collected->sum_list;
> +
> +		while (walk->u.next) {
> +			walk = walk->u.next;
> +		}
> +		walk->u.next = (union jffs2_sum_mem *) item;
> +	}

Your hand-crafted list has runtime of O(n).  Why don't you just use
the stuff in <linux/list.h>, which is O(1)?

> +	switch (je16_to_cpu(item->u.nodetype)) {
> +		case JFFS2_NODETYPE_INODE:
> +			jeb->sum_collected->sum_size += JFFS2_SUMMARY_INODE_SIZE;
> +			jeb->sum_collected->sum_num++;
> +			break;
> +		case JFFS2_NODETYPE_DIRENT:
> +			jeb->sum_collected->sum_size += JFFS2_SUMMARY_DIRENT_SIZE(item->d.nsize);
> +			jeb->sum_collected->sum_num++;
> +			break;
> +		default:
> +			printk(KERN_WARNING "__jffs2_add_sum_mem(): UNKNOWN node type %d\n", je16_to_cpu(item->u.nodetype));
> +			return 1;
> +	}
> +	return 0;
> +}
> +
> +void jffs2_sum_clean_all_info(struct jffs2_sb_info *c)
> +{
> +	int i;
> +
> +	for (i=0; i<c->nr_blocks; i++) {
> +		struct jffs2_eraseblock *jeb = &c->blocks[i];
> +
> +		jffs2_sum_clean_collected(jeb);
> +		kfree(jeb->sum_collected);
> +		jeb->sum_collected = NULL;
> +	}
> +}
> +
> +/* These 3 functions are called from scan.c to collect summary info for not closed jeb */
> +
> +int jffs2_sum_add_padding_mem(struct jffs2_eraseblock *jeb, uint32_t size)
> +{
> +	int ret;
> +
> +	if ((ret=jffs2_sum_care_sum_collected(jeb))) return ret;
> +	jeb->sum_collected->sum_padded += size;
> +	return 0;
> +}
> +
> +int jffs2_sum_add_inode_mem(struct jffs2_eraseblock *jeb, struct jffs2_raw_inode *ri, uint32_t ofs)
> +{
> +
> +	struct jffs2_sum_inode_mem *temp = (struct jffs2_sum_inode_mem *) kmalloc(sizeof(struct jffs2_sum_inode_mem), GFP_KERNEL);

Remove cast.

> +	if (!temp)
> +		return -ENOMEM;
> +
> +	ofs -= jeb->offset;
> +
> +	temp->nodetype = ri->nodetype;
> +	temp->inode = ri->ino;
> +	temp->version = ri->version;
> +	temp->offset = cpu_to_je32(ofs); 
> +	temp->totlen = ri->totlen;
> +	temp->next = NULL;
> +
> +	return jffs2_sum_add_mem(jeb, (union jffs2_sum_mem *)temp);
> +}
> +
> +int jffs2_sum_add_dirent_mem(struct jffs2_eraseblock *jeb, struct jffs2_raw_dirent *rd, uint32_t ofs)
> +{
> +
> +	struct jffs2_sum_dirent_mem *temp = (struct jffs2_sum_dirent_mem *)

Remove cast.

> +
> +	if (!temp)
> +		return -ENOMEM;
> +
> +	ofs -= jeb->offset;
> +
> +	temp->nodetype = rd->nodetype;
> +	temp->totlen = rd->totlen;
> +	temp->offset = cpu_to_je32(ofs);
> +	temp->pino = rd->pino;
> +	temp->version = rd->version;
> +	temp->ino = rd->ino;
> +	temp->nsize = rd->nsize;
> +	temp->type = rd->type;
> +	temp->next = NULL;
> +
> +	memcpy(temp->name, rd->name, rd->nsize);
> +
> +	return jffs2_sum_add_mem(jeb, (union jffs2_sum_mem *)temp);
> +}
> +
> +/* Cleanup every collected summary information */
> +
> +void jffs2_sum_clean_collected(struct jffs2_eraseblock *jeb)
> +{
> +
> +	union jffs2_sum_mem *temp;
> +
> +	if (jeb && jeb->sum_collected) {
> +
> +		while (jeb->sum_collected->sum_list) {
> +			temp = jeb->sum_collected->sum_list;
> +			jeb->sum_collected->sum_list = jeb->sum_collected->sum_list->u.next;
> +			kfree(temp);
> +			jeb->sum_collected->sum_num--;
> +		}
> +
> +		if (jeb->sum_collected->sum_num != 0) {
> +			printk(KERN_WARNING "Ooops, something wrong happened! sum_num != 0, but sum_list = null ???");
> +			jeb->sum_collected->sum_num = 0;
> +		}
> +	}
> +}
> +
> +void jffs2_sum_reset_collected(struct jffs2_eraseblock *jeb)
> +{
> +	if (jeb->sum_collected) {
> +		jffs2_sum_clean_collected(jeb);
> +		jeb->sum_collected->sum_size = 0;
> +		jeb->sum_collected->sum_padded = 0;
> +	}
> +}
> +
> +/* Called from wbuf.c to collect writed node info */
> +
> +int jffs2_sum_add_kvec(struct jffs2_sb_info *c, const struct kvec *invecs, unsigned long count, uint32_t ofs)
> +{
> +	union jffs2_node_union *node;
> +	struct jffs2_eraseblock *jeb;
> +	int ret;
> +
> +	node = (union jffs2_node_union *) invecs[0].iov_base;

Remove cast.

> +	jeb = &c->blocks[ofs / c->sector_size];
> +	ofs -= jeb->offset;
> +
> +	if ((ret=jffs2_sum_care_sum_collected(jeb))) return ret;

You really don't want people to understand this line quickly, do you?
;)

> +	switch (je16_to_cpu(node->u.nodetype)) {
> +		case JFFS2_NODETYPE_INODE : {
> +			struct jffs2_sum_inode_mem *temp = (struct jffs2_sum_inode_mem *)
> +				kmalloc(sizeof(struct jffs2_sum_inode_mem), GFP_KERNEL);

Remove cast.

> +
> +			if (!temp)
> +				return -ENOMEM;
> +
> +			temp->nodetype = node->i.nodetype;
> +			temp->inode = node->i.ino;
> +			temp->version = node->i.version;
> +			temp->offset = cpu_to_je32(ofs);
> +			temp->totlen = node->i.totlen;
> +			temp->next = NULL;
> +
> +			return jffs2_sum_add_mem(jeb, (union jffs2_sum_mem *)temp);
> +
> +			break;

Break after return has no effect.  Just remove it.

> +		}
> +
> +		case JFFS2_NODETYPE_DIRENT : {
> +			struct jffs2_sum_dirent_mem *temp = (struct jffs2_sum_dirent_mem *)
> +				kmalloc(sizeof(struct jffs2_sum_dirent_mem) + node->d.nsize, GFP_KERNEL);

Return value of kmalloc() is void* for a reason.  It implicitly gets
casted to any pointer type.  So just remove the superfluous explicit
one.

> +			if (!temp)
> +				return -ENOMEM;
> +
> +			temp->nodetype = node->d.nodetype;
> +			temp->totlen = node->d.totlen;
> +			temp->offset = cpu_to_je32(ofs);
> +			temp->pino = node->d.pino;
> +			temp->version = node->d.version;
> +			temp->ino = node->d.ino;
> +			temp->nsize = node->d.nsize;
> +			temp->type = node->d.type;
> +			temp->next = NULL;
> +
> +			switch (count) {
> +
> +				case 1 :
> +					memcpy(temp->name,node->d.name,node->d.nsize);
> +					break;
> +
> +				case 2 :
> +					memcpy(temp->name,invecs[1].iov_base,node->d.nsize);
> +					break;
> +
> +				default :
> +					printk(KERN_WARNING "jffs2_sum_add_kvec(): bad count value \n");
> +					break;
> +			}
> +
> +			return jffs2_sum_add_mem(jeb, (union jffs2_sum_mem *)temp);
> +			break;

Remove.

> +		}
> +
> +		case JFFS2_NODETYPE_PADDING : {
> +			D1(printk(KERN_DEBUG "jffs2_sum_add_kvec(): Node PADDING\n"));
> +			jeb->sum_collected->sum_padded += je32_to_cpu(node->u.totlen);
> +			break;
> +		}
> +
> +		case JFFS2_NODETYPE_CLEANMARKER : {
> +			D1(printk(KERN_DEBUG "jffs2_sum_add_kvec(): Node CLEANMARKER\n"));
> +			break;
> +		}
> +
> +		case JFFS2_NODETYPE_SUMMARY : {
> +			D1(printk(KERN_DEBUG "jffs2_sum_add_kvec(): Node SUMMARY\n"));
> +			break;
> +		}
> +
> +		default : {
> +			printk(KERN_WARNING "jffs2_sum_add_kvec(): Node not supported\n");
> +			BUG();

BUG() should already print out enough information to debug this.  Is
the extra printk() really useful?

> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +/* Process the summary information - called from jffs2_scan_eraseblock() */
> +
> +int jffs2_sum_scan_sumnode(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb, uint32_t ofs, uint32_t *pseudo_random)
> +{
> +
> +	struct jffs2_unknown_node crcnode;
> +	struct jffs2_raw_node_ref *raw;
> +	struct jffs2_raw_node_ref *cache_ref;
> +	struct jffs2_inode_cache *ic;
> +	struct jffs2_full_dirent *fd;
> +
> +	int i, err;
> +	int bad_sum = 0;
> +	int sumsize;
> +	uint32_t ino;
> +	uint32_t crc;
> +	struct jffs2_summary_node *summary;
> +
> +	sumsize = c->sector_size - ofs;
> +	ofs += jeb->offset;
> +
> +	D1(printk(KERN_DEBUG "JFFS2: summary found for 0x%08x at 0x%08x (0x%x bytes)\n", jeb->offset, ofs, sumsize));
> +
> +	summary = (struct jffs2_summary_node *) kmalloc(sumsize, GFP_KERNEL);

Remove cast.

> +	if (!summary) {
> +		return -ENOMEM;
> +	}
> +
> +	err = jffs2_fill_scan_buf(c, (unsigned char *)summary, ofs, sumsize);

jffs2_fill_scan_buf() should be changed to take a void* as buffer
argument.  That's the type of choice for "just a chunk of memory".
And it gets rid of the explicit cast as well.

> +	if (err) {
> +		kfree(summary);
> +		return err;
> +	}
> +
> +	/* OK, now check for node validity and CRC */
> +	crcnode.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
> +	crcnode.nodetype = cpu_to_je16(JFFS2_NODETYPE_SUMMARY);
> +	crcnode.totlen = summary->totlen;
> +	crc = crc32(0, &crcnode, sizeof(crcnode)-4);
> +
> +	if (je32_to_cpu(summary->hdr_crc) != crc) {
> +		D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node header is corrupt (bad CRC or no summary at all)\n"));
> +		bad_sum = 1;
> +	}
> +
> +	if ((!bad_sum) && (je32_to_cpu(summary->totlen) != sumsize)) {
> +		D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node is corrupt (wrong erasesize?)\n"));
> +		bad_sum = 1;
> +	}
> +
> +	crc = crc32(0, summary, sizeof(struct jffs2_summary_node)-8);
> +
> +	if ((!bad_sum) && (je32_to_cpu(summary->node_crc) != crc)) {
> +		D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node is corrupt (bad CRC)\n"));
> +		bad_sum = 1;
> +	}
> +
> +	crc = crc32(0, summary->sum, sumsize - sizeof(struct jffs2_summary_node));
> +
> +	if ((!bad_sum) && (je32_to_cpu(summary->sum_crc) != crc)) {
> +		D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node data is corrupt (bad CRC)\n"));
> +		bad_sum = 1;
> +	}
> +
> +	if (!bad_sum) {
> +
> +		struct jffs2_sum_unknown_flash *sp;
> +		sp = (struct jffs2_sum_unknown_flash *) summary->sum;
> +
> +		if ( je32_to_cpu(summary->cln_mkr) ) {
> +
> +			D1(printk(KERN_DEBUG "Summary : CLEANMARKER node \n"));
> +
> +			if (je32_to_cpu(summary->cln_mkr) != c->cleanmarker_size) {
> +				D1(printk(KERN_DEBUG "CLEANMARKER node has totlen 0x%x != normal 0x%x\n", 
> +					je32_to_cpu(summary->cln_mkr), c->cleanmarker_size));
> +				UNCHECKED_SPACE(PAD(je32_to_cpu(summary->cln_mkr)));
> +			} 
> +			else if (jeb->first_node) {
> +				D1(printk(KERN_DEBUG "CLEANMARKER node not first node in block (0x%08x)\n", jeb->offset));
> +				UNCHECKED_SPACE(PAD(je32_to_cpu(summary->cln_mkr)));
> +			}
> +			else {
> +				struct jffs2_raw_node_ref *marker_ref = jffs2_alloc_raw_node_ref();
> +
> +				if (!marker_ref) {
> +					D1(printk(KERN_NOTICE "Failed to allocate node ref for clean marker\n"));
> +					kfree(summary);
> +					return -ENOMEM;
> +				}
> +
> +				marker_ref->next_in_ino = NULL;
> +				marker_ref->next_phys = NULL;
> +				marker_ref->flash_offset = jeb->offset | REF_NORMAL;
> +				marker_ref->__totlen = je32_to_cpu(summary->cln_mkr);
> +				jeb->first_node = jeb->last_node = marker_ref;
> +
> +				USED_SPACE( PAD(je32_to_cpu(summary->cln_mkr)) );
> +			}
> +		}
> +
> +		if (je32_to_cpu(summary->padded)) {
> +			DIRTY_SPACE(je32_to_cpu(summary->padded));
> +		}
> +
> +		for(i=0; i<je16_to_cpu(summary->sum_num); i++) {
> +			uint8_t *temp8ptr = NULL;
> +			D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Processing summary information %d\n", i));
> +
> +			switch (je16_to_cpu(sp->nodetype)) {
> +				case JFFS2_NODETYPE_INODE : {
> +					struct jffs2_sum_inode_flash *spi;
> +					spi = (struct jffs2_sum_inode_flash *) sp;
> +
> +					ino = je32_to_cpu(spi->inode);
> +					D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Inode at 0x%08x\n", jeb->offset + je32_to_cpu(spi->offset)));
> +					raw = jffs2_alloc_raw_node_ref();
> +					if (!raw) {
> +						printk(KERN_NOTICE "jffs2_scan_eraseblock(): allocation of node reference failed\n");
> +						kfree(summary);
> +						return -ENOMEM;
> +					}
> +
> +					ic = jffs2_scan_make_ino_cache(c, ino);
> +					if (!ic) {
> +						printk(KERN_NOTICE "jffs2_scan_eraseblock(): scan_make_ino_cache failed\n");
> +						jffs2_free_raw_node_ref(raw);
> +						kfree(summary);
> +						return -ENOMEM;
> +					}
> +
> +					raw->flash_offset = (jeb->offset + je32_to_cpu(spi->offset)) | REF_UNCHECKED;
> +					raw->__totlen = PAD(je32_to_cpu(spi->totlen));
> +					raw->next_phys = NULL;
> +					raw->next_in_ino = ic->nodes;
> +
> +					ic->nodes = raw;
> +					if (!jeb->first_node)
> +						jeb->first_node = raw;
> +					if (jeb->last_node)
> +						jeb->last_node->next_phys = raw;
> +					jeb->last_node = raw;
> +					*pseudo_random += je32_to_cpu(spi->version);
> +
> +					UNCHECKED_SPACE(PAD(je32_to_cpu(spi->totlen)));
> +
> +					temp8ptr = (uint8_t *) sp;
> +					temp8ptr += JFFS2_SUMMARY_INODE_SIZE;
> +					sp = (struct jffs2_sum_unknown_flash *) temp8ptr;

Gcc supports arithmetic on void*.  While sizeof(void*) is 0, adding
JFFS2_SUMMARY_INODE_SIZE to a void* has the same effect as adding it
to your temp8ptr.  But it has the big advantage of not requiring any
casts, which only distract readers from the intent.

> +
> +					break;
> +				}
> +
> +				case JFFS2_NODETYPE_DIRENT : {
> +					struct jffs2_sum_dirent_flash *spd;
> +					spd = (struct jffs2_sum_dirent_flash *) sp;
> +
> +					fd = jffs2_alloc_full_dirent(spd->nsize+1);
> +					if (!fd) {
> +						kfree(summary);
> +						return -ENOMEM;
> +					}
> +
> +					memcpy(&fd->name, spd->name, spd->nsize);
> +					fd->name[spd->nsize] = 0;
> +
> +					raw = jffs2_alloc_raw_node_ref();
> +					if (!raw) {
> +						jffs2_free_full_dirent(fd);
> +						printk(KERN_NOTICE "jffs2_scan_dirent_node(): allocation of node reference failed\n");
> +						kfree(summary);
> +						return -ENOMEM;
> +					}
> +
> +					ic = jffs2_scan_make_ino_cache(c, je32_to_cpu(spd->pino));
> +					if (!ic) {
> +						jffs2_free_full_dirent(fd);
> +						jffs2_free_raw_node_ref(raw);
> +						kfree(summary);
> +						return -ENOMEM;
> +					}
> +
> +					raw->__totlen = PAD(je32_to_cpu(spd->totlen));
> +					raw->flash_offset = (jeb->offset + je32_to_cpu(spd->offset)) | REF_PRISTINE;
> +					raw->next_phys = NULL;
> +					raw->next_in_ino = ic->nodes;
> +					ic->nodes = raw;
> +					if (!jeb->first_node)
> +						jeb->first_node = raw;
> +					if (jeb->last_node)
> +						jeb->last_node->next_phys = raw;
> +					jeb->last_node = raw;
> +
> +					fd->raw = raw;
> +					fd->next = NULL;
> +					fd->version = je32_to_cpu(spd->version);
> +					fd->ino = je32_to_cpu(spd->ino);
> +					fd->nhash = full_name_hash(fd->name, spd->nsize);
> +					fd->type = spd->type;
> +					USED_SPACE(PAD(je32_to_cpu(spd->totlen)));
> +					jffs2_add_fd_to_list(c, fd, &ic->scan_dents);
> +
> +					*pseudo_random += je32_to_cpu(spd->version);
> +
> +					temp8ptr = (uint8_t *) sp;
> +					temp8ptr += JFFS2_SUMMARY_DIRENT_SIZE(spd->nsize);
> +					sp = (struct jffs2_sum_unknown_flash *) temp8ptr;

void*

> +					break;
> +				}
> +
> +				default : {
> +					printk(KERN_WARNING "Kernel doesn't support this type of node !!! Exiting");
> +					return -EIO;
> +					break;
> +				}
> +			}
> +		}
> +
> +		kfree(summary);
> +
> +		/* for ACCT_PARANOIA_CHECK */
> +		cache_ref = jffs2_alloc_raw_node_ref();
> +
> +		if (!cache_ref) {
> +			printk(KERN_NOTICE "Failed to allocate node ref for cache\n");
> +			return -ENOMEM;
> +		}
> +
> +		cache_ref->next_in_ino = NULL;
> +		cache_ref->next_phys = NULL;
> +		cache_ref->flash_offset = ofs | REF_NORMAL;
> +		cache_ref->__totlen = sumsize;
> +
> +		if (!jeb->first_node)
> +			jeb->first_node = cache_ref;
> +		if (jeb->last_node)
> +			jeb->last_node->next_phys = cache_ref;
> +		jeb->last_node = cache_ref;
> +
> +		USED_SPACE(sumsize);
> +
> +		jeb->wasted_size += jeb->free_size;
> +		c->wasted_size += jeb->free_size;
> +		c->free_size -= jeb->free_size;
> +		jeb->free_size = 0;
> +		
> +		return jffs2_scan_classify_jeb(c, jeb);
> +	}
> +
> +	return 0;
> +}

Function has >200 lines.  Split it up.

> +/* Write out summary information - called from jffs2_do_reserve_space */
> +
> +int jffs2_sum_write_sumnode(struct jffs2_sb_info *c)
> +{
> +	struct jffs2_summary_node isum;
> +	struct jffs2_raw_node_ref *summary_ref;
> +	union jffs2_sum_mem *temp;
> +	jint32_t offset;
> +	jint32_t *wpage;
> +	uint8_t *tempptr;
> +	int datasize;
> +	int infosize;
> +	int padsize;
> +	size_t retlen;
> +	int ret;
> +	struct jffs2_eraseblock *jeb;
> +	struct kvec vecs[2];
> +	jint32_t magic = cpu_to_je32(JFFS2_SUM_MAGIC);

This one as well.  Just reading the variable definitions is scary
enough to keep me awake at night.  eek.

> +	D2(printk(KERN_DEBUG "jffs2_sum_write_sumnode()\n"));
> +
> +	jeb = c->nextblock;
> +
> +	if (!jeb->sum_collected->sum_num || !jeb->sum_collected->sum_list) {
> +		printk(KERN_WARNING "JFFS2: jffs2_sum_write_sumnode(): empty summary info!!!\n");
> +		BUG();
> +	}
> +
> +	datasize = jeb->sum_collected->sum_size + sizeof(struct jffs2_sum_marker);
> +	infosize = sizeof(struct jffs2_summary_node) + datasize;
> +	padsize = jeb->free_size - infosize;
> +	infosize += padsize; datasize += padsize;
> +	offset = cpu_to_je32(c->sector_size - jeb->free_size);
> +
> +	if (padsize < 0) { // if jeb hasn't got enought free space for summary
                                                      ^
Typo

> +
> +		union jffs2_sum_mem *temp;
> +
> +		while(jeb->sum_collected->sum_list){ //cleanup sum_list
> +			temp = jeb->sum_collected->sum_list;
> +			jeb->sum_collected->sum_list = jeb->sum_collected->sum_list->u.next;
> +			kfree(temp);
> +			jeb->sum_collected->sum_num--;
> +		}
> +
> +		jeb->sum_collected->sum_list = NULL;
> +		jeb->sum_collected->sum_num = 0;
> +		jeb->sum_collected->sum_size = JFFS2_SUMMARY_NOSUM_SIZE; // don't try to write out summary for this node
> +
> +		printk(KERN_WARNING "JFFS2: not enough space for summary, padsize = %d\n",padsize);
> +		return 0;
> +	}
> +
> +	memset(c->summary_buf, 0xff, datasize);
> +	memset(&isum, 0, sizeof(isum));
> +
> +	isum.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
> +	isum.nodetype = cpu_to_je16(JFFS2_NODETYPE_SUMMARY);
> +	isum.totlen = cpu_to_je32(infosize);
> +	isum.hdr_crc = cpu_to_je32(crc32(0, &isum, sizeof(struct jffs2_unknown_node) - 4));
> +	isum.padded = cpu_to_je32(jeb->sum_collected->sum_padded);
> +
> +	if (c->cleanmarker_size) {
> +		isum.cln_mkr = cpu_to_je32(c->cleanmarker_size);
> +	}
> +	else {
> +		isum.cln_mkr = cpu_to_je32(0);
> +	}

What is the difference between those two cases?

> +	isum.sum_num = cpu_to_je16(jeb->sum_collected->sum_num);
> +	wpage = c->summary_buf;
> +
> +	while (jeb->sum_collected->sum_num) { // write sum_data 
                                                 ^^^^^^^^^^^^^^
You could turn this comment into a function name and move all relevant
code into that function.

> +
> +		switch(je16_to_cpu(jeb->sum_collected->sum_list->u.nodetype)){
> +			case JFFS2_NODETYPE_INODE : {
> +				jint16_t *temp16ptr = (jint16_t *)wpage;
> +
> +				*(temp16ptr++) = jeb->sum_collected->sum_list->i.nodetype;
> +				wpage = (jint32_t *) temp16ptr;
> +
> +				*(wpage++) = jeb->sum_collected->sum_list->i.inode;
> +				*(wpage++) = jeb->sum_collected->sum_list->i.version;
> +				*(wpage++) = jeb->sum_collected->sum_list->i.offset;
> +				*(wpage++) = jeb->sum_collected->sum_list->i.totlen;
> +				break;
> +			}
> +
> +			case JFFS2_NODETYPE_DIRENT : {
> +				jint16_t *temp16ptr = (jint16_t *) wpage;
> +				uint8_t *temp8ptr = NULL;
> +
> +				*(temp16ptr++) = jeb->sum_collected->sum_list->d.nodetype;
> +				wpage = (jint32_t *) temp16ptr;
> +
> +				*(wpage++) = jeb->sum_collected->sum_list->d.totlen;
> +				*(wpage++) = jeb->sum_collected->sum_list->d.offset;
> +				*(wpage++) = jeb->sum_collected->sum_list->d.pino;
> +				*(wpage++) = jeb->sum_collected->sum_list->d.version;
> +				*(wpage++) = jeb->sum_collected->sum_list->d.ino;

I'm not sure about this pointer stuff.  It looks complicated and hence
easy to get wrong.  Something simpler would be nice.  But I don't have
a good idea right now.

> +				temp8ptr = (uint8_t *) wpage;
> +				*(temp8ptr++) = jeb->sum_collected->sum_list->d.nsize;
> +				*(temp8ptr++) = jeb->sum_collected->sum_list->d.type;
> +
> +				memcpy(temp8ptr,jeb->sum_collected->sum_list->d.name,jeb->sum_collected->sum_list->d.nsize);
> +				temp8ptr += jeb->sum_collected->sum_list->d.nsize;
> +				wpage = (jint32_t *) temp8ptr;
> +
> +				break;
> +			}
> +
> +			default : {
> +				printk(KERN_WARNING "Unknown node in summary information!!!\n");
> +				BUG();
> +			}
> +		}
> +
> +		temp = jeb->sum_collected->sum_list;
> +		jeb->sum_collected->sum_list = jeb->sum_collected->sum_list->u.next;
> +		kfree(temp);
> +
> +		jeb->sum_collected->sum_num--;
> +	}
> +
> +	jeb->sum_collected->sum_size = 0;
> +	jeb->sum_collected->sum_padded = 0;
> +
> +	tempptr = (uint8_t *) wpage;
> +	tempptr += padsize;
> +	wpage = (jint32_t *) tempptr;
> +
> +	*(wpage++) = offset;
> +	*(wpage++) = cpu_to_je32(c->sector_size);
> +	*(wpage++) = magic;
> +	isum.sum_crc = cpu_to_je32(crc32(0, c->summary_buf, datasize));
> +	isum.node_crc = cpu_to_je32(crc32(0, &isum, sizeof(isum) - 8));
> +
> +	vecs[0].iov_base = &isum;
> +	vecs[0].iov_len = sizeof(isum);
> +	vecs[1].iov_base = c->summary_buf;
> +	vecs[1].iov_len = datasize;
> +
> +	D1(printk(KERN_DEBUG "JFFS2: writing out data to flash to pos : 0x%08x\n",
> +			jeb->offset + c->sector_size - jeb->free_size));
> +
> +	spin_unlock(&c->erase_completion_lock);
> +	ret = jffs2_flash_writev(c, vecs, 2, jeb->offset + c->sector_size - jeb->free_size, &retlen, 0);
> +	spin_lock(&c->erase_completion_lock);
> +
> +
> +	if (ret || (retlen != infosize)) {
> +		printk(KERN_WARNING "JFFS2: write of %zd bytes at 0x%08x failed. returned %d, retlen %zd\n",
> +			infosize, jeb->offset + c->sector_size - jeb->free_size, ret, retlen);
> +
> +		jeb->sum_collected->sum_size = JFFS2_SUMMARY_NOSUM_SIZE;
> +		WASTED_SPACE(infosize);
> +
> +		return 0;
> +	}
> +
> +	/* for ACCT_PARANOIA_CHECK */
> +	spin_unlock(&c->erase_completion_lock);
> +	summary_ref = jffs2_alloc_raw_node_ref();
> +	spin_lock(&c->erase_completion_lock);
> +
> +	if (!summary_ref) {
> +		printk(KERN_NOTICE "Failed to allocate node ref for summary\n");
> +		return -ENOMEM;
> +	}
> +
> +	summary_ref->next_in_ino = NULL;
> +	summary_ref->next_phys = NULL;
> +	summary_ref->flash_offset = (jeb->offset + c->sector_size - jeb->free_size) | REF_NORMAL;
> +	summary_ref->__totlen = infosize;
> +
> +	if (!jeb->first_node)
> +		jeb->first_node = summary_ref;
> +	if (jeb->last_node)
> +		jeb->last_node->next_phys = summary_ref;
> +	jeb->last_node = summary_ref;
> +
> +	USED_SPACE(infosize);
> +
> +	return 0;
> +}
> diff -Narup Mtd-orig/fs/jffs2/summary.h mtd/fs/jffs2/summary.h
> --- Mtd-orig/fs/jffs2/summary.h	1970-01-01 01:00:00.000000000 +0100
> +++ mtd/fs/jffs2/summary.h	2005-08-11 16:21:04.000000000 +0200
> @@ -0,0 +1,172 @@
> +/*
> + * JFFS2 -- Journalling Flash File System, Version 2.
> + *
> + * Copyright (C) 2004  Ferenc Havasi <havasi@inf.u-szeged.hu>,
> + *                     Zoltan Sogor <weth@inf.u-szeged.hu>,
> + *                     Patrik Kluba <pajko@halom.u-szeged.hu>,
> + *                     University of Szeged, Hungary
> + *
> + * For licensing information, see the file 'LICENCE' in this directory.
> + *
> + * $Id$
> + *
> + */
> +
> +#ifndef JFFS2_SUMMARY_H
> +#define JFFS2_SUMMARY_H
> +
> +#include <linux/uio.h>
> +#include <linux/jffs2.h>
> +
> +#define DIRTY_SPACE(x) do { typeof(x) _x = (x); \
> +		c->free_size -= _x; c->dirty_size += _x; \
> +		jeb->free_size -= _x ; jeb->dirty_size += _x; \
> +		}while(0)
> +#define USED_SPACE(x) do { typeof(x) _x = (x); \
> +		c->free_size -= _x; c->used_size += _x; \
> +		jeb->free_size -= _x ; jeb->used_size += _x; \
> +		}while(0)
> +#define WASTED_SPACE(x) do { typeof(x) _x = (x); \
> +		c->free_size -= _x; c->wasted_size += _x; \
> +		jeb->free_size -= _x ; jeb->wasted_size += _x; \
> +		}while(0)
> +#define UNCHECKED_SPACE(x) do { typeof(x) _x = (x); \
> +		c->free_size -= _x; c->unchecked_size += _x; \
> +		jeb->free_size -= _x ; jeb->unchecked_size += _x; \
> +		}while(0)
> +
> +#define BLK_STATE_ALLFF		0
> +#define BLK_STATE_CLEAN		1
> +#define BLK_STATE_PARTDIRTY	2
> +#define BLK_STATE_CLEANMARKER	3
> +#define BLK_STATE_ALLDIRTY	4
> +#define BLK_STATE_BADBLOCK	5
> +
> +#define JFFS2_SUMMARY_NOSUM_SIZE 0xffffffff
> +#define JFFS2_SUMMARY_INODE_SIZE (sizeof(struct jffs2_sum_inode_flash))
> +#define JFFS2_SUMMARY_DIRENT_SIZE(x) (sizeof(struct jffs2_sum_dirent_flash) + (x))
> +
> +struct jffs2_sum_unknown_flash
> +{
> +	jint16_t nodetype;	/* node type */
> +};
> +
> +struct jffs2_sum_inode_flash
> +{
> +	jint16_t nodetype;	/* node type */
> +	jint32_t inode;		/* inode number */
> +	jint32_t version;	/* inode version */
> +	jint32_t offset;	/* offset on jeb */
> +	jint32_t totlen; 	/* record length */
> +} __attribute__((packed));
> +
> +struct jffs2_sum_dirent_flash
> +{
> +	jint16_t nodetype;	/* == JFFS_NODETYPE_DIRENT */
> +	jint32_t totlen;	/* record length */
> +	jint32_t offset;	/* ofset on jeb */
> +	jint32_t pino;		/* parent inode */
> +	jint32_t version;	/* dirent version */
> +	jint32_t ino; 		/* == zero for unlink */
> +	uint8_t nsize;		/* dirent name size */
> +	uint8_t type;		/* dirent type */
> +	uint8_t name[0];	/* dirent name */
> +} __attribute__((packed));
> +
> +union jffs2_sum_flash
> +{
> +	struct jffs2_sum_unknown_flash u;
> +	struct jffs2_sum_inode_flash i;
> +	struct jffs2_sum_dirent_flash d;
> +};
> +
> +/* list version of jffs2_sum_*flash for kernel and sumtool */
> +struct jffs2_sum_unknown_mem
> +{
> +	union jffs2_sum_mem *next;
> +	jint16_t nodetype;	/* node type */
> +};
> +
> +struct jffs2_sum_inode_mem
> +{
> +	union jffs2_sum_mem *next;
> +	jint16_t nodetype;	/* node type */
> +	jint32_t inode;		/* inode number */
> +	jint32_t version;	/* inode version */
> +	jint32_t offset;	/* offset on jeb */
> +	jint32_t totlen; 	/* record length */
> +} __attribute__((packed));
> +
> +struct jffs2_sum_dirent_mem
> +{
> +	union jffs2_sum_mem *next;
> +	jint16_t nodetype;	/* == JFFS_NODETYPE_DIRENT */
> +	jint32_t totlen;	/* record length */
> +	jint32_t offset;	/* ofset on jeb */
> +	jint32_t pino;		/* parent inode */
> +	jint32_t version;	/* dirent version */
> +	jint32_t ino; 		/* == zero for unlink */
> +	uint8_t nsize;		/* dirent name size */
> +	uint8_t type;		/* dirent type */
> +	uint8_t name[0];	/* dirent name */
> +} __attribute__((packed));
> +
> +union jffs2_sum_mem
> +{
> +	struct jffs2_sum_unknown_mem u;
> +	struct jffs2_sum_inode_mem i;
> +	struct jffs2_sum_dirent_mem d;
> +};
> +
> +struct jffs2_sum_info
> +{
> +	uint32_t sum_size;
> +	uint32_t sum_num;
> +	uint32_t sum_padded;
> +	union jffs2_sum_mem *sum_list;
> +};
> +
> +struct jffs2_sum_marker
> +{
> +	jint32_t offset;
> +	jint32_t erase_size;
> +	jint32_t magic;
> +};
> +
> +#define JFFS2_SUMMARY_FRAME_SIZE (sizeof(struct jffs2_summary_node) + sizeof(struct jffs2_sum_marker))
> +
> +int jffs2_sum_init(struct jffs2_sb_info *c);
> +void jffs2_sum_exit(struct jffs2_sb_info *c);
> +void jffs2_sum_clean_all_info(struct jffs2_sb_info *c); /* clean up all summary information in all jeb (umount) */
> +
> +#ifdef CONFIG_JFFS2_SUMMARY		// SUMMARY SUPPORT ENABLED
> +#define jffs2_sum_active() (1)
> +void jffs2_sum_reset_collected(struct jffs2_eraseblock *jeb);
> +int jffs2_sum_add_kvec(struct jffs2_sb_info *c, const struct kvec *invecs,
> +			unsigned long count,  uint32_t to);
> +int jffs2_sum_care_sum_collected(struct jffs2_eraseblock *jeb);
> +void jffs2_sum_clean_collected(struct jffs2_eraseblock *jeb);
> +	
> +int jffs2_sum_write_sumnode(struct jffs2_sb_info *c);
> +int jffs2_sum_add_padding_mem(struct jffs2_eraseblock *jeb, uint32_t size);
> +int jffs2_sum_add_inode_mem(struct jffs2_eraseblock *jeb, struct jffs2_raw_inode *ri, uint32_t ofs);
> +int jffs2_sum_add_dirent_mem(struct jffs2_eraseblock *jeb, struct jffs2_raw_dirent *rd, uint32_t ofs);
> +
> +int jffs2_sum_scan_sumnode(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb,
> +			uint32_t ofs, uint32_t *pseudo_random);
> +
> +#else	// SUMMARY NOT ENABLED
> +#define jffs2_sum_active() (0)
> +#define jffs2_sum_reset_collected(a)
> +#define jffs2_sum_add_kvec(a,b,c,d) (0)
> +#define jffs2_sum_care_sum_collected(a) (0)
> +#define jffs2_sum_clean_collected(a)
> +#define jffs2_sum_write_sumnode(a) (0)
> +#define jffs2_sum_add_padding_mem(a,b)
> +#define jffs2_sum_add_inode_mem(a,b,c)
> +#define jffs2_sum_add_dirent_mem(a,b,c)
> +#define jffs2_sum_scan_sumnode(a,b,c,d) (0)
> +
> +#endif // CONFIG_JFFS2_SUMMARY
> +
> +#endif /* JFFS2_SUMMARY_H */
> diff -Narup Mtd-orig/fs/jffs2/super-v24.c mtd/fs/jffs2/super-v24.c
> --- Mtd-orig/fs/jffs2/super-v24.c	2005-07-20 16:21:40.000000000 +0200
> +++ mtd/fs/jffs2/super-v24.c	2005-08-11 15:17:41.000000000 +0200
> @@ -24,6 +24,7 @@
>  #include <linux/mtd/mtd.h>
>  #include "compr.h"
>  #include "nodelist.h"
> +#include "summary.h"
>  
>  #ifndef MTD_BLOCK_MAJOR
>  #define MTD_BLOCK_MAJOR 31
> @@ -82,6 +83,9 @@ static struct super_block *jffs2_read_su
>  		return NULL;
>  	}
>  
> +	if (jffs2_sum_active())
> +		jffs2_sum_init(c);
> +

Just jffs2_sum_init(c).  That function should contain something like
	if (!jffs2_sum_active)
		return;
before all other code.

>  	return sb;
>  }
>  
> @@ -97,6 +101,12 @@ static void jffs2_put_super (struct supe
>  	down(&c->alloc_sem);
>  	jffs2_flush_wbuf_pad(c);
>  	up(&c->alloc_sem);
> +
> +	if (jffs2_sum_active()) {
> +		jffs2_sum_clean_all_info(c);
> +		jffs2_sum_exit(c);
> +	}
> +

These two functions are never called alone.  Combine them somehow.
And while you're at it, move the jffs2_sum_active() check inside the
converged function as well.

>  	jffs2_free_ino_caches(c);
>  	jffs2_free_raw_node_refs(c);
>  	if (c->mtd->flags & MTD_NO_VIRTBLOCKS)
> @@ -122,6 +132,9 @@ static int __init init_jffs2_fs(void)
>  #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>  	       " (NAND)"
>  #endif
> +#ifdef CONFIG_JFFS2_SUMMARY
> +	       " (SUMMARY)"
> +#endif
>  	       " (C) 2001-2003 Red Hat, Inc.\n");
>  
>  #ifdef JFFS2_OUT_OF_KERNEL
> diff -Narup Mtd-orig/fs/jffs2/super.c mtd/fs/jffs2/super.c
> --- Mtd-orig/fs/jffs2/super.c	2005-07-12 18:37:08.000000000 +0200
> +++ mtd/fs/jffs2/super.c	2005-08-11 15:17:41.000000000 +0200
> @@ -161,6 +161,9 @@ static struct super_block *jffs2_get_sb_
>  		return ERR_PTR(ret);
>  	}
>  
> +	if (jffs2_sum_active())
> +		jffs2_sum_init(c);
> +
>  	sb->s_flags |= MS_ACTIVE;
>  	return sb;
>  
> @@ -282,6 +285,12 @@ static void jffs2_put_super (struct supe
>  	down(&c->alloc_sem);
>  	jffs2_flush_wbuf_pad(c);
>  	up(&c->alloc_sem);
> +
> +	if (jffs2_sum_active()) {
> +		jffs2_sum_clean_all_info(c);
> +		jffs2_sum_exit(c);
> +	}
> +
>  	jffs2_free_ino_caches(c);
>  	jffs2_free_raw_node_refs(c);
>  	if (c->mtd->flags & MTD_NO_VIRTBLOCKS)
> @@ -321,6 +330,9 @@ static int __init init_jffs2_fs(void)
>  #ifdef CONFIG_JFFS2_FS_WRITEBUFFER
>  	       " (NAND)"
>  #endif
> +#ifdef CONFIG_JFFS2_SUMMARY
> +	       " (SUMMARY) "
> +#endif
>  	       " (C) 2001-2003 Red Hat, Inc.\n");
>  
>  	jffs2_inode_cachep = kmem_cache_create("jffs2_i",
> diff -Narup Mtd-orig/fs/jffs2/wbuf.c mtd/fs/jffs2/wbuf.c
> --- Mtd-orig/fs/jffs2/wbuf.c	2005-08-06 06:51:30.000000000 +0200
> +++ mtd/fs/jffs2/wbuf.c	2005-08-11 15:24:16.000000000 +0200
> @@ -263,7 +263,7 @@ static void jffs2_wbuf_recover(struct jf
>  
>  
>  	/* ... and get an allocation of space from a shiny new block instead */
> -	ret = jffs2_reserve_space_gc(c, end-start, &ofs, &len);
> +	ret = jffs2_reserve_space_gc(c, end-start, &ofs, &len, JFFS2_SUMMARY_NOSUM_SIZE);
>  	if (ret) {
>  		printk(KERN_WARNING "Failed to allocate space for wbuf recovery. Data loss ensues.\n");
>  		kfree(buf);
> @@ -834,6 +834,12 @@ int jffs2_flash_writev(struct jffs2_sb_i
>  alldone:
>  	*retlen = donelen;
>  
> +	if (jffs2_sum_active()) {
> +		if (jffs2_sum_add_kvec(c, invecs, count, (uint32_t) to)) {
> +			printk("jffs2_sum_add_kvec(): MEMORY ALLOCATION ERROR!");
> +		}
> +	}
> +

Please fold all this into jffs2_sum_add_kvec().  You really only want
a single line plus error recovery in the calling code.  And printk()
is *not* error recovery.  "return -EIO" would be.

>  	if (c->wbuf_len && ino)
>  		jffs2_wbuf_dirties_inode(c, ino);
>  
> @@ -853,7 +859,7 @@ int jffs2_flash_write(struct jffs2_sb_in
>  	struct kvec vecs[1];
>  
>  	if (!jffs2_is_writebuffered(c))
> -		return c->mtd->write(c->mtd, ofs, len, retlen, buf);
> +		return jffs2_flash_direct_write(c, ofs, len, retlen, buf);
>  
>  	vecs[0].iov_base = (unsigned char *) buf;
>  	vecs[0].iov_len = len;
> diff -Narup Mtd-orig/fs/jffs2/write.c mtd/fs/jffs2/write.c
> --- Mtd-orig/fs/jffs2/write.c	2005-07-20 17:50:51.000000000 +0200
> +++ mtd/fs/jffs2/write.c	2005-08-11 15:17:41.000000000 +0200
> @@ -153,13 +153,15 @@ struct jffs2_full_dnode *jffs2_write_dno
>  			jffs2_dbg_acct_paranoia_check(c, jeb);
>  
>  			if (alloc_mode == ALLOC_GC) {
> -				ret = jffs2_reserve_space_gc(c, sizeof(*ri) + datalen, &flash_ofs, &dummy);
> +				ret = jffs2_reserve_space_gc(c, sizeof(*ri) + datalen, &flash_ofs,
> +							&dummy, JFFS2_SUMMARY_INODE_SIZE);
>  			} else {
>  				/* Locking pain */
>  				up(&f->sem);
>  				jffs2_complete_reservation(c);
>  			
> -				ret = jffs2_reserve_space(c, sizeof(*ri) + datalen, &flash_ofs, &dummy, alloc_mode);
> +				ret = jffs2_reserve_space(c, sizeof(*ri) + datalen, &flash_ofs,
> +							&dummy, alloc_mode, JFFS2_SUMMARY_INODE_SIZE);
>  				down(&f->sem);
>  			}
>  
> @@ -299,13 +301,15 @@ struct jffs2_full_dirent *jffs2_write_di
>  			jffs2_dbg_acct_paranoia_check(c, jeb);
>  
>  			if (alloc_mode == ALLOC_GC) {
> -				ret = jffs2_reserve_space_gc(c, sizeof(*rd) + namelen, &flash_ofs, &dummy);
> +				ret = jffs2_reserve_space_gc(c, sizeof(*rd) + namelen, &flash_ofs,
> +							&dummy, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  			} else {
>  				/* Locking pain */
>  				up(&f->sem);
>  				jffs2_complete_reservation(c);
>  			
> -				ret = jffs2_reserve_space(c, sizeof(*rd) + namelen, &flash_ofs, &dummy, alloc_mode);
> +				ret = jffs2_reserve_space(c, sizeof(*rd) + namelen, &flash_ofs,
> +							&dummy, alloc_mode, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  				down(&f->sem);
>  			}
>  
> @@ -362,7 +366,8 @@ int jffs2_write_inode_range(struct jffs2
>  	retry:
>  		D2(printk(KERN_DEBUG "jffs2_commit_write() loop: 0x%x to write to 0x%x\n", writelen, offset));
>  
> -		ret = jffs2_reserve_space(c, sizeof(*ri) + JFFS2_MIN_DATA_LEN, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +		ret = jffs2_reserve_space(c, sizeof(*ri) + JFFS2_MIN_DATA_LEN, &phys_ofs,
> +					&alloclen, ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
>  		if (ret) {
>  			D1(printk(KERN_DEBUG "jffs2_reserve_space returned %d\n", ret));
>  			break;
> @@ -449,7 +454,8 @@ int jffs2_do_create(struct jffs2_sb_info
>  	/* Try to reserve enough space for both node and dirent. 
>  	 * Just the node will do for now, though 
>  	 */
> -	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*ri), &phys_ofs, &alloclen, ALLOC_NORMAL,
> +				JFFS2_SUMMARY_INODE_SIZE);
>  	D1(printk(KERN_DEBUG "jffs2_do_create(): reserved 0x%x bytes\n", alloclen));
>  	if (ret) {
>  		up(&f->sem);
> @@ -478,7 +484,8 @@ int jffs2_do_create(struct jffs2_sb_info
>  
>  	up(&f->sem);
>  	jffs2_complete_reservation(c);
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  		
>  	if (ret) {
>  		/* Eep. */
> @@ -548,7 +555,8 @@ int jffs2_do_unlink(struct jffs2_sb_info
>  		if (!rd)
>  			return -ENOMEM;
>  
> -		ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_DELETION);
> +		ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +					ALLOC_DELETION, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  		if (ret) {
>  			jffs2_free_raw_dirent(rd);
>  			return ret;
> @@ -657,7 +665,8 @@ int jffs2_do_link (struct jffs2_sb_info 
>  	if (!rd)
>  		return -ENOMEM;
>  
> -	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen, ALLOC_NORMAL);
> +	ret = jffs2_reserve_space(c, sizeof(*rd)+namelen, &phys_ofs, &alloclen,
> +				ALLOC_NORMAL, JFFS2_SUMMARY_DIRENT_SIZE(namelen));
>  	if (ret) {
>  		jffs2_free_raw_dirent(rd);
>  		return ret;
> diff -Narup Mtd-orig/fs/jffs2/writev.c mtd/fs/jffs2/writev.c
> --- Mtd-orig/fs/jffs2/writev.c	2004-11-16 21:36:12.000000000 +0100
> +++ mtd/fs/jffs2/writev.c	2005-08-11 16:16:17.000000000 +0200
> @@ -44,7 +44,33 @@ int jffs2_flash_direct_writev(struct jff
>  {
>  	if (c->mtd->writev)
>  		return c->mtd->writev(c->mtd, vecs, count, to, retlen);
> -	else
> +	else {
> +		if (jffs2_sum_active()) {
> +			D1(printk("SUMMARY: jffs2_flash_direct_writev(): Without NAND support\n"));
> +			if (jffs2_sum_add_kvec(c, vecs, count, (uint32_t) to)) {
> +				printk("jffs2_sum_add_kvec(): MEMORY ALLOCATION ERROR!");
> +			}
> +		}

Fold into jffs2_sum_add_kvec().

>  		return mtd_fake_writev(c->mtd, vecs, count, to, retlen);
> +	}
>  }
>  
> +int jffs2_flash_direct_write(struct jffs2_sb_info *c, loff_t ofs, size_t len,
> +			size_t *retlen, const u_char *buf)
> +{
> +	int ret;
> +	ret = c->mtd->write(c->mtd, ofs, len, retlen, buf);
> +
> +	if (jffs2_sum_active()) {
> +		struct kvec vecs[1];
> +
> +		vecs[0].iov_base = (unsigned char *) buf;
> +		vecs[0].iov_len = len;
> +
> +		if (jffs2_sum_add_kvec(c, vecs, 1, (uint32_t) ofs)) {
> +			printk("%s(): MEMORY ALLOCATION ERROR!",__FUNCTION__);
> +		}

Fold into jffs2_sum_add_kvec().

> +	}
> +	return ret;
> +}
> diff -Narup Mtd-orig/include/linux/jffs2.h mtd/include/linux/jffs2.h
> --- Mtd-orig/include/linux/jffs2.h	2005-07-26 15:19:36.000000000 +0200
> +++ mtd/include/linux/jffs2.h	2005-08-11 16:16:39.000000000 +0200
> @@ -28,6 +28,9 @@
>  #define JFFS2_EMPTY_BITMASK 0xffff
>  #define JFFS2_DIRTY_BITMASK 0x0000
>  
> +/* Summary node MAGIC marker */
> +#define JFFS2_SUM_MAGIC	0x02851885
> +
>  /* We only allow a single char for length, and 0xFF is empty flash so
>     we don't want it confused with a real length. Hence max 254.
>  */
> @@ -60,6 +63,8 @@
>  #define JFFS2_NODETYPE_CLEANMARKER (JFFS2_FEATURE_RWCOMPAT_DELETE | JFFS2_NODE_ACCURATE | 3)
>  #define JFFS2_NODETYPE_PADDING (JFFS2_FEATURE_RWCOMPAT_DELETE | JFFS2_NODE_ACCURATE | 4)
>  
> +#define JFFS2_NODETYPE_SUMMARY (JFFS2_FEATURE_RWCOMPAT_DELETE | JFFS2_NODE_ACCURATE | 6)
> +
>  // Maybe later...
>  //#define JFFS2_NODETYPE_CHECKPOINT (JFFS2_FEATURE_RWCOMPAT_DELETE | JFFS2_NODE_ACCURATE | 3)
>  //#define JFFS2_NODETYPE_OPTIONS (JFFS2_FEATURE_RWCOMPAT_COPY | JFFS2_NODE_ACCURATE | 4)
> @@ -146,10 +151,24 @@ struct jffs2_raw_inode
>  	uint8_t data[0];
>  } __attribute__((packed));
>  
> +struct jffs2_summary_node{
> +	jint16_t magic;
> +	jint16_t nodetype; 	/* = JFFS2_NODETYPE_INODE_SUM */
> +	jint32_t totlen;
> +	jint32_t hdr_crc;
> +	jint16_t sum_num;	/* number of sum entries*/

This breaks natural alignment on some architectures.  Besides wasting
memory on them, it can also defeat the purpose of
__attribute__((packed)), creating an incompatibility between different
architectures.

You should rearrange the fields or add some padding.

> +	jint32_t cln_mkr;	/* clean marker size, 0 = no cleanmarker */
> +	jint32_t padded;	/* sum of the size of padding nodes */
> +	jint32_t sum_crc;	/* summary information crc */
> +	jint32_t node_crc; 	/* node crc */
> +	jint32_t sum[0]; 	/* inode summary info */
> +} __attribute__((packed));
> +
>  union jffs2_node_union {
>  	struct jffs2_raw_inode i;
>  	struct jffs2_raw_dirent d;
>  	struct jffs2_unknown_node u;
> +	struct jffs2_summary_node s;
>  };
>  
>  #endif /* __LINUX_JFFS2_H__ */
> diff -Narup Mtd-orig/include/linux/jffs2_fs_sb.h mtd/include/linux/jffs2_fs_sb.h
> --- Mtd-orig/include/linux/jffs2_fs_sb.h	2005-05-19 18:12:17.000000000 +0200
> +++ mtd/include/linux/jffs2_fs_sb.h	2005-08-11 15:17:41.000000000 +0200
> @@ -112,6 +112,8 @@ struct jffs2_sb_info {
>  	uint32_t fsdata_len;
>  #endif
>  
> +	jint32_t *summary_buf;		// buffer for Erase Block Summary
> +
>  	/* OS-private pointer for getting back to master superblock info */
>  	void *os_priv;
>  };

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

 >diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
 >--- Mtd-orig/fs/jffs2/build.c	2005-07-30 17:29:27.000000000 +0200
 >+++ mtd/fs/jffs2/build.c	2005-08-11 15:17:41.000000000 +0200
 >@@ -336,6 +336,7 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
 > 		c->blocks[i].first_node = NULL;
 > 		c->blocks[i].last_node = NULL;
 > 		c->blocks[i].bad_count = 0;
 >+		c->blocks[i].sum_collected = NULL;
 > 	}

And my question. Why 'sum_collected; is per-eraseblock? JFFS2 always 
writes to c->nextblock, so 'sum_collected' ought to be per-superblock, 
isn't it? Why do you inflate the c->blocks[] array ?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Artem,

> >diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
> >--- Mtd-orig/fs/jffs2/build.c    2005-07-30 17:29:27.000000000 +0200
> >+++ mtd/fs/jffs2/build.c    2005-08-11 15:17:41.000000000 +0200
> >@@ -336,6 +336,7 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
> >         c->blocks[i].first_node = NULL;
> >         c->blocks[i].last_node = NULL;
> >         c->blocks[i].bad_count = 0;
> >+        c->blocks[i].sum_collected = NULL;
> >     }
>
> And my question. Why 'sum_collected; is per-eraseblock? JFFS2 always 
> writes to c->nextblock, so 'sum_collected' ought to be per-superblock, 
> isn't it? Why do you inflate the c->blocks[] array ?

We thought a lot about it when we designed summary. Now I don't remember 
why we decided to do it in this way (Zoltan, do you?), we'll reconsider it.

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 13:42:19 +0200, Ferenc Havasi wrote:
> 
> >>diff -Narup Mtd-orig/fs/jffs2/build.c mtd/fs/jffs2/build.c
> >>--- Mtd-orig/fs/jffs2/build.c    2005-07-30 17:29:27.000000000 +0200
> >>+++ mtd/fs/jffs2/build.c    2005-08-11 15:17:41.000000000 +0200
> >>@@ -336,6 +336,7 @@ int jffs2_do_mount_fs(struct jffs2_sb_in
> >>         c->blocks[i].first_node = NULL;
> >>         c->blocks[i].last_node = NULL;
> >>         c->blocks[i].bad_count = 0;
> >>+        c->blocks[i].sum_collected = NULL;
> >>     }
> >
> >And my question. Why 'sum_collected; is per-eraseblock? JFFS2 always 
> >writes to c->nextblock, so 'sum_collected' ought to be per-superblock, 
> >isn't it? Why do you inflate the c->blocks[] array ?
> 
> We thought a lot about it when we designed summary. Now I don't remember 
> why we decided to do it in this way (Zoltan, do you?), we'll reconsider it.

Having it per erase block would come in handy, if someone actually
implemented the "use two erase blocks to write data - one for GC'd
stuff and the other for new data" approach.

But then again, we could have a "struct active_block" or something
similar to describe an erase block that is currently use for writing,
so it doesn't really matter.  Yeah, should go into the superblock.

Jörn

-- 
All art is but imitation of nature.
-- Lucius Annaeus Seneca

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ok, to keep you busy with some technical stuff also, here is a remark.

 >+		if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
 >+
 >+			if(je32_to_cpu(sm->erase_size) == c->sector_size) {
 >+				err = 
 >jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);
 >+
 >+				if (err) {
 >+					kfree(sm);
 >+					return err;
 >+				}
 >+			}
 >+			printk(KERN_WARNING "FS erase_block_size != JFFS2 erase_block_size 
=> skipping summary information\n");
 >+		}

if(je32_to_cpu(sm->erase_size) == c->sector_size) { ...

AFAIU, you're protecting against the situation when the size of the 
JFFS2 virtual eraseblock is changed.

But why is the eraseblock size may change? If it becomes less, this is a 
problem since some nodes that cross the boundary will be ignored. If the 
size of the JFFS2 virtual eraseblock becomes larger, it is also a 
problem - due to present bad blocks, some eraseblocks with valid data 
will be ignored (remember, if there is a bad eraseblock in the JFFS2 
virtual eraseblock, the whole virtual eraseblock is regarded as bad).

Thus, please, don't solve this problem this way. You're trying to solve 
it privately for your summaries, while you have to solve it for *JFFS2 
in general* (if it exists at all). I suppose it exists in your case 
since you add one more field to struct eraseblock...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Artem,

> Ok, to keep you busy with some technical stuff also, here is a remark.
>
> >+        if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
> >+
> >+            if(je32_to_cpu(sm->erase_size) == c->sector_size) {
> >+                err = 
> >jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);
> >+
> >+                if (err) {
> >+                    kfree(sm);
> >+                    return err;
> >+                }
> >+            }
> >+            printk(KERN_WARNING "FS erase_block_size != JFFS2 
> erase_block_size => skipping summary information\n");
> >+        }
>
> if(je32_to_cpu(sm->erase_size) == c->sector_size) { ...
>
> AFAIU, you're protecting against the situation when the size of the 
> JFFS2 virtual eraseblock is changed.

No. The task of this condition is to make sure that the summary was 
generated correctly. If the summary is generated by the filesystem, it 
will be always good, but you are also able to generate summary with the 
userspace tool 'sumtool'. If we do not recognise this mistake that can 
cause big problems and make the user confused.

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 13:48:57 +0200, Ferenc Havasi wrote:
> 
> No. The task of this condition is to make sure that the summary was 
> generated correctly. If the summary is generated by the filesystem, it 
> will be always good, but you are also able to generate summary with the 
> userspace tool 'sumtool'. If we do not recognise this mistake that can 
> cause big problems and make the user confused.

I don't buy this argument.  If 'sumtool' fucks up, it should either be
fixed or pulled off the net.  There is no excuse for broken code.

At least you need to go into details if you want to convince me.

Jörn

-- 
If you're willing to restrict the flexibility of your approach,
you can almost always do something better.
-- John Carmack

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Hi Jörn & Artem,

>>No. The task of this condition is to make sure that the summary was 
>>generated correctly. If the summary is generated by the filesystem, it 
>>will be always good, but you are also able to generate summary with the 
>>userspace tool 'sumtool'. If we do not recognise this mistake that can 
>>cause big problems and make the user confused.
>>    
>>
>
>I don't buy this argument.  If 'sumtool' fucks up, it should either be
>fixed or pulled off the net.  There is no excuse for broken code.
>
>At least you need to go into details if you want to convince me.
>  
>
OK, we can remove this check, and field.

I only say that for a user the size of the virtual erase block may not 
be evident. They know about the real erase block size. Maybe we should 
write a tool for calculating it.

It is not too relevant problem without summary. For example if the erase 
block size is 32K, and the virtual is 64K, and you generates JFFS2 
image  for 32K it will work (maybe with some warning but will work). If 
you generates summary for that image for 32K and use it on 64K virtual 
erase block size that will not work.

Bye,
Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> I only say that for a user the size of the virtual erase block may not 
> be evident. They know about the real erase block size. Maybe we should 
> write a tool for calculating it.
Why not to just copy the algorithm from JFFS2 (it is in 
jffs2_do_fill_super())?

> 
> It is not too relevant problem without summary. For example if the erase 
> block size is 32K, and the virtual is 64K, and you generates JFFS2 
> image  for 32K it will work (maybe with some warning but will work). If 
> you generates summary for that image for 32K and use it on 64K virtual 
> erase block size that will not work.
I think, this is better if you solve this inconsistency in general.

And by the way, I don't understand, why do you need sumtool and 
sumtool2? Is sumtool3 coming ?

Why not to merge sumtoolX with mkfs.jffs2 and add more options?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 16:38:45 +0400, Artem B. Bityuckiy wrote:
> Ferenc Havasi wrote:
> >I only say that for a user the size of the virtual erase block may not 
> >be evident. They know about the real erase block size. Maybe we should 
> >write a tool for calculating it.
> Why not to just copy the algorithm from JFFS2 (it is in 
> jffs2_do_fill_super())?

Because it's not good enough.

> >It is not too relevant problem without summary. For example if the erase 
> >block size is 32K, and the virtual is 64K, and you generates JFFS2 
> >image  for 32K it will work (maybe with some warning but will work). If 
> >you generates summary for that image for 32K and use it on 64K virtual 
> >erase block size that will not work.
> I think, this is better if you solve this inconsistency in general.

Yes.  The cumulation of erase blocks is an ugly ugly hack.  The fact
that vmalloc() of more than 128M will always fail doesn't imply that
vmalloc() of less will always succeed.  On i386 with 1GiB of ram and a
single kernel module loaded, the chance has already dropped to 0.

Now, "calculating" the erase block size such that we only vmalloc() a
bit less than 128M is an exquisitely bad idea.  It may work on some
machines, but will utterly fail on others.

The more I think about it, the more I believe that we have to remove
the vmalloc completely.  Anything else will still break at random,
depending on hardware, code like summary, moonphase and the taste of
your coffee this morning.

Jörn

-- 
Those who come seeking peace without a treaty are plotting.
-- Sun Tzu

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Jörn Engel wrote:

>Yes.  The cumulation of erase blocks is an ugly ugly hack.  The fact
>that vmalloc() of more than 128M will always fail doesn't imply that
>vmalloc() of less will always succeed.  On i386 with 1GiB of ram and a
>single kernel module loaded, the chance has already dropped to 0.
>
>  
>
I agree.

But If I am right we use kmalloc, and its limit is 128K. Why don't we 
use vmalloc() instead there?

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> But If I am right we use kmalloc, and its limit is 128K. Why don't we 
> use vmalloc() instead there?
kmalloc'ed memory is faster... But I would just always use it and 1:1 
mapping...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 17:42:51 +0400, Artem B. Bityuckiy wrote:
> Ferenc Havasi wrote:
> >But If I am right we use kmalloc, and its limit is 128K. Why don't we 
> >use vmalloc() instead there?
> kmalloc'ed memory is faster... But I would just always use it and 1:1 
> mapping...

Ouch.  High-order memory allocations anyone?

My preferred approach would be to allocate a slab cache for jffs2
erase blocks.  Then we either need a (much smaller) array of pointers
to the slab objects or maintain them as linked lists.  But then again,
it wouldn't be the first ugly hack in jffs2.  Go right ahead if you
really want to!

Jörn

-- 
Mundie uses a textbook tactic of manipulation: start with some
reasonable talk, and lead the audience to an unreasonable conclusion.
-- Bruce Perens

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> My preferred approach would be to allocate a slab cache for jffs2
> erase blocks.  Then we either need a (much smaller) array of pointers
> to the slab objects or maintain them as linked lists.  But then again,
> it wouldn't be the first ugly hack in jffs2.  Go right ahead if you
> really want to!
> 

Only not slab cache, what for? We allocate the erasblock structure once 
on mount and deallocate them all once on unmount. No need to use a slab 
cache. The merit of the slab cache is very fast allocations, and we will 
not make use of this property anyway. No need to use slab cache here at all.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 18:00:28 +0400, Artem B. Bityuckiy wrote:
> Jörn Engel wrote:
> >My preferred approach would be to allocate a slab cache for jffs2
> >erase blocks.  Then we either need a (much smaller) array of pointers
> >to the slab objects or maintain them as linked lists.  But then again,
> >it wouldn't be the first ugly hack in jffs2.  Go right ahead if you
> >really want to!
> 
> Only not slab cache, what for? We allocate the erasblock structure once 
> on mount and deallocate them all once on unmount. No need to use a slab 
> cache. The merit of the slab cache is very fast allocations, and we will 
> not make use of this property anyway. No need to use slab cache here at all.

One of its merits is fast allocation, yes.

Others are less external fragmentation and lower-order memory
allocations.

Say, you need about 65k for your erase blocks.  With slab, you need to
allocate roughly 17 pages of 4k each, plus slab overhead.  With
kmalloc, you allocate 128k.  That's nearly twice what you need.

With slab, you allocate order-0 pages.  With kmalloc you allocate
order-5 pages.  Ask Martin J Bligh about high-order allocations if you
want to hear some spooky stories.

Going from kmalloc to vmalloc makes things even worse.

Jörn

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

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

On Mon, 2005-08-15 at 16:05 +0200, Jörn Engel wrote:
> Others are less external fragmentation and lower-order memory
> allocations.
> 
> Say, you need about 65k for your erase blocks.  With slab, you need to
> allocate roughly 17 pages of 4k each, plus slab overhead.  With
> kmalloc, you allocate 128k.  That's nearly twice what you need.
> 
> With slab, you allocate order-0 pages.  With kmalloc you allocate
> order-5 pages.  Ask Martin J Bligh about high-order allocations if you
> want to hear some spooky stories.
> 
> Going from kmalloc to vmalloc makes things even worse.
> 

Ok, how about just to allocate as many pages as we need, and make an
array of pointers to pages? Then the 'struct jffs2_eraseblock' object of
the eraseblock with offset 'offs' will be something like this:

blocks_per_page = PAGE_SIZE/sizeof(struct jffs2_eraseblock);
blknum = offs/c->erase_size;
pgidx = blknum/PAGE_SIZE;
blkidx = blknum - pgidx * blocks_per_page;

jeb = &c->blocks[pgidx][blkidx];

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 18:19:07 +0400, Artem B. Bityuckiy wrote:
> On Mon, 2005-08-15 at 16:05 +0200, Jörn Engel wrote:
> > Others are less external fragmentation and lower-order memory
> > allocations.
> > 
> > Say, you need about 65k for your erase blocks.  With slab, you need to
> > allocate roughly 17 pages of 4k each, plus slab overhead.  With
> > kmalloc, you allocate 128k.  That's nearly twice what you need.
> > 
> > With slab, you allocate order-0 pages.  With kmalloc you allocate
> > order-5 pages.  Ask Martin J Bligh about high-order allocations if you
> > want to hear some spooky stories.
> > 
> > Going from kmalloc to vmalloc makes things even worse.
> 
> Ok, how about just to allocate as many pages as we need, and make an
> array of pointers to pages? Then the 'struct jffs2_eraseblock' object of
> the eraseblock with offset 'offs' will be something like this:
> 
> blocks_per_page = PAGE_SIZE/sizeof(struct jffs2_eraseblock);
> blknum = offs/c->erase_size;
> pgidx = blknum/PAGE_SIZE;
> blkidx = blknum - pgidx * blocks_per_page;
> 
> jeb = &c->blocks[pgidx][blkidx];

How about encapsulating all the page handling code in - say -
mm/slab.c and just handling single objects? ;)

Jörn

-- 
Unless something dramatically changes, by 2015 we'll be largely
wondering what all the fuss surrounding Linux was really about.
-- Rob Enderle

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> How about encapsulating all the page handling code in - say -
> mm/slab.c and just handling single objects? ;)
>
Well, after thinking for the second time - yes, slab does the same, and 
even better If we remember about the colouring. Forget about my offer.
:-)

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

On Mon, 2005-08-15 at 19:22 +0400, Artem B. Bityuckiy wrote:
> Jörn Engel wrote:
> > How about encapsulating all the page handling code in - say -
> > mm/slab.c and just handling single objects? ;)
> >
> Well, after thinking for the second time - yes, slab does the same, and 
> even better If we remember about the colouring. Forget about my offer.
> :-)
> 
Wait, but how will you then find the needed struct jffs2_eraseblock
object by its physical offset on flash ? Use Hash / lists ? Bad idea
(slow). Use array of pointers to each object ? It will be rather large,
and I cannot take easy the fact that we will use distinct pointers for
objects which are mostly physically contiguous in RAM (they are laying
contiguously in slabs). This probably will neglect the pluses of
colouring, which, BTW, we may also organize using my way...

Comments?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Artem B. Bityuckiy wrote:
> Wait, but how will you then find the needed struct jffs2_eraseblock
> object by its physical offset on flash ? Use Hash / lists ? Bad idea
> (slow). Use array of pointers to each object ? It will be rather large,
> and I cannot take easy the fact that we will use distinct pointers for
> objects which are mostly physically contiguous in RAM (they are laying
> contiguously in slabs). This probably will neglect the pluses of
> colouring, which, BTW, we may also organize using my way...
> 
> Comments?

This assumed to be a question for Joern, not to myself :-)

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Tue, 16 August 2005 11:25:40 +0400, Artem B. Bityuckiy wrote:
> Artem B. Bityuckiy wrote:
> >Wait, but how will you then find the needed struct jffs2_eraseblock
> >object by its physical offset on flash ? Use Hash / lists ? Bad idea
> >(slow). Use array of pointers to each object ? It will be rather large,
> >and I cannot take easy the fact that we will use distinct pointers for
> >objects which are mostly physically contiguous in RAM (they are laying
> >contiguously in slabs). This probably will neglect the pluses of
> >colouring, which, BTW, we may also organize using my way...
> >
> >Comments?
> 
> This assumed to be a question for Joern, not to myself :-)

Yes, I'd go for a pointer array.  That would cost us one pointer
dereference per access.

Your approach, imo, is fundamentally borked because it requires
high-order allocations.  Your array of physical pages has to be a
contiguous virtual memory area, which won't always work.  And if you
want to maintain a list of pages instead, you end up with the same
design as with an array of slab objects, just one level higher and
with another indirection.  Whatever the performance is, the code will
be ugly.

You could - if you really want to play tricks - allocate objects that
contain 2, 4, 8, 16, etc. struct jffs2_eraseblocks instead of just
one.  But that wouldn't make the code any better, either.

Oh well.  If you volunteer to write the code and fix the bugs, you get
to decide.  I just have an opinion, nothing more.

Jörn

-- 
"Security vulnerabilities are here to stay."
-- Scott Culp, Manager of the Microsoft Security Response Center, 2001

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> Your approach, imo, is fundamentally borked because it requires
> high-order allocations.  Your array of physical pages has to be a
> contiguous virtual memory area, which won't always work.  And if you
> want to maintain a list of pages instead, you end up with the same
> design as with an array of slab objects, just one level higher and
> with another indirection.  Whatever the performance is, the code will
> be ugly.

I meant something like:

objs_per_page = PAGE_SIZE / sizeof(struct jffs2_eraseblock);
pages_num = c->blocks_nr / objs_per_page;

for (i = 0; i < pages_num; i++) {
     page = alloc_pages(flags, 0);
     c->blocks[i] = page_address(page);
}

So c->blocks[] is an array of pages. Nonetheless, I have never done 
things like this before, may be there is some cleverer method exists...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 15:34:09 +0200, Ferenc Havasi wrote:
> 
> But If I am right we use kmalloc, and its limit is 128K. Why don't we 
> use vmalloc() instead there?

You're right, it was kmalloc and not vmalloc.  Fundamental problem
remains, though.

I went through this with Artem on irc.  We haven't found a solution
yet, but here are some of the bits and pieces.  Maybe someone can
assembler them into something sane.

Constraints:
1. Once a jffs2 partition/image is created, the virtual erase block
   size may never, under any circumstances, change.
2. Virtual erase block size must be stored in a way that it will
   *always* be read during mount, preferrably early on.

A simple solution for this problem is virtual size == physical size.

One reason to aggregate physical blocks into virtual ones is to save
memory.  The aggregation code makes sure that we never spend more than
128k on the array of struct erase_block.  Not sure if this is really a
valid concern, though.  Once we have such a big flash, jffs2 will
require tons of memory anyway.

If we go for a virtual:physical mapping other than 1:1, we need to be
safe against this scenario:

1. we have a flash with N erase blocks, 1:1 phyical:virtual mapping
2. first erase block is being erased
3. power-fail during erase, block is all FFs
4. resize of partition
5. with new partition size, physical:virtual mapping is N:1
6. no more block headers at the beginning of a virtual block

Jörn

-- 
Why do musicians compose symphonies and poets write poems?
They do it because life wouldn't have any meaning for them if they didn't.
That's why I draw cartoons.  It's my life.
-- Charles Shultz

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> One reason to aggregate physical blocks into virtual ones is to save
> memory.  The aggregation code makes sure that we never spend more than
> 128k on the array of struct erase_block.  Not sure if this is really a
> valid concern, though.  Once we have such a big flash, jffs2 will
> require tons of memory anyway.
Agree, I don't think this is important. I would just swith to firm 1:1.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

And one more question:

> +    if (je32_to_cpu(summary->hdr_crc) != crc) {
> +        D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node 
> header is corrupt (bad CRC or no summary at all)\n"));
> +        bad_sum = 1;
> +    }
Ok, we have bad CRC, why do we continue checking the other CRCs?

> +
> +    if ((!bad_sum) && (je32_to_cpu(summary->totlen) != sumsize)) {
> +        D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node
is 
> corrupt (wrong erasesize?)\n"));
> +        bad_sum = 1;
> +    }
The same, why do not use goto here?

> +
> +    crc = crc32(0, summary, sizeof(struct jffs2_summary_node)-8);
> +
> +    if ((!bad_sum) && (je32_to_cpu(summary->node_crc) != crc)) {
> +        D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node
is 
> corrupt (bad CRC)\n"));
> +        bad_sum = 1;
> +    }
Here.

> +
> +    crc = crc32(0, summary->sum, sumsize - sizeof(struct 
> jffs2_summary_node));
> +
> +    if ((!bad_sum) && (je32_to_cpu(summary->sum_crc) != crc)) {
> +        D1(printk(KERN_DEBUG "jffs2_scan_eraseblock(): Summary node 
> data is corrupt (bad CRC)\n"));
> +        bad_sum = 1;
> +    }
Here.

> +
> +    if (!bad_sum) {
> +
> +        struct jffs2_sum_unknown_flash *sp;
> +        sp = (struct jffs2_sum_unknown_flash *) summary->sum;
Now we have:

if (!bad_sum) {
    Huge block where we waste one indent! What for ?
}

return 0;


Why not to do like this:

if (1st crc check fails ())
    goto fail;

if (2nd crc check fails ())
    goto fail;

if (3rd crc check fails ())
    goto fail;

process summary with good CRC ();

fail:

return 0;

And we save one indent which is rather expensive in Linux :-)


-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Artem B. Bityuckiy wrote:

>> It is not too relevant problem without summary. For example if the 
>> erase block size is 32K, and the virtual is 64K, and you generates 
>> JFFS2 image  for 32K it will work (maybe with some warning but will 
>> work). If you generates summary for that image for 32K and use it on 
>> 64K virtual erase block size that will not work.
>
> I think, this is better if you solve this inconsistency in general.
>
I agree. We should find out how.

> And by the way, I don't understand, why do you need sumtool and 
> sumtool2? Is sumtool3 coming ?
>
It is an old stuff. Sumtool was the part of JFFS3, and sumtool2 was an 
additional stuff for JFFS2. Now, we will delete sumtool, and rename 
sumtool2 to sumtool.

> Why not to merge sumtoolX with mkfs.jffs2 and add more options?

We did not wanted to make the code of mkfs.jffs2 more complex. I think 
it is cleaner to do it in a seperate tool.

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> We did not wanted to make the code of mkfs.jffs2 more complex. I think 
> it is cleaner to do it in a seperate tool.
If summaries are get merged to JFFS2 it looks sane for me
not to have a separate tool ...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 17:40:07 +0400, Artem B. Bityuckiy wrote:
> Ferenc Havasi wrote:
> >We did not wanted to make the code of mkfs.jffs2 more complex. I think 
> >it is cleaner to do it in a seperate tool.
> If summaries are get merged to JFFS2 it looks sane for me
> not to have a separate tool ...

Dito.  But I don't care much for the userspace tools.  So the reviews
should be done by someone that cares.  Artem? ;)

Jörn

-- 
To recognize individual spam features you have to try to get into the
mind of the spammer, and frankly I want to spend as little time inside
the minds of spammers as possible.
-- Paul Graham

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> Dito.  But I don't care much for the userspace tools.  So the reviews
> should be done by someone that cares.  Artem? ;)
> 
Oh no, I did not ever even glimpse to mkfs.jffs2 code. I also only care 
about the kernel-side stuff... :-) But the presence of 2 separate tool 
bothers me. Imagine if ext3 have different tools to make the FS 
(mkfs.ext3), then to add the journal (a "journaltool"). What for ? I do 
not insist much though, if the others don't care.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> No. The task of this condition is to make sure that the summary was 
> generated correctly. If the summary is generated by the filesystem, it 
> will be always good, but you are also able to generate summary with the 
> userspace tool 'sumtool'. If we do not recognise this mistake that can 
> cause big problems and make the user confused.
> 
Err, do you mean that sumtool may generate summary with wrong erase_size?

Well, but my assumption is also valid. Glance at fs.c:466

c->sector_size = c->mtd->erasesize;
blocks = c->flash_size / c->sector_size;
if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
     while ((blocks * sizeof (struct jffs2_eraseblock)) > (128 * 1024)) {
         blocks >>= 1;
         c->sector_size <<= 1;
     }
}

Here the size of eraseblock depends on sizeof(struct jffs2_eraseblock). 
You increase the size of 'struct jffs2_eraseblock'. So, you may have a 
situation when one mounts an *old* image with *new* JFFS2 with *summary 
enabled*, and JFFS2 uses *larger* virtual eraseblock size, because

blocks * sizeof (struct jffs2_eraseblock)) > (128 * 1024)

condition is changed.

How is this situation handled?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Ferenc Havasi]

Artem B. Bityuckiy wrote:

> Ferenc Havasi wrote:
>
>> No. The task of this condition is to make sure that the summary was 
>> generated correctly. If the summary is generated by the filesystem, 
>> it will be always good, but you are also able to generate summary 
>> with the userspace tool 'sumtool'. If we do not recognise this 
>> mistake that can cause big problems and make the user confused.
>>
> Err, do you mean that sumtool may generate summary with wrong erase_size?

It means: the user called it with wrong parameters - because may not 
understood what is virtual erase block and how he should calculate it. I 
think it will be a typical mistake.

> Here the size of eraseblock depends on sizeof(struct 
> jffs2_eraseblock). You increase the size of 'struct jffs2_eraseblock'. 
> So, you may have a situation when one mounts an *old* image with *new* 
> JFFS2 with *summary enabled*, and JFFS2 uses *larger* virtual 
> eraseblock size, because
>
> blocks * sizeof (struct jffs2_eraseblock)) > (128 * 1024)
>
> condition is changed.
>
> How is this situation handled?

Now the user should know the virutal erase block size at summary 
generation time. If it recognized someshing is wrong it uses the 
original scanning method.

Ferenc

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Ok, let's change JFFS2. I would prefer just alwayse assume 1:1 mapping 
and always use vmalloc in JFFS2 for c->blocks[] (no 128K limitation).

For compatimility, we need to be able to fallback to the old JFFS2 
algorithms.

And here we definitely must introduce the way to recognise versions of 
the image. We may introduce the version number to clean markers. We also 
must set INCOMPAT flag in clean markers to prevent new images to be 
mounted by old JFFS2s. New JFFS2s will mount the FS only if the version 
is <= then the current JFFS2 version. Something like this. Versioning 
will anyway be useful in future.

Moreover, Forrest is adding the trick to store the erase count at the 
beginning of each eraseblock. So, we may store this erasecount in clean 
markers as well. Then we may rename "clean marker" to "node header".

On NAND, clean marker is in OOB of the first page. As new cleanmarkers 
become larger, we may span them to the 2nd, the 3rd, etc OOBs.

Comments?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 17:38:41 +0400, Artem B. Bityuckiy wrote:
> 
> Ok, let's change JFFS2. I would prefer just alwayse assume 1:1 mapping 
> and always use vmalloc in JFFS2 for c->blocks[] (no 128K limitation).
> 
> For compatimility, we need to be able to fallback to the old JFFS2 
> algorithms.
> 
> And here we definitely must introduce the way to recognise versions of 
> the image. We may introduce the version number to clean markers. We also 
> must set INCOMPAT flag in clean markers to prevent new images to be 
> mounted by old JFFS2s. New JFFS2s will mount the FS only if the version 
> is <= then the current JFFS2 version. Something like this. Versioning 
> will anyway be useful in future.

JFFS2 doesn't have a superblock.  Where should the versions be stored?

I'd create a new node with INCOMPATIBLE flag set.  That's the JFFS2
translation of what you just stated.  Converging this node with
something else like the former erase marker would make sense, yes.

> Moreover, Forrest is adding the trick to store the erase count at the 
> beginning of each eraseblock. So, we may store this erasecount in clean 
> markers as well. Then we may rename "clean marker" to "node header".

Sane.  We can combine these changes.

> On NAND, clean marker is in OOB of the first page. As new cleanmarkers 
> become larger, we may span them to the 2nd, the 3rd, etc OOBs.

You're the expert here.

Jörn

-- 
Linux is more the core point of a concept that surrounds "open source"
which, in turn, is based on a false concept. This concept is that
people actually want to look at source code.
-- Rob Enderle

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

Jörn Engel wrote:
> JFFS2 doesn't have a superblock.  Where should the versions be stored?
I thought to have them in cleanmarkers.

> I'd create a new node with INCOMPATIBLE flag set.  That's the JFFS2
> translation of what you just stated.  Converging this node with
> something else like the former erase marker would make sense, yes.
Agree, this is better.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

JFFS2 eraseblock header

[Artem B. Bityuckiy]

Ok, here is my *rough* offer about the JFFS2 eraseblock header.

What for?
~~~~~~~~~

a). Zhao Fusheng (aka Forrest) wants to implement fairer wear-levelling 
in JFFS2. For this reason he needs to store the 'erasecount' for each 
flash eraseblock.

b). The recent review of Ferenc Havasi's patch (aka the summary patch) 
showed a JFFS2 inconsistency in how it calculates the physical 
eraseblock to virtual eraseblock ratio. We want to fix this, probably by 
switching to constant 1:1 mapping. Because of this, for compatibility, 
we need to introduce JFFS2 versioning. Since there is no superblock in 
JFFS2, we may add the version number to the eraseblock header as well.

Versioning support will anyway be useful in future.

How to implement?
~~~~~~~~~~~~~~~~~

1. Get rid of clean marker and use eraseblock header instead (the 
eraseblock header will play the role of cleanmarker).
2. The length of the eraseblock header will not be constant, it may grow 
with growing version.
3. As it is implemented currently, store the eraseblock header at the 
beginning of the eraseblock, in OOB for NAND.
4. For NAND, we assume the eraseblock header may span several OOBs, not 
only the first page's.

What's with compatibility?
~~~~~~~~~~~~~~~~~~~~~~~~~~

1. Old JFFS2 binaries have to reject mounting new JFFS2 images (for 
example, because we are going to change the phys:virt mapping to 
constant 1:1, which is anyway incompatible). This is the problem in NAND 
since the compatibility bits are not checked in clean marker (see 
jffs2_check_nand_cleanmarker()). We may solve this problem by, for 
example, changing types of dirent/inode nodes and adding INCOMPAT flag 
there (yes, this is ugly workaround).

2. New JFFS2, when mounting an old JFFS2 binary, will use old algorithms 
(1:M mappings, clean markers).

3. If JFFS2 finds that the version of the image is larger, it will 
reject mounting the image. This will be useful in future.

Comments?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 eraseblock header

[Artem B. Bityuckiy]

Artem B. Bityuckiy wrote:
> 1. Get rid of clean marker and use eraseblock header instead (the 
> eraseblock header will play the role of cleanmarker).
Get rid if clean markers, not to add a new node, because anyway old 
JFFS2s must be incompatible with new images, so no need to have 2 
distinct nodes...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 eraseblock header

[Ferenc Havasi]

Artem B. Bityuckiy wrote:

> 2. New JFFS2, when mounting an old JFFS2 binary, will use old 
> algorithms (1:M mappings, clean markers).


I think if we do this change it would be better to forget old JFFS2 
binaries. If not that makes things even more complicated.

To make old images usable we can write a small user space converter tool.

Ferenc

Re: JFFS2 eraseblock header

[Artem B. Bityuckiy]

Ferenc Havasi wrote:
> I think if we do this change it would be better to forget old JFFS2 
> binaries. If not that makes things even more complicated.
Ugh, this is very much undesirable. And I hope it is not very difficult 
to support this. Just assume the version of old JFFS2 images = 1, and 
depending on the version, choose the algorithms... Let's at least try.

I would also like to hear David's coments...

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 eraseblock header

[David Woodhouse]

On Tue, 2005-08-16 at 12:22 +0400, Artem B. Bityuckiy wrote:
> 1. Get rid of clean marker and use eraseblock header instead (the 
> eraseblock header will play the role of cleanmarker).
> 2. The length of the eraseblock header will not be constant, it may grow 
> with growing version.
> 3. As it is implemented currently, store the eraseblock header at the 
> beginning of the eraseblock, in OOB for NAND.
> 4. For NAND, we assume the eraseblock header may span several OOBs, not 
> only the first page's.

OK.

> What's with compatibility?
> ~~~~~~~~~~~~~~~~~~~~~~~~~~
> 
> 1. Old JFFS2 binaries have to reject mounting new JFFS2 images (for 
> example, because we are going to change the phys:virt mapping to 
> constant 1:1, which is anyway incompatible). This is the problem in NAND 
> since the compatibility bits are not checked in clean marker (see 
> jffs2_check_nand_cleanmarker()). We may solve this problem by, for 
> example, changing types of dirent/inode nodes and adding INCOMPAT flag 
> there (yes, this is ugly workaround).

Hm, OK.

> 2. New JFFS2, when mounting an old JFFS2 binary, will use old algorithms 
> (1:M mappings, clean markers).

Should it? I wonder if it should silently upgrade, instead? Or at least
have a mount option to make it upgrade? We want to leave the old format
behind...

> 3. If JFFS2 finds that the version of the image is larger, it will 
> reject mounting the image. This will be useful in future.

OK.

-- 
dwmw2

Re: JFFS2 eraseblock header

[Artem B. Bityuckiy]

David Woodhouse wrote:
> Should it? I wonder if it should silently upgrade, instead? Or at least
> have a mount option to make it upgrade? We want to leave the old format
> behind...
That would be spiffy to leave it behind and save a lot efforts.
But in this case new JFFS2 binaries will reject mounting old JFFS2 images.
If this is accaptable - fine. Personally I don't care, but people may.
:-)

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 eraseblock header

[Jörn Engel]

On Thu, 8 September 2005 17:35:15 +0400, Artem B. Bityuckiy wrote:
> David Woodhouse wrote:
>
> >Should it? I wonder if it should silently upgrade, instead? Or at least
> >have a mount option to make it upgrade? We want to leave the old format
> >behind...
>
> That would be spiffy to leave it behind and save a lot efforts.
> But in this case new JFFS2 binaries will reject mounting old JFFS2 images.
> If this is accaptable - fine. Personally I don't care, but people may.
> :-)

Current strategy is to push the effort to the user.  Not perfect, but
it is necessary to inform the users about the problem anyway.

What we could additionally do is detect nodes that span several erase
blocks.  Whenever such a thing comes up, refuse the mount and error
out.

But then again, I just couldn't care less.  So someone else please
provide the code.

Jörn

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

Re: JFFS2 eraseblock header

[Josh Boyer]

On Thu, 2005-09-08 at 20:43 +0200, Jörn Engel wrote:
> On Thu, 8 September 2005 17:35:15 +0400, Artem B. Bityuckiy wrote:
> > David Woodhouse wrote:
> >
> > >Should it? I wonder if it should silently upgrade, instead? Or at least
> > >have a mount option to make it upgrade? We want to leave the old format
> > >behind...
> >
> > That would be spiffy to leave it behind and save a lot efforts.
> > But in this case new JFFS2 binaries will reject mounting old JFFS2 images.
> > If this is accaptable - fine. Personally I don't care, but people may.
> > :-)
> 
> Current strategy is to push the effort to the user.  Not perfect, but
> it is necessary to inform the users about the problem anyway.
> 
> What we could additionally do is detect nodes that span several erase
> blocks.  Whenever such a thing comes up, refuse the mount and error
> out.

In any case, would you please bump the overall version of JFFS2 if this
change is made?  I don't mean "JFFS3", I mean "JFFS2 version 2.3" or
similar.  See super.c line 320 for where I'm talking about.

That will at least give the users some indication that things have
changed.  Or rather, it will give the JFFS2 developers a hint when users
report problems and they can use a canned response in the FAQ ;).

josh

Re: JFFS2 eraseblock header

[Artem B. Bityuckiy]

Josh Boyer wrote:
> 
> In any case, would you please bump the overall version of JFFS2 if this
> change is made?  I don't mean "JFFS3", I mean "JFFS2 version 2.3" or
> similar.  See super.c line 320 for where I'm talking about.
Agree, this should be done. I also offered to introduce a version number 
byte to the erasebloch header. This is somewhat ugly but will help to 
quickly recognize the JFFS2 image version. In future this will help us.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: JFFS2 eraseblock header

[Jörn Engel]

On Fri, 9 September 2005 17:08:28 +0400, Artem B. Bityuckiy wrote:
> Josh Boyer wrote:
> >
> >In any case, would you please bump the overall version of JFFS2 if this
> >change is made?  I don't mean "JFFS3", I mean "JFFS2 version 2.3" or
> >similar.  See super.c line 320 for where I'm talking about.
> Agree, this should be done. I also offered to introduce a version number 
> byte to the erasebloch header. This is somewhat ugly but will help to 
> quickly recognize the JFFS2 image version. In future this will help us.

All right, you win.  Can you bump the version to 2.3?

Jörn

-- 
tglx1 thinks that joern should get a (TM) for "Thinking Is Hard"
-- Thomas Gleixner

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Jörn Engel]

On Mon, 15 August 2005 15:07:31 +0400, Artem B. Bityuckiy wrote:
> 
> Ok, to keep you busy with some technical stuff also, here is a remark.
> 
> >+		if (je32_to_cpu(sm->magic) == JFFS2_SUM_MAGIC ) {
> >+
> >+			if(je32_to_cpu(sm->erase_size) == c->sector_size) {
> >+				err = 
> >jffs2_sum_scan_sumnode(c,jeb,je32_to_cpu(sm->offset),&pseudo_random);
> >+
> >+				if (err) {
> >+					kfree(sm);
> >+					return err;
> >+				}
> >+			}
> >+			printk(KERN_WARNING "FS erase_block_size != JFFS2 
> erase_block_size => skipping summary information\n");
> >+		}
> 
> if(je32_to_cpu(sm->erase_size) == c->sector_size) { ...
> 
> AFAIU, you're protecting against the situation when the size of the 
> JFFS2 virtual eraseblock is changed.
> 
> But why is the eraseblock size may change? If it becomes less, this is a 
> problem since some nodes that cross the boundary will be ignored. If the 
> size of the JFFS2 virtual eraseblock becomes larger, it is also a 
> problem - due to present bad blocks, some eraseblocks with valid data 
> will be ignored (remember, if there is a bad eraseblock in the JFFS2 
> virtual eraseblock, the whole virtual eraseblock is regarded as bad).
> 
> Thus, please, don't solve this problem this way. You're trying to solve 
> it privately for your summaries, while you have to solve it for *JFFS2 
> in general* (if it exists at all). I suppose it exists in your case 
> since you add one more field to struct eraseblock...

Agreed.  Best place to put the check would be in
jffs2_do_fill_super(), about here:

	c->sector_size = c->mtd->erasesize; 
	blocks = c->flash_size / c->sector_size;
	if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
		while ((blocks * sizeof (struct jffs2_eraseblock))
				> (128 * 1024)) {
			blocks >>= 1;
			c->sector_size <<= 1;
		}	
	}

Imo, this block growing is an ugly hack.  It defeats the ability to
grow/shrink your partition on the fly.  Worse, it makes
growing/shrinking fail only rarely, while it usually just works.

Anyway.  Once the size of erase blocks changes, we're basically
fucked - even without summary.  So we want that check unconditionally.
Of course, we don't have any nodes containing the erase_size yet.
Hmm.

David, my best idea right now would be to change the erase marker and
add a field to it.  Doesn't really make me happy, though.  Do you have
a better idea?

And while we're at it, why not create a new function for this code.
Something like this:

int jffs2_check_erase_size(struct jffs2_sb_info *c)
{
	size_t blocks;

	c->sector_size = c->mtd->erasesize; 
	blocks = c->flash_size / c->sector_size;
	if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
		while ((blocks * sizeof (struct jffs2_eraseblock))
				> (128 * 1024)) {
			blocks >>= 1;
			c->sector_size <<= 1;
		}	
	}

	if (/* magic check about changed erase_size fails */)
		return -EIO;
	return 0;
}

Jörn

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

Re: [PATCH]fs/jffs2/wbuf.c: add compatibility support for OOB data block

[Artem B. Bityuckiy]

On Mon, 2005-08-15 at 13:53 +0200, Jörn Engel wrote:

> David, my best idea right now would be to change the erase marker and
> add a field to it.  Doesn't really make me happy, though.  Do you have
> a better idea?
> 
> And while we're at it, why not create a new function for this code.
> Something like this:
> 
> int jffs2_check_erase_size(struct jffs2_sb_info *c)
> {
> 	size_t blocks;
> 
> 	c->sector_size = c->mtd->erasesize; 
> 	blocks = c->flash_size / c->sector_size;
> 	if (!(c->mtd->flags & MTD_NO_VIRTBLOCKS)) {
> 		while ((blocks * sizeof (struct jffs2_eraseblock))
> 				> (128 * 1024)) {
> 			blocks >>= 1;
> 			c->sector_size <<= 1;
> 		}	
> 	}
> 
> 	if (/* magic check about changed erase_size fails */)
> 		return -EIO;
> 	return 0;
> }

If we will have the same algorithm for calculating the size of virtual
block both in JFFS2 and in sumtool, the problem should go, isn't it?
User supplies sumtool with size of the eraseblock, sumtool knows the
size of struct jffs2_eraseblock, so it knows the virtual block size and
we're happy.

Jffs2 problem with Versatile PB926EJ-S

[Soma sundaram Veerappan]

Hi,

I am working with Versatile PB926EJ-S.I am trying
porting  jffs2 with Linux-2.6.12.

I am getting 'No space left on device' message when i
 try to remove to create files.

I got reply from another mailing list to use below
command like below.

mkfs.jffs2 --eraseblock=0x40000 --pad=10485760 \
        -d your-filesystem-dir > filesystem.jffs2

I used below command to make 10MB flash image

mkfs.jffs2 -l --root=image --eraseblock=0x040000
--no-cleanmarkers --pad=10485760 -o flashdisk_jffs2 -D
devices.txt 

When i passed this image size to linux from u-boot
bootargs, i got following messages

Erase at 0x00240000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00200000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00980000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00940000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00900000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x008c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00880000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00840000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00800000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x007c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00780000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00740000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00700000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x006c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00680000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00640000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00600000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x005c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00580000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00540000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00500000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x004c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00480000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00440000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00400000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x003c0000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00380000 failed immediately: -EROFS. Is the
sector locked?
Erase at 0x00340000 failed immediately: -EROFS. Is the
sector locked?

Any help on this regard is highly appreciated.

Regards,


		
__________________________________________________________
How much free photo storage do you get? Store your friends 'n family snaps for FREE with Yahoo! Photos http://in.photos.yahoo.com

Re: Jffs2 problem with Versatile PB926EJ-S

[Todd Poynor]

Soma sundaram Veerappan wrote:
> Hi,
> 
> I am working with Versatile PB926EJ-S.I am trying
> porting  jffs2 with Linux-2.6.12.
> 
> I am getting 'No space left on device' message when i
>  try to remove to create files.
> 
> I got reply from another mailing list to use below
> command like below.
...
> 
> When i passed this image size to linux from u-boot
> bootargs, i got following messages
> 
> Erase at 0x00240000 failed immediately: -EROFS. Is the
> sector locked?

I also replied on that mailing list regarding the above.

-- 
Todd