[PATCH]erase block header(revision 4)

[PATCH]erase block header(revision 4)

[zhao forrest]

Hi,
This is the forth version. Compared with version 3, the
main changes are:
1 move "priv->jeb->has_ebh = 1;" from jffs2_erase_callback()
to jffs2_erase_succeeded() to prevent eCos from being broken.
2 define ebh at the top of erase.c and this ebh is shared
by jffs2_write_nand_ebh() in wbuf.c
3 remove unnecessary PAD() stuff
4 change has_ebh from uint32_t to one bit of flags
5 remove unnecessary dsize field from struct jffs2_raw_ebh{}
6 use ref_totlen() instead of directly getting __totlen.

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

Thanks,
Forrest

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

[root]

On Wed, 2005-09-28 at 15:33 +0800, zhao forrest wrote:
> Hi,
> This is the forth version. Compared with version 3, the
> main changes are:
> 1 move "priv->jeb->has_ebh = 1;" from jffs2_erase_callback()
> to jffs2_erase_succeeded() to prevent eCos from being broken.
> 2 define ebh at the top of erase.c and this ebh is shared
> by jffs2_write_nand_ebh() in wbuf.c
> 3 remove unnecessary PAD() stuff
> 4 change has_ebh from uint32_t to one bit of flags
> 5 remove unnecessary dsize field from struct jffs2_raw_ebh{}
> 6 use ref_totlen() instead of directly getting __totlen.
> 
> The patch is at:
> http://www.infradead.org/~forrest/eraseblock_header_r4.patch
> 

I would assume the patch is accepted if there're no further
review comments for it........

Thanks,
Forrest

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

[Jörn Engel]

On Fri, 30 September 2005 09:15:29 +0800, root wrote:
> 
> I would assume the patch is accepted if there're no further
> review comments for it........

No comment merely means no comment.  I didn't find the time to look at
it yet.

Jörn

-- 
Ninety percent of everything is crap.
-- Sturgeon's Law

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

[Jörn Engel]

On Wed, 28 September 2005 15:33:46 +0800, zhao forrest wrote:
> 
> This is the forth version. Compared with version 3, the
> main changes are:
> 1 move "priv->jeb->has_ebh = 1;" from jffs2_erase_callback()
> to jffs2_erase_succeeded() to prevent eCos from being broken.
> 2 define ebh at the top of erase.c and this ebh is shared
> by jffs2_write_nand_ebh() in wbuf.c
> 3 remove unnecessary PAD() stuff
> 4 change has_ebh from uint32_t to one bit of flags
> 5 remove unnecessary dsize field from struct jffs2_raw_ebh{}
> 6 use ref_totlen() instead of directly getting __totlen.


> diff -auNrp mtd_9_28/fs/jffs2/erase.c mtd_9_28_EBH/fs/jffs2/erase.c
> --- mtd_9_28/fs/jffs2/erase.c	2005-09-28 10:51:57.000000000 +0800
> +++ mtd_9_28_EBH/fs/jffs2/erase.c	2005-09-28 14:38:00.000000000 +0800
> @@ -24,7 +24,18 @@ struct erase_priv_struct {
>  	struct jffs2_eraseblock *jeb;
>  	struct jffs2_sb_info *c;
>  };
> -      
> +
> +struct jffs2_raw_ebh ebh = {
> +	.magic =        cpu_to_je16(JFFS2_MAGIC_BITMASK),
> +	.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,
> +	.data_crc =     cpu_to_je32(0)
> +};

I don't like this.  A global variable (without locking, it appears),
not declared as static and with a 3-letter name.

You could use it as a prototype, but even that seems a bit too much
for 5 constant fields.  And as long as the struct jffs2_raw_ebh is
small enough, you can just allocate it on the stack.

> @@ -210,6 +222,7 @@ static void jffs2_erase_callback(struct 
>  	} else {
>  		jffs2_erase_succeeded(priv->c, priv->jeb);
>  	}	
> +
>  	kfree(instr);
>  }
>  #endif /* !__ECOS */

Please remove.

> @@ -351,7 +364,7 @@ fail:
>  
>  static void jffs2_mark_erased_block(struct jffs2_sb_info *c, struct jffs2_eraseblock *jeb)
>  {
> -	struct jffs2_raw_node_ref *marker_ref = NULL;
> +	struct jffs2_raw_node_ref *ebh_ref = NULL;
>  	size_t retlen;
>  	int ret;
>  	uint32_t bad_offset;
> @@ -362,16 +375,14 @@ static void jffs2_mark_erased_block(stru
>  	}
>  
>  	/* Write the erase complete marker */	
> -	D1(printk(KERN_DEBUG "Writing erased marker to block at 0x%08x\n", jeb->offset));
> +	D1(printk(KERN_DEBUG "Writing eraseblock header to block at 0x%08x\n", jeb->offset));
>  	bad_offset = jeb->offset;
>  
>  	/* Cleanmarker in oob area or no cleanmarker at all ? */
> -	if (jffs2_cleanmarker_oob(c) || c->cleanmarker_size == 0) {
> +	if (jffs2_cleanmarker_oob(c)) {

What was (c->cleanmarker_size == 0) used for?

> -		if (jffs2_cleanmarker_oob(c)) {
> -			if (jffs2_write_nand_cleanmarker(c, jeb))
> -				goto filebad;
> -		}
> +		if (jffs2_write_nand_ebh(c, jeb))
> +			goto filebad;
>  
>  		jeb->first_node = jeb->last_node = NULL;
>  		jeb->free_size = c->sector_size;
> @@ -382,45 +393,41 @@ static void jffs2_mark_erased_block(stru
>  	} else {
>  
>  		struct kvec vecs[1];
> -		struct jffs2_unknown_node marker = {
> -			.magic =	cpu_to_je16(JFFS2_MAGIC_BITMASK),
> -			.nodetype =	cpu_to_je16(JFFS2_NODETYPE_CLEANMARKER),
> -			.totlen =	cpu_to_je32(c->cleanmarker_size)
> -		};

Good example of stack allocation.

> -		marker_ref = jffs2_alloc_raw_node_ref();
> -		if (!marker_ref) {
> +		ebh_ref = jffs2_alloc_raw_node_ref();
> +		if (!ebh_ref) {
>  			printk(KERN_WARNING "Failed to allocate raw node ref for clean marker. Refiling\n");
>  			goto refile;
>  		}
> +		ebh.erase_count =  cpu_to_je32(jeb->erase_count);
> +		ebh.hdr_crc = cpu_to_je32(crc32(0, &ebh, sizeof(struct jffs2_unknown_node)-4));
> +		ebh.node_crc = cpu_to_je32(crc32(0, &ebh, sizeof(struct jffs2_raw_ebh)-8));
>  
> -		marker.hdr_crc = cpu_to_je32(crc32(0, &marker, sizeof(struct jffs2_unknown_node)-4));
> -
> -		vecs[0].iov_base = (unsigned char *) ▮
> -		vecs[0].iov_len = sizeof(marker);
> +		vecs[0].iov_base = (unsigned char *) &ebh;
> +		vecs[0].iov_len = sizeof(ebh);
>  		ret = jffs2_flash_direct_writev(c, vecs, 1, jeb->offset, &retlen);
>  		
> -		if (ret || retlen != sizeof(marker)) {
> +		if (ret || retlen != sizeof(ebh)) {
>  			if (ret)
> -				printk(KERN_WARNING "Write clean marker to block at 0x%08x failed: %d\n",
> +				printk(KERN_WARNING "Write eraseblock header to block at 0x%08x failed: %d\n",
>  				       jeb->offset, ret);
>  			else
>  				printk(KERN_WARNING "Short write to newly-erased block at 0x%08x: Wanted %zd, got %zd\n",
> -				       jeb->offset, sizeof(marker), retlen);
> +				       jeb->offset, sizeof(ebh), retlen);
>  
> -			jffs2_free_raw_node_ref(marker_ref);
> +			jffs2_free_raw_node_ref(ebh_ref);
>  			goto filebad;
>  		}

Most of this can remain unchanged with stack allocations.

> diff -auNrp mtd_9_28/fs/jffs2/fs.c mtd_9_28_EBH/fs/jffs2/fs.c
> --- mtd_9_28/fs/jffs2/fs.c	2005-09-28 10:51:57.000000000 +0800
> +++ mtd_9_28_EBH/fs/jffs2/fs.c	2005-09-28 11:51:53.000000000 +0800
> @@ -476,6 +476,7 @@ int jffs2_do_fill_super(struct super_blo
>  	}
>  
>  	c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
> +	c->ebh_size = PAD(sizeof(struct jffs2_raw_ebh));

Remove the PAD.  Instead you should make sure that PAD(sizeof(struct
jffs2_raw_ebh)) and sizeof(struct jffs2_raw_ebh) are equal.

If they aren't, you have some other problem anyway.

> @@ -196,8 +197,14 @@ 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 erase_count;
>  };
>  
> +#define SET_EBFLAGS_HAS_EBH(jeb) (jeb->flags |= 1)
> +#define CLR_EBFLAGS_HAS_EBH(jeb) (jeb->flags &= ~1)
> +#define EBFLAGS_HAS_EBH(jeb) ((jeb->flags & 1) == 1)

SET_EBFLAGS_HAS_EBH and CLR_EBFLAGS_HAS_EBH don't make sense.
EBFLAGS_SET_EBH does.

>  static inline int jffs2_blocks_use_vmalloc(struct jffs2_sb_info *c)
>  {
>  	return ((c->flash_size / c->sector_size) * sizeof (struct jffs2_eraseblock)) > (128 * 1024);
> @@ -398,14 +405,15 @@ int jffs2_scan_classify_jeb(struct jffs2
>  int jffs2_do_mount_fs(struct jffs2_sb_info *c);
>  
>  /* erase.c */
> +extern struct jffs2_raw_ebh ebh;

If you need this, you have a bug.

> diff -auNrp mtd_9_28/fs/jffs2/nodemgmt.c mtd_9_28_EBH/fs/jffs2/nodemgmt.c
> --- mtd_9_28/fs/jffs2/nodemgmt.c	2005-09-28 10:51:57.000000000 +0800
> +++ mtd_9_28_EBH/fs/jffs2/nodemgmt.c	2005-09-28 13:42:53.000000000 +0800
> @@ -342,7 +342,10 @@ static int jffs2_do_reserve_space(struct
>  
>  		jeb = c->nextblock;
>  
> -		if (jeb->free_size != c->sector_size - c->cleanmarker_size) {
> +		if ((!EBFLAGS_HAS_EBH(jeb) && jeb->free_size != c->sector_size - c->cleanmarker_size) ||
> +		    (EBFLAGS_HAS_EBH(jeb) && c->ebh_size && jeb->free_size != c->sector_size - ref_totlen(c, jeb, jeb->first_node)) ||
> +		    (EBFLAGS_HAS_EBH(jeb) && !c->ebh_size && jeb->free_size != c->sector_size))
> +		{

This is too complicated.  What are you trying to do?

>  			printk(KERN_WARNING "Eep. Block 0x%08x taken from free_list had free_size of 0x%08x!!\n", jeb->offset, jeb->free_size);
>  			goto restart;
>  		}
> @@ -352,8 +355,10 @@ static int jffs2_do_reserve_space(struct
>  	*ofs = jeb->offset + (c->sector_size - jeb->free_size);
>  	*len = jeb->free_size - reserved_size;
>  
> -	if (c->cleanmarker_size && jeb->used_size == c->cleanmarker_size &&
> -	    !jeb->first_node->next_in_ino) {
> +	if ((!EBFLAGS_HAS_EBH(jeb) && c->cleanmarker_size && jeb->used_size == c->cleanmarker_size
> +	     && !jeb->first_node->next_in_ino)
> +	     || (EBFLAGS_HAS_EBH(jeb) && c->ebh_size && jeb->used_size == ref_totlen(c, jeb, jeb->first_node)
> +	     && !jeb->first_node->next_in_ino)) {

Same thing.

[...]

> +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 */
> +	jint32_t node_crc;
> +	jint32_t data_crc;

You don't need three crc fields.  Move node_crc up just below hdr_crc
and calculate the crc over the complete header.  If it is later
extended, you know the length from totlen, so you can still check the
complete crc, even if you don't understand the fields below.

> +	jint32_t data[0];
> +} __attribute__((packed));
> +
> +
>  union jffs2_node_union 
>  {
>  	struct jffs2_raw_inode i;
>  	struct jffs2_raw_dirent d;
>  	struct jffs2_raw_summary s;
> +	struct jffs2_raw_ebh eh;
>  	struct jffs2_unknown_node u;
>  };

Jörn

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

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

[zhao forrest]

> > -
> > +
> > +struct jffs2_raw_ebh ebh = {
> > +	.magic =        cpu_to_je16(JFFS2_MAGIC_BITMASK),
> > +	.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,
> > +	.data_crc =     cpu_to_je32(0)
> > +};
>
>I don't like this.  A global variable (without locking, it appears),
>not declared as static and with a 3-letter name.
>
>You could use it as a prototype, but even that seems a bit too much
>for 5 constant fields.  And as long as the struct jffs2_raw_ebh is
>small enough, you can just allocate it on the stack.
>
Artem asked me to remove allocation on the stack, so who should I 
listen to? This really made me very very confused :(


>
> > @@ -351,7 +364,7 @@ fail:
> >
> >  static void jffs2_mark_erased_block(struct jffs2_sb_info *c, struct 
jffs2_eraseblock *jeb)
> >  {
> > -	struct jffs2_raw_node_ref *marker_ref = NULL;
> > +	struct jffs2_raw_node_ref *ebh_ref = NULL;
> >  	size_t retlen;
> >  	int ret;
> >  	uint32_t bad_offset;
> > @@ -362,16 +375,14 @@ static void jffs2_mark_erased_block(stru
> >  	}
> >
> >  	/* Write the erase complete marker */
> > -	D1(printk(KERN_DEBUG "Writing erased marker to block at 0x%08x\n", 
jeb->offset));
> > +	D1(printk(KERN_DEBUG "Writing eraseblock header to block at 
0x%08x\n", jeb->offset));
> >  	bad_offset = jeb->offset;
> >
> >  	/* Cleanmarker in oob area or no cleanmarker at all ? */
> > -	if (jffs2_cleanmarker_oob(c) || c->cleanmarker_size == 0) {
> > +	if (jffs2_cleanmarker_oob(c)) {
>
>What was (c->cleanmarker_size == 0) used for?
For the flash that don't need a clean marker.


> > diff -auNrp mtd_9_28/fs/jffs2/fs.c mtd_9_28_EBH/fs/jffs2/fs.c
> > --- mtd_9_28/fs/jffs2/fs.c	2005-09-28 10:51:57.000000000 +0800
> > +++ mtd_9_28_EBH/fs/jffs2/fs.c	2005-09-28 11:51:53.000000000 +0800
> > @@ -476,6 +476,7 @@ int jffs2_do_fill_super(struct super_blo
> >  	}
> >
> >  	c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
> > +	c->ebh_size = PAD(sizeof(struct jffs2_raw_ebh));
>
>Remove the PAD.  Instead you should make sure that PAD(sizeof(struct
>jffs2_raw_ebh)) and sizeof(struct jffs2_raw_ebh) are equal.
>
>If they aren't, you have some other problem anyway.

I don't see a problem here, could you explain what kind of
problem I will have??

> > @@ -196,8 +197,14 @@ 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 erase_count;
> >  };
> >
> > +#define SET_EBFLAGS_HAS_EBH(jeb) (jeb->flags |= 1)
> > +#define CLR_EBFLAGS_HAS_EBH(jeb) (jeb->flags &= ~1)
> > +#define EBFLAGS_HAS_EBH(jeb) ((jeb->flags & 1) == 1)
>
>SET_EBFLAGS_HAS_EBH and CLR_EBFLAGS_HAS_EBH don't make sense.
>EBFLAGS_SET_EBH does.
Sorry. I can't understand why EBFLAGS_SET_EBH make sense.
This flag is used to indicate whether an erase block has the
eraseblock header. From EBFLAGS_SET_EBH code reader can't
associate it with "_has_ eraseblock header".
I think the flag's name is has_ebh instead of ebh.

>
> > diff -auNrp mtd_9_28/fs/jffs2/nodemgmt.c 
mtd_9_28_EBH/fs/jffs2/nodemgmt.c
> > --- mtd_9_28/fs/jffs2/nodemgmt.c	2005-09-28 10:51:57.000000000 +0800
> > +++ mtd_9_28_EBH/fs/jffs2/nodemgmt.c	2005-09-28 13:42:53.000000000 
+0800
> > @@ -342,7 +342,10 @@ static int jffs2_do_reserve_space(struct
> >
> >  		jeb = c->nextblock;
> >
> > -		if (jeb->free_size != c->sector_size - c->cleanmarker_size) {
> > +		if ((!EBFLAGS_HAS_EBH(jeb) && jeb->free_size != c->sector_size - 
c->cleanmarker_size) ||
> > +		    (EBFLAGS_HAS_EBH(jeb) && c->ebh_size && jeb->free_size != 
c->sector_size - ref_totlen(c, jeb, jeb->first_node)) ||
> > +		    (EBFLAGS_HAS_EBH(jeb) && !c->ebh_size && jeb->free_size != 
c->sector_size))
> > +		{
>
>This is too complicated.  What are you trying to do?
I don't think it too complicated. It's only the extension of original
condition judgement. The aim is that if free_size is not the expected
size, then goto restart......


>
> >  			printk(KERN_WARNING "Eep. Block 0x%08x taken from free_list had 
free_size of 0x%08x!!\n", jeb->offset, jeb->free_size);
> >  			goto restart;
> >  		}
> > +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 */
> > +	jint32_t node_crc;
> > +	jint32_t data_crc;
>
>You don't need three crc fields.  Move node_crc up just below hdr_crc
>and calculate the crc over the complete header.  If it is later
>extended, you know the length from totlen, so you can still check the
>complete crc, even if you don't understand the fields below.
>
OK

Thanks,
Forrest

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

[Artem B. Bityutskiy]

zhao forrest wrote:
> Artem asked me to remove allocation on the stack, so who should I listen 
> to? This really made me very very confused :(
Yes, I suggested to share one static clean-marker structure for both
NAND and NOR cases.

>> What was (c->cleanmarker_size == 0) used for?
> 
> For the flash that don't need a clean marker.
No, that worked for NAND this way (c->cleanmarker_size was 0)

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

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

[Jörn Engel]

On Mon, 3 October 2005 17:50:14 +0400, Artem B. Bityutskiy wrote:
> zhao forrest wrote:
> > Artem asked me to remove allocation on the stack, so who should I listen 
> > to? This really made me very very confused :(
> Yes, I suggested to share one static clean-marker structure for both
> NAND and NOR cases.

Bad suggestion then.  Try to draw a complete graph of all possible
users to this static structure, the locking required to make it
correct, then prove its correctness.

If that didn't already scare you to death, try to anticipate future
code changes to such brittle code.

In one word: don't.

> >> What was (c->cleanmarker_size == 0) used for?
> > 
> > For the flash that don't need a clean marker.
> No, that worked for NAND this way (c->cleanmarker_size was 0)

So why is this part unnecessary now?  Or should it just be replaced by
something else?

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]erase block header(revision 4)

[Artem B. Bityutskiy]

Jörn Engel wrote:
> Bad suggestion then.  Try to draw a complete graph of all possible
> users to this static structure, the locking required to make it
> correct, then prove its correctness.
Oh, It is not constant anymore (we have the "erasecount" filed there). 
Right. It must not be static. Zhao, I apologize for incorrect suggestion.

> 
> If that didn't already scare you to death, try to anticipate future
> code changes to such brittle code.
Yes, yuo could have just proved its incorrectnes in 2-3 words instead.

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

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

[Jörn Engel]

On Mon, 3 October 2005 18:43:04 +0400, Artem B. Bityutskiy wrote:
> Jörn Engel wrote:
> >Bad suggestion then.  Try to draw a complete graph of all possible
> >users to this static structure, the locking required to make it
> >correct, then prove its correctness.
> Oh, It is not constant anymore (we have the "erasecount" filed there). 
> Right. It must not be static. Zhao, I apologize for incorrect suggestion.
> 
> >
> >If that didn't already scare you to death, try to anticipate future
> >code changes to such brittle code.
> Yes, yuo could have just proved its incorrectnes in 2-3 words instead.

It's always hard to anticipate why people are barking up the wrong
tree.  Instead of figuring it out, the quick solution is to take a
chainsaw and cut the tree down.  Not pretty, but it works.

Jörn

-- 
Prosperity makes friends, adversity tries them.
-- Publilius Syrus 

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

[zhao forrest]

>
> > >> What was (c->cleanmarker_size == 0) used for?
> > >
> > > For the flash that don't need a clean marker.
> > No, that worked for NAND this way (c->cleanmarker_size was 0)
>
>So why is this part unnecessary now?  Or should it just be replaced by
>something else?

It's true that "c->cleanmarker_size was 0" for NAND flash.
But besides  NAND flash, "c->cleanmarker_size was 0" is also true
for dataflash. That means that cleanmarker was not needed for
dataflash for some reason that I don't know :)
Now eraseblock header is needed for any kind of flash. So I removed
the original (c->cleanmarker_size == 0).

Thanks,
Forrest

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

[Artem B. Bityutskiy]

zhao forrest wrote:
> It's true that "c->cleanmarker_size was 0" for NAND flash.
> But besides  NAND flash, "c->cleanmarker_size was 0" is also true
> for dataflash. That means that cleanmarker was not needed for
> dataflash for some reason that I don't know :)

DataFlash is has a minimum IO unit (DataFlash block, 528/1056 AFAIR),
and has no OOB. So the cleanmarker would consume the whole DataFlash
block which considered wasteful I suppose.

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

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

[Jörn Engel]

On Sun, 9 October 2005 11:06:17 +0400, Artem B. Bityutskiy wrote:
> zhao forrest wrote:
> > It's true that "c->cleanmarker_size was 0" for NAND flash.
> > But besides  NAND flash, "c->cleanmarker_size was 0" is also true
> > for dataflash. That means that cleanmarker was not needed for
> > dataflash for some reason that I don't know :)
> 
> DataFlash is has a minimum IO unit (DataFlash block, 528/1056 AFAIR),
> and has no OOB. So the cleanmarker would consume the whole DataFlash
> block which considered wasteful I suppose.

Is it possible for dataflash to erase a block, power cycle before
erase has completed, and later write to this block before erasing it
again?

Cleanmarkers are the means to prevent above events from happening.  If
dataflash has some other means, we can dump cleanmarkers.  If not,
dumping them would be a reliability problem.

Jörn

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

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

[Jörn Engel]

On Mon, 3 October 2005 21:40:07 +0800, zhao forrest wrote:
>
> >> diff -auNrp mtd_9_28/fs/jffs2/fs.c mtd_9_28_EBH/fs/jffs2/fs.c
> >> --- mtd_9_28/fs/jffs2/fs.c	2005-09-28 10:51:57.000000000 +0800
> >> +++ mtd_9_28_EBH/fs/jffs2/fs.c	2005-09-28 11:51:53.000000000 +0800
> >> @@ -476,6 +476,7 @@ int jffs2_do_fill_super(struct super_blo
> >>  	}
> >>
> >>  	c->cleanmarker_size = sizeof(struct jffs2_unknown_node);
> >> +	c->ebh_size = PAD(sizeof(struct jffs2_raw_ebh));
> >
> >Remove the PAD.  Instead you should make sure that PAD(sizeof(struct
> >jffs2_raw_ebh)) and sizeof(struct jffs2_raw_ebh) are equal.
> >
> >If they aren't, you have some other problem anyway.
> 
> I don't see a problem here, could you explain what kind of
> problem I will have??

You have a structure without proper alignment.  That would be ok for a
single field in the struct - the last - but it is much easier to make
the rule a bit stricter and require that the struct must be a multiple
of 4 or 8 bytes.

And once you've done that, there simply is no need for the PAD
anymore.  Less complicated.

> >> @@ -196,8 +197,14 @@ 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 erase_count;
> >>  };
> >>
> >> +#define SET_EBFLAGS_HAS_EBH(jeb) (jeb->flags |= 1)
> >> +#define CLR_EBFLAGS_HAS_EBH(jeb) (jeb->flags &= ~1)
> >> +#define EBFLAGS_HAS_EBH(jeb) ((jeb->flags & 1) == 1)
> >
> >SET_EBFLAGS_HAS_EBH and CLR_EBFLAGS_HAS_EBH don't make sense.
> >EBFLAGS_SET_EBH does.
> Sorry. I can't understand why EBFLAGS_SET_EBH make sense.
> This flag is used to indicate whether an erase block has the
> eraseblock header. From EBFLAGS_SET_EBH code reader can't
> associate it with "_has_ eraseblock header".
> I think the flag's name is has_ebh instead of ebh.

Ok, I see.  Terms like "HAS" or "IS" are commonly used for
truth-returning functions/macros.  Hence, they should be avoided
elsewhere.  For flags, most people seem to use

FOO_HAS_BAR
FOO_SET_BAR
FOO_CLEAR_BAR

or some variants thereof.

> >> diff -auNrp mtd_9_28/fs/jffs2/nodemgmt.c 
> mtd_9_28_EBH/fs/jffs2/nodemgmt.c
> >> --- mtd_9_28/fs/jffs2/nodemgmt.c	2005-09-28 10:51:57.000000000 +0800
> >> +++ mtd_9_28_EBH/fs/jffs2/nodemgmt.c	2005-09-28 
> >13:42:53.000000000 
> +0800
> >> @@ -342,7 +342,10 @@ static int jffs2_do_reserve_space(struct
> >>
> >>  		jeb = c->nextblock;
> >>
> >> -		if (jeb->free_size != c->sector_size - c->cleanmarker_size) {
> >> +		if ((!EBFLAGS_HAS_EBH(jeb) && jeb->free_size != 
> >c->sector_size - 
> c->cleanmarker_size) ||
> >> +		    (EBFLAGS_HAS_EBH(jeb) && c->ebh_size && jeb->free_size 
> >!= 
> c->sector_size - ref_totlen(c, jeb, jeb->first_node)) ||
> >> +		    (EBFLAGS_HAS_EBH(jeb) && !c->ebh_size && jeb->free_size 
> >!= 
> c->sector_size))
> >> +		{
> >
> >This is too complicated.  What are you trying to do?
> I don't think it too complicated. It's only the extension of original
> condition judgement. The aim is that if free_size is not the expected
> size, then goto restart......

It was too complicated for me to understand quickly.  When something
like this happens, there are usually two options:
1. You fucked up and this thing is actually wrong.  Go and fix it.
2. Condition is essentially correct.  Create small function with a
   name like "action_foo_required", which returns 1 if you need to do
   foo, else 0.  The condition is a simple function call then, which
   should be obviously correct.
   The function itself can be easily expanded into many smaller
   conditions:
	if (this)
		return 1;
	if (that)
		return 0;
	if (other)
		return 1;
	return 0;
   Now each of the smaller conditions is simple to verify and can be
   commented if it isn't simple to verify.

Jörn

-- 
And spam is a useful source of entropy for /dev/random too!
-- Jasmine Strong