Re: [PATCH]Erase block header(revision 1)

Re: [PATCH]Erase block header(revision 1)

[Ferenc Havasi]

Hi Zhao,

First of all: your implementation of EBH summary support a good job,
congratulation!

I noticed only two things:

+        case JFFS2_NODETYPE_ERASEBLOCK_HEADER:
+            s->sum_size +=
JFFS2_SUMMARY_EBH_SIZE(je16_to_cpu(item->eh.dsize)*4);

Here I think you should use sizeof(uint32_t) instead of simple 4, as you
used it otherwhere. I's a very small thing, I know.

The other is related to it: if I were you I would store eh.dsize in
bytes, not in uint32. In every other places of JFFS2 the size is stored
byte based (if I am right), and there is no need to always multible by
sizeof(uint32_t) if the real size is needed.

I have only one big problem with this patch. I'm strongly against to
introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and
JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old
JFFS2 code on NAND devices, too. Do I understand well: is this the only
reason?

Ferenc

Re: [PATCH]Erase block header(revision 1)

[Artem B. Bityutskiy]

Ferenc Havasi wrote:
> I have only one big problem with this patch. I'm strongly against to
> introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and
> JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old
> JFFS2 code on NAND devices, too. Do I understand well: is this the only
> reason?
Haven't we already noted/agreed that JFFS2_NODETYPE_DIRENT_EBH is ugly 
and should go away???


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

Re: [PATCH]Erase block header(revision 1)

[zhao forrest]

>
>I have only one big problem with this patch. I'm strongly against to
>introduce INCOMPAT nodes JFFS2_NODETYPE_DIRENT_EBH and
>JFFS2_NODETYPE_INODE_EBH just to make unusable new JFFS2 images for old
>JFFS2 code on NAND devices, too. Do I understand well: is this the only
>reason?

Yes. We have decided to remove JFFS2_NODETYPE_DIRENT_EBH and
JFFS2_NODETYPE_INODE_EBH.

I'll send out a patch to fix all these problems next week.

Have a happy weekend!

Forrest

Re: [PATCH]Erase block header(revision 1)

[Jörn Engel]

On Fri, 23 September 2005 14:53:09 +0800, zhao forrest wrote:
> 
> This is the revised version of eraseblock header patch.
> Compared with previous version, the following changes are
> made:
> 1 eliminate the too long lines, and make them shorter
> 2 change eraseblock_heaer_size to ebh_size; has_eraseblock
> _header to has_ebh to avoid long variable name
> 3 change jffs2_write_nand_cleanmarker() to jffs2_write_nand_ebh()
> to avoid confusion
> 4 change jffs2_check_nand_cleanmarker() to
> jffs2_check_nand_cleanmarker_ebh() to avoid confusion

I ignored all the code and took a look at the new node type only.

> +struct jffs2_eraseblock_header
> +{
> +	jint16_t magic;
> +	jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */
> +	jint32_t totlen;
> +	jint32_t hdr_crc;
> +	uint8_t  fs_version; /* the version of this JFFS2 fs image */

Version is completely useless.

> +	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 */

Breaks alignment and pretty useless.  Information can be deduced from
totlen.  Remove.

> +	jint32_t node_crc;
> +	jint32_t data_crc;

Combine and move up, just below hdr_crc.

> +	jint32_t data[0];
> +} __attribute__((packed));
> +
>  struct jffs2_raw_dirent
>  {
>  	jint16_t magic;
 


Jörn

-- 
Good warriors cause others to come to them and do not go to others.
-- Sun Tzu

Re: [PATCH]Erase block header(revision 1)

[zhao forrest]

>
> > +struct jffs2_eraseblock_header
> > +{
> > +	jint16_t magic;
> > +	jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */
> > +	jint32_t totlen;
> > +	jint32_t hdr_crc;
> > +	uint8_t  fs_version; /* the version of this JFFS2 fs image */
>
>Version is completely useless.

I agree that version field is not used in my patch after compat_fset,
incompat_fset and rocompat_fset are introduced in my patch.
But I'm not sure if we should keep this field. 
Artem,
What's your opinion about this?

> > +	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 */
>
>Breaks alignment and pretty useless.  Information can be deduced from
>totlen.  Remove.
>
Sometimes we can't deduce the exact additional data size from totlen.
In particular this is true for NOR ECC. For example, sizeof(struct
 jffs2_eraseblock_header) is 34 bytes; totlen is 40 bytes, dsize is
0 byte.

Thanks,
Forrest

Re: [PATCH]Erase block header(revision 1)

[Jörn Engel]

On Mon, 26 September 2005 16:02:50 +0800, zhao forrest wrote:
> 
> >> +struct jffs2_eraseblock_header
> >> +{
> >> +	jint16_t magic;
> >> +	jint16_t nodetype; /* == JFFS2_NODETYPE_ERASEBLOCK_HEADER */
> >> +	jint32_t totlen;
> >> +	jint32_t hdr_crc;
> >> +	uint8_t  fs_version; /* the version of this JFFS2 fs image */
> >
> >Version is completely useless.
> 
> I agree that version field is not used in my patch after compat_fset,
> incompat_fset and rocompat_fset are introduced in my patch.
> But I'm not sure if we should keep this field. 
> Artem,
> What's your opinion about this?

Just give it a useful name like "reserved" or "unused".  We still need
the alignment.

> >> +	uint8_t  compat_fset;
> >> +	uint8_t  incompat_fset;
> >> +	uint8_t  rocompat_fset;

What does "fset" mean?  Why not just "flags_compat" etc.?

> >> +	jint32_t erase_count; /* the erase count of this erase block */
> >> +	jint16_t dsize; /* the size of additional data behind node_crc */
> >
> >Breaks alignment and pretty useless.  Information can be deduced from
> >totlen.  Remove.
> >
> Sometimes we can't deduce the exact additional data size from totlen.
> In particular this is true for NOR ECC. For example, sizeof(struct
> jffs2_eraseblock_header) is 34 bytes; totlen is 40 bytes, dsize is
> 0 byte.

Even then it doesn't matter.  We just copy all fields and ignore the
ones we don't know about.  What we know about is defined in the three
flags fields.

Jörn

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

Re: [PATCH]Erase block header(revision 1)

[Artem B. Bityutskiy]

zhao forrest wrote:
> I agree that version field is not used in my patch after compat_fset,
> incompat_fset and rocompat_fset are introduced in my patch.
> But I'm not sure if we should keep this field. Artem,
> What's your opinion about this?

Well, expect that EBH format may be changed in future, for example if
some Mr.Smith will and one more per-eraseblock field. The same may in
principle happen with any other node type. In this case, we may use the
'version' filed as the FS format identifier. So, IMO, the field is not
useless.

>> > +    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 */

I still not really like the data[] field. At least the name... Is it
just for future EBH extentions?

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