[PATCH]erase block header(revision 3)

[PATCH]erase block header(revision 3)

[zhao forrest]

Hi,

This is the revision 3 of erase block header patch.
Compare with revision 2, the main changes are:
1 to keep consistency, rename struct jffs2_eraseblock_header
to struct jffs2_raw_ebh
2 change the type of has_ebh to uint32_t
3 store the real length into totlen
4 rename fs_version into reserved in order to give it a
useful name.

The patch is at:
http://www.infradead.org/~forrest/eraseblock_header_r3.patch

Thanks,
Forrest

Re: [PATCH]erase block header(revision 3)

[Jörn Engel]

On Tue, 27 September 2005 15:51:42 +0800, zhao forrest wrote:
> 
> This is the revision 3 of erase block header patch.
> Compare with revision 2, the main changes are:
> 1 to keep consistency, rename struct jffs2_eraseblock_header
> to struct jffs2_raw_ebh
> 2 change the type of has_ebh to uint32_t
> 3 store the real length into totlen
> 4 rename fs_version into reserved in order to give it a
> useful name.


> diff -auNrp ./mtd/fs/jffs2/erase.c ./mtd_EBH_EBS/fs/jffs2/erase.c
> --- ./mtd/fs/jffs2/erase.c	2005-09-22 11:08:47.000000000 +0800
> +++ ./mtd_EBH_EBS/fs/jffs2/erase.c	2005-09-27 15:30:26.000000000 +0800
> @@ -210,6 +210,10 @@ static void jffs2_erase_callback(struct 
>  	} else {
>  		jffs2_erase_succeeded(priv->c, priv->jeb);
>  	}	
> +
> +	if (!priv->jeb->has_ebh) {
> +		priv->jeb->has_ebh = 1;
> +	}

The complete condition is bullock.  See last review.

>  	kfree(instr);
>  }
>  #endif /* !__ECOS */

[...]

> +struct jffs2_raw_ebh
> +{
> +	jint16_t magic;
> +	jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */
> +	jint32_t totlen;
> +	jint32_t hdr_crc;
> +	uint8_t  reserved; /* reserved for future use and alignment */
> +	uint8_t  compat_fset;
> +	uint8_t  incompat_fset;
> +	uint8_t  rocompat_fset;
> +	jint32_t erase_count; /* the erase count of this erase block */
> +	jint16_t dsize; /* the size of additional data behind node_crc */
> +	jint32_t node_crc;
> +	jint32_t data_crc;
> +	jint32_t data[0];
> +} __attribute__((packed));

Structure still doesn't make sense.  See my last review.

Jörn

-- 
Invincibility is in oneself, vulnerability is in the opponent.
-- Sun Tzu

Re: [PATCH]erase block header(revision 3)

[Artem B. Bityutskiy]

zhao forrest wrote:
> Hi,
> 
> This is the revision 3 of erase block header patch.
> Compare with revision 2, the main changes are:
> 1 to keep consistency, rename struct jffs2_eraseblock_header
> to struct jffs2_raw_ebh
> 2 change the type of has_ebh to uint32_t
> 3 store the real length into totlen
> 4 rename fs_version into reserved in order to give it a
> useful name.

Here is my review. Sure, not full yet.

1. The patch is not against the latest MTD.

Hunk #4 FAILED at 191.
1 out of 4 hunks FAILED -- saving rejects to file include/linux/jffs2.h.rej


2. The patch is not applied with 'patch -p1', use

"diff -auNrp mtd/ mtd_EBH_EBS/", not
"diff -auNrp ./mtd/ ./mtd_EBH_EBS/"

3. Really _very_ minor. Could you please add the definition of struct
jffs2_raw_ebh in jffs2.h after the definitions of other nodes, not before.

4. From your patch:
----------------------------
+
+	if (!priv->jeb->has_ebh) {
+		priv->jeb->has_ebh = 1;
+	}
----------------------------
a. Joern already pointed, just set it unconditionally.
b. This flag is set in a function which does not exist in eCos. eCos
will be broken. I would move this to jffs2_erase_succeeded().


5. From your patch:
----------------------------
 		struct kvec vecs[1];
-		struct jffs2_unknown_node marker = {
+		struct jffs2_raw_ebh marker = {
 			.magic =	cpu_to_je16(JFFS2_MAGIC_BITMASK),
-			.nodetype =	cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER),
-			.totlen =	cpu_to_je32(c->cleanmarker_size)
+			.nodetype =	cpu_to_je16(JFFS2_NODETYPE_ERASEBLOCK_HEADER),
+			.totlen =	cpu_to_je32(sizeof(struct jffs2_raw_ebh)),
+			.reserved =     0,
+			.compat_fset =  JFFS2_EBH_COMPAT_FSET,
+			.incompat_fset = JFFS2_EBH_INCOMPAT_FSET,
+			.rocompat_fset = JFFS2_EBH_ROCOMPAT_FSET,
+			.erase_count =  cpu_to_je32(jeb->erase_count),
+			.dsize =        cpu_to_je16(0),
+			.data_crc =     cpu_to_je32(0)
 		};
----------------------------
I understand that you just do things as JFFS2 does. But there is a
better way. IMO, there is no need to define the EBH object straight in
jffs2_mark_erased_block(). Define it at the top of erase.c.

Furthermore, the same static object is initialized in
jffs2_write_nand_ebh() (wbuf.c). Please, share the same object.

I would also move jffs2_write_nand_ebh() to erase.c.

And please, for readability, don't name this structure 'marker' ...


6. From your patch:
----------------------------
 	c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
+	c->ebh_size = sizeof(struct jffs2_raw_ebh);
----------------------------
Use PAD() here, and don't use PAD(c->ebh_size) elsewhere.

And could you please add a comment at the definition of struct
jffs2_eraseblock which will clarify what is ebh_size?


7. From your patch:
----------------------------
@@ -196,6 +196,9 @@ struct jffs2_eraseblock
 	struct jffs2_raw_node_ref *last_node;

 	struct jffs2_raw_node_ref *gc_node;	/* Next node to be garbage
collected */
+
+	uint32_t has_ebh;
+	uint32_t erase_count;
 };
----------------------------
struct jffs2_eraseblock has an 'int bad_count' field which actually
stores only small values. To save some memory I would better do:
- int bad_count;
+ uint16_t bad_count
+ uint16_t flags;

and add a JFFS2_EBFLAGS_HAS_EBH flag instead of has_ebh field. This
would save 4 bytes for each 'struct jffs2_eraseblock' object.

8. From your patch:
----------------------------
+		if ((!jeb->has_ebh && jeb->free_size != c->sector_size -
c->cleanmarker_size) ||
+		    (jeb->has_ebh && c->ebh_size && jeb->free_size != c->sector_size
- jeb->first_node->__totlen) ||
+		    (jeb->has_ebh && !c->ebh_size && jeb->free_size != c->sector_size))
----------------------------
See the previous review about __totlen usage. use ref_totlen() instead.

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

Re: [PATCH]erase block header(revision 3)

[Artem B. Bityutskiy]

zhao forrest wrote:
> Hi,
> 
> This is the revision 3 of erase block header patch.
BTW, did you realize that you should update MTD tools accordingly as well?

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

Re: [PATCH]erase block header(revision 3)

[zhao forrest]

>
>zhao forrest wrote:
> > Hi,
> >
> > This is the revision 3 of erase block header patch.
>BTW, did you realize that you should update MTD tools accordingly as well?
>
>--

Yes, this has been on my TODO list. I think I should update them
after the patch for JFFS2 kernel module is checked into MTD CVS.
Does it make sense to you?

Thanks,
Forrest

Re: [PATCH]erase block header(revision 3)

[zhao forrest]

>Here is my review. Sure, not full yet.
>
>1. The patch is not against the latest MTD.
>
>Hunk #4 FAILED at 191.
>1 out of 4 hunks FAILED -- saving rejects to file 
include/linux/jffs2.h.rej
>
The revision 4 will be based on latest MTD CVS.
>
>2. The patch is not applied with 'patch -p1', use
>
>"diff -auNrp mtd/ mtd_EBH_EBS/", not
>"diff -auNrp ./mtd/ ./mtd_EBH_EBS/"
OK.

>
>3. Really _very_ minor. Could you please add the definition of struct
>jffs2_raw_ebh in jffs2.h after the definitions of other nodes, not before.
OK.

>4. From your patch:
>----------------------------
>+
>+	if (!priv->jeb->has_ebh) {
>+		priv->jeb->has_ebh = 1;
>+	}
>----------------------------
>a. Joern already pointed, just set it unconditionally.
>b. This flag is set in a function which does not exist in eCos. eCos
>will be broken. I would move this to jffs2_erase_succeeded().
OK.

>
>5. From your patch:
>----------------------------
>  		struct kvec vecs[1];
>-		struct jffs2_unknown_node marker = {
>+		struct jffs2_raw_ebh marker = {
>  			.magic =	cpu_to_je16(JFFS2_MAGIC_BITMASK),
>-			.nodetype =	cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER),
>-			.totlen =	cpu_to_je32(c->cleanmarker_size)
>+			.nodetype =	cpu_to_je16(JFFS2_NODETYPE_ERASEBLOCK_HEADER),
>+			.totlen =	cpu_to_je32(sizeof(struct jffs2_raw_ebh)),
>+			.reserved =     0,
>+			.compat_fset =  JFFS2_EBH_COMPAT_FSET,
>+			.incompat_fset = JFFS2_EBH_INCOMPAT_FSET,
>+			.rocompat_fset = JFFS2_EBH_ROCOMPAT_FSET,
>+			.erase_count =  cpu_to_je32(jeb->erase_count),
>+			.dsize =        cpu_to_je16(0),
>+			.data_crc =     cpu_to_je32(0)
>  		};
>----------------------------
>I understand that you just do things as JFFS2 does. But there is a
>better way. IMO, there is no need to define the EBH object straight in
>jffs2_mark_erased_block(). Define it at the top of erase.c.
>
>Furthermore, the same static object is initialized in
>jffs2_write_nand_ebh() (wbuf.c). Please, share the same object.
OK.

>I would also move jffs2_write_nand_ebh() to erase.c.
But when jffs2_write_nand_ebh() is put in wbuf.c, it will not be
compiled when CONFIG_JFFS2_FS_WRITEBUFFER is not defined.
I think it's better to put it in wbuf.c since it's the function
for wbuf. Make sense?


>And please, for readability, don't name this structure 'marker' ...
OK.

>6. From your patch:
>----------------------------
>  	c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
>+	c->ebh_size = sizeof(struct jffs2_raw_ebh);
>----------------------------
>Use PAD() here, and don't use PAD(c->ebh_size) elsewhere.
>
OK.

>And could you please add a comment at the definition of struct
>jffs2_eraseblock which will clarify what is ebh_size?
OK.

>7. From your patch:
>----------------------------
>@@ -196,6 +196,9 @@ struct jffs2_eraseblock
>  	struct jffs2_raw_node_ref *last_node;
>
>  	struct jffs2_raw_node_ref *gc_node;	/* Next node to be garbage
>collected */
>+
>+	uint32_t has_ebh;
>+	uint32_t erase_count;
>  };
>----------------------------
>struct jffs2_eraseblock has an 'int bad_count' field which actually
>stores only small values. To save some memory I would better do:
>- int bad_count;
>+ uint16_t bad_count
>+ uint16_t flags;
>
>and add a JFFS2_EBFLAGS_HAS_EBH flag instead of has_ebh field. This
>would save 4 bytes for each 'struct jffs2_eraseblock' object.
OK. 

>8. From your patch:
>----------------------------
>+		if ((!jeb->has_ebh && jeb->free_size != c->sector_size -
>c->cleanmarker_size) ||
>+		    (jeb->has_ebh && c->ebh_size && jeb->free_size != c->sector_size
>- jeb->first_node->__totlen) ||
>+		    (jeb->has_ebh && !c->ebh_size && jeb->free_size != c->sector_size))
>----------------------------
>See the previous review about __totlen usage. use ref_totlen() instead.
>
OK.

I'll send out the revised patch soon.

Thanks,
Forrest