1:1 mapping

1:1 mapping

[Jörn Engel]

New subject, removed reference to old mail.  I guess we agree that
this is completely independent of EBH and the first person who tries
to bring EBH back into this thread will have to do 20 pushups.  Right?

On Thu, 22 September 2005 15:47:06 +0400, Artem B. Bityutskiy wrote:
>
> Well, this was discussed... But I still don't see any conclusion. My
> opinion is below.
> 
> Terms:
> 1. New JFFS2 - recent JFFS2 where 1:1 was added.
> 2. Old JFFS2 - older JFFS2, before 1:1 was added.
> 3. New JFFS2 image - an image made for new JFFS2.
> 4. Old JFFS2 image - an image made for old JFFS2.
> 
> Q: are there any issues when an old JFFS2 image is mounted by new JFFS2
> code?
> A: yes, there are. Old JFFS2 did eraseblock concatenations and worked
> with virtual eraseblocks. So, there are nodes which cross the eraseblock
> boundary.

You're giving it an interesting spin.  The above was correct for very
few people only.  Everyone I know about didn't have virtual blocks.  A
few people would have had virtual blocks and protected themselves
against it by setting the MTD_NO_VIRTBLOCKS flag.

Your concern to the remaining people - if there are any - is
honorable.  But I am concerned about the vast majority as well.
*Everyone* would suffer an incompatible update.  *Everyone* would
suffer complicated code to detect old 1:m mapping in terms of code
size, bugs, etc.

Last, those few - if any - who would be in danger from an update are
in danger already.  The need for virtual blocks depends on subtle
things like processor type, code used, resizing of JFFS2 partitions,
etc.  People could have lost data already.

So this is effectively a trade-off.  How much do we inconvenience the
minory?  How much do we inconvenience everyone?  And how do we warn
the minority about the danger they are in?

> Q: why is this a problem?
> A: because with 1:1 mapping we will have nodes which cross the
> eraseblock boundary. JFFS2 will suffer hard.
> 
> Q: what to do?
> A: either handle this or reject mounting old images in New JFFS2.

Or we tell users very clearly about the problem.  In a way they can't
ignore.  Proposed patch below.

Jörn

-- 
The competent programmer is fully aware of the strictly limited size of
his own skull; therefore he approaches the programming task in full
humility, and among other things he avoids clever tricks like the plague. 
-- Edsger W. Dijkstra

--- linux-2.6.12-rc4logfs/fs/Kconfig~warn_users	2005-05-12 06:38:11.000000000 +0200
+++ linux-2.6.12-rc4logfs/fs/Kconfig	2005-09-22 14:10:54.000000000 +0200
@@ -1033,10 +1033,28 @@ config JFFS_PROC_FS
 	  Enabling this option will cause statistics from mounted JFFS file systems
 	  to be made available to the user in the /proc/fs/jffs/ directory.
 
+config JFFS2_FS_CORRUPTION_WARNING
+	bool "Yes, I have read the warning about a possible JFFS2 corruption"
+	default n
+	help
+	  An old version of JFFS2 introduced the concept of virtual blocks.
+	  Close analysis of that feature showed that virtual block change
+	  the on-flash structure of JFFS2 in a way that can cause data
+	  corruption and data loss if mounted with different virtual block
+	  sizes (or no virtual blocks) later.
+	  If you want to mount old JFFS2 images, you should read the complete
+	  mail threads below before answering yes - better be safe than sorry.
+
+	  http://lists.infradead.org/pipermail/linux-mtd/2005-August/013498.html
+	  http://lists.infradead.org/pipermail/linux-mtd/2005-August/013465.html
+
+	  If in doubt, say n.
+
 config JFFS2_FS
 	tristate "Journalling Flash File System v2 (JFFS2) support"
 	select CRC32
 	depends on MTD
+	depends on JFFS2_FS_CORRUPTION_WARNING
 	help
 	  JFFS2 is the second generation of the Journalling Flash File System
 	  for use on diskless embedded devices. It provides improved wear

Re: 1:1 mapping

[Josh Boyer]

On Thu, 2005-09-22 at 14:20 +0200, Jörn Engel wrote:
> 
> Or we tell users very clearly about the problem.  In a way they can't
> ignore.  Proposed patch below.

Erm... I hope that was a bit sarcastic.  Actually putting something like
that into the kernel would look a bit ridiculous.

josh

Re: 1:1 mapping

[Jörn Engel]

On Thu, 22 September 2005 08:05:10 -0500, Josh Boyer wrote:
> On Thu, 2005-09-22 at 14:20 +0200, Jörn Engel wrote:
> > 
> > Or we tell users very clearly about the problem.  In a way they can't
> > ignore.  Proposed patch below.
> 
> Erm... I hope that was a bit sarcastic.  Actually putting something like
> that into the kernel would look a bit ridiculous.

Apart from a missing dependency on CONFIG_MTD, no.  With the BIG FAT
WARNING mail sent to the list, everyone tracking CVS should know about
the problem.  But the remaining people who upgrade via -linus might
still have a problem.  This way, they also get forced to notice.

If you have a better idea, please speak up.  This beats an
incompatible option by far, imo.  We could also do something like
this:

int jffs2_blocks_use_vmalloc(struct jffs2_sb_info *c)
{
	int ret = ((c->flash_size / c->sector_size) * sizeof (struct jffs2_eraseblock)) > (128 * 1024);
	if (unlikely(ret)) {
		printk(KERN_ERR "Please read up on the virtual mapping stuff");
		BUG();
	}
	return ret;
}

Then all people who depend on Ferenc's virtual mapping patch to work
will see a warning message.  Others don't notice.  People may remove
the BUG and printk if they know what they're doing.

Then after a couple of kernel versions, we'll turn it into a one-time
warning and a bit later completely remove it.

Jörn

PS: I really don't care about looking ridiculous here.  As long as it
solved people's problems, that's just fine.

-- 
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: 1:1 mapping

[Artem B. Bityutskiy]

On Thu, 2005-09-22 at 15:21 +0200, Jörn Engel wrote:
> If you have a better idea, please speak up.  This beats an
> incompatible option by far, imo.  We could also do something like
> this:
> 
> int jffs2_blocks_use_vmalloc(struct jffs2_sb_info *c)
> {
> 	int ret = ((c->flash_size / c->sector_size) * sizeof (struct jffs2_eraseblock)) > (128 * 1024);
> 	if (unlikely(ret)) {
> 		printk(KERN_ERR "Please read up on the virtual mapping stuff");
> 		BUG();
> 	}
> 	return ret;
> }

Agreed, but with 2 notes:

1. a distinct function should be created which is invoked when JFFS2 has
detected that the image is an old image:

int safe_to_mount(struct jffs2_sb_info *c)
{
	int virtual;
	virtual = ((c->flash_size / c->sector_size) * JFFS2_LEGACY_EB_SIZE > (128 * 1024);
	if (virtual)
		return 0;
	return 1;
}

Where JFFS2_LEGACY_EB_SIZE = sizeof(struct jffs2_eraseblock) of old
JFFS2.

2. No need to invoke BUG(), it is enough just to return an error and let
JFFS2 cleanly reject mounting the image.

3. No need to add any additional config option, it is enough to describe
this on the MTD site and in a readme.

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

Re: 1:1 mapping

[Jörn Engel]

On Thu, 22 September 2005 17:41:44 +0400, Artem B. Bityutskiy wrote:
> On Thu, 2005-09-22 at 15:21 +0200, Jörn Engel wrote:
> > If you have a better idea, please speak up.  This beats an
> > incompatible option by far, imo.  We could also do something like
> > this:
> > 
> > int jffs2_blocks_use_vmalloc(struct jffs2_sb_info *c)
> > {
> > 	int ret = ((c->flash_size / c->sector_size) * sizeof (struct jffs2_eraseblock)) > (128 * 1024);
> > 	if (unlikely(ret)) {
> > 		printk(KERN_ERR "Please read up on the virtual mapping stuff");
> > 		BUG();
> > 	}
> > 	return ret;
> > }
> 
> Agreed, but with 2 notes:
> 
> 1. a distinct function should be created which is invoked when JFFS2 has
> detected that the image is an old image:

Can you explain "when JFFS2 has detected that the image is an old
image" in C?

> int safe_to_mount(struct jffs2_sb_info *c)
> {
> 	int virtual;
> 	virtual = ((c->flash_size / c->sector_size) * JFFS2_LEGACY_EB_SIZE > (128 * 1024);
> 	if (virtual)
> 		return 0;
> 	return 1;
> }
> 
> Where JFFS2_LEGACY_EB_SIZE = sizeof(struct jffs2_eraseblock) of old
> JFFS2.

32bit or 64bit?  I guess using the size of 64bit, possibly adding a
little for safety, should be fine.

> 2. No need to invoke BUG(), it is enough just to return an error and let
> JFFS2 cleanly reject mounting the image.

Fine with me.

> 3. No need to add any additional config option, it is enough to describe
> this on the MTD site and in a readme.

Fine with me.

Jörn

-- 
Homo Sapiens is a goal, not a description.
-- unknown

Re: 1:1 mapping

[Artem B. Bityutskiy]

On Thu, 2005-09-22 at 15:48 +0200, Jörn Engel wrote:
> Can you explain "when JFFS2 has detected that the image is an old
> image" in C?
Well, we may even do better. Scan FS. If we meet any node which crosses
the eraseblock boundary - bye bye. That must be easy to implement as
node headers contain the total node length.

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

Re: 1:1 mapping

[Jörn Engel]

On Thu, 22 September 2005 17:54:58 +0400, Artem B. Bityutskiy wrote:
> On Thu, 2005-09-22 at 15:48 +0200, Jörn Engel wrote:
> > Can you explain "when JFFS2 has detected that the image is an old
> > image" in C?
> Well, we may even do better. Scan FS. If we meet any node which crosses
> the eraseblock boundary - bye bye. That must be easy to implement as
> node headers contain the total node length.

Sounds fine.  Yes, that appears to be the best solution.

Jörn

-- 
Optimizations always bust things, because all optimizations are, in
the long haul, a form of cheating, and cheaters eventually get caught.
-- Larry Wall 

Re: 1:1 mapping

[Ferenc Havasi]

Jörn Engel wrote:

>On Thu, 22 September 2005 17:54:58 +0400, Artem B. Bityutskiy wrote:
>  
>
>>On Thu, 2005-09-22 at 15:48 +0200, Jörn Engel wrote:
>>    
>>
>>>Can you explain "when JFFS2 has detected that the image is an old
>>>image" in C?
>>>      
>>>
>>Well, we may even do better. Scan FS. If we meet any node which crosses
>>the eraseblock boundary - bye bye. That must be easy to implement as
>>node headers contain the total node length.
>>    
>>
>
>Sounds fine.  Yes, that appears to be the best solution.
>  
>

Yes, that is the same I wrote in my letter. As Artem asked me to
implement it I wrote a patch, you can find it at
http://www.inf.u-szeged.hu/jffs2/cross-boundary.patch

I have not tested it yet (it will be task of tomorrow) I would just to
show what is in my mind.

Ferenc

Re: 1:1 mapping

[zhao forrest]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=gb2312; format=flowed, Size: 1994 bytes --]




>From: Ferenc Havasi <havasi@inf.u-szeged.hu>
>To: Jörn Engel <joern@wohnheim.fh-wedel.de>
>CC: "Artem B. Bityutskiy" <dedekind@infradead.org>,  zhao forrest 
<zhao_fusheng@hotmail.com>,  linux-mtd@lists.infradead.org
>Subject: Re: 1:1 mapping
>Date: Fri, 23 Sep 2005 01:48:31 +0200
>
>Jörn Engel wrote:
>
> >On Thu, 22 September 2005 17:54:58 +0400, Artem B. Bityutskiy wrote:
> >
> >
> >>On Thu, 2005-09-22 at 15:48 +0200, Jörn Engel wrote:
> >>
> >>
> >>>Can you explain "when JFFS2 has detected that the image is an old
> >>>image" in C?
> >>>
> >>>
> >>Well, we may even do better. Scan FS. If we meet any node which crosses
> >>the eraseblock boundary - bye bye. That must be easy to implement as
> >>node headers contain the total node length.
> >>
> >>
> >
> >Sounds fine.  Yes, that appears to be the best solution.
> >
> >
>
>Yes, that is the same I wrote in my letter. As Artem asked me to
>implement it I wrote a patch, you can find it at
>http://www.inf.u-szeged.hu/jffs2/cross-boundary.patch
>
>I have not tested it yet (it will be task of tomorrow) I would just to
>show what is in my mind.
>
>Ferenc
>

Your patch may have problem.
In fact this "reject to mount when cross-boundary" has been in my
patch, it's quite simple. This code segment is extracted from my patch:

@@ -570,10 +584,8 @@ scan_more:
              /* Eep. Node goes over the end of the erase block. */
         printk(KERN_WARNING "Node at 0x%08x with length 0x%08x would
 run over the end of the erase block\n",
                               ofs, je32_to_cpu(node->totlen));
-        printk(KERN_WARNING "Perhaps the file system was created 
           with the wrong erase size?\n");
-                       DIRTY_SPACE(4);
-                       ofs += 4;
-                       continue;
+                       printk(KERN_NOTICE "Perhaps the file system
       was created with the wrong erase size? Reject to mount.\n");
+                       return -EINVAL;
                }

Thanks,
Forrest

Re: 1:1 mapping

[Ferenc Havasi]

zhao forrest wrote:

> Your patch may have problem.
> In fact this "reject to mount when cross-boundary" has been in my
> patch, it's quite simple. This code segment is extracted from my patch:
>
> @@ -570,10 +584,8 @@ scan_more:
>              /* Eep. Node goes over the end of the erase block. */
>         printk(KERN_WARNING "Node at 0x%08x with length 0x%08x would
> run over the end of the erase block\n",
>                               ofs, je32_to_cpu(node->totlen));
> -        printk(KERN_WARNING "Perhaps the file system was created
>           with the wrong erase size?\n");
> -                       DIRTY_SPACE(4);
> -                       ofs += 4;
> -                       continue;
> +                       printk(KERN_NOTICE "Perhaps the file system
>       was created with the wrong erase size? Reject to mount.\n");
> +                       return -EINVAL;
>                }

My patch has one more part: detecting that case, if the node header is
also cross-boundary. But that case we cannot say: it is absolutely sure
that it is a cross-boundary node, just can say: it is likely.

Forutnatelly the node header is small enough to say that if someone uses
bad (larger) erase block size, there will be a cross-boundary node which
have a correct node header at the end of the erase block, and only the
body is cross-boundary.

So If everyone agrees we can commit this small part of Zhao's patch.

Ferenc

Re: 1:1 mapping

[Jörn Engel]

On Fri, 23 September 2005 10:10:15 +0200, Ferenc Havasi wrote:
> zhao forrest wrote:
> 
> > Your patch may have problem.
> > In fact this "reject to mount when cross-boundary" has been in my
> > patch, it's quite simple. This code segment is extracted from my patch:
> >
> > @@ -570,10 +584,8 @@ scan_more:
> >              /* Eep. Node goes over the end of the erase block. */
> >         printk(KERN_WARNING "Node at 0x%08x with length 0x%08x would
> > run over the end of the erase block\n",
> >                               ofs, je32_to_cpu(node->totlen));
> > -        printk(KERN_WARNING "Perhaps the file system was created
> >           with the wrong erase size?\n");
> > -                       DIRTY_SPACE(4);
> > -                       ofs += 4;
> > -                       continue;
> > +                       printk(KERN_NOTICE "Perhaps the file system
> >       was created with the wrong erase size? Reject to mount.\n");
> > +                       return -EINVAL;
> >                }
> 
> My patch has one more part: detecting that case, if the node header is
> also cross-boundary. But that case we cannot say: it is absolutely sure
> that it is a cross-boundary node, just can say: it is likely.
> 
> Forutnatelly the node header is small enough to say that if someone uses
> bad (larger) erase block size, there will be a cross-boundary node which
> have a correct node header at the end of the erase block, and only the
> body is cross-boundary.
> 
> So If everyone agrees we can commit this small part of Zhao's patch.

Actually, I preferred your patch:
o -EIO makes slightly more sense than -EINVAL.
o Detecting nodes with headers crossing the boundary also makes sense.

The shorter string constant of Zhao's patch, on the other hand,
reduces binary size and should be enough.  People will report problem
to the mailing list and get pointed in the right direction.

I'll try to have a better look over the weekend.

Jörn

-- 
It's just what we asked for, but not what we want!
-- anonymous

Re: 1:1 mapping

[zhao forrest]

> > > Can you explain "when JFFS2 has detected that the image is an old
> > > image" in C?
> > Well, we may even do better. Scan FS. If we meet any node which crosses
> > the eraseblock boundary - bye bye. That must be easy to implement as
> > node headers contain the total node length.
>
>Sounds fine.  Yes, that appears to be the best solution.
>
This solution has been in my patch.

Until now we only disscussed one aspect of the problem.
Another aspect is that what should we do when users mount new JFFS2 image
with old JFFS2 code? Is the "big warning" in readme enough? Or we should
let old JFFS2 code reject mounting new JFFS2 image?

Thanks,
Forrest

Re: 1:1 mapping

[Jörn Engel]

On Fri, 23 September 2005 10:56:56 +0800, zhao forrest wrote:
> >> > Can you explain "when JFFS2 has detected that the image is an old
> >> > image" in C?
> >> Well, we may even do better. Scan FS. If we meet any node which crosses
> >> the eraseblock boundary - bye bye. That must be easy to implement as
> >> node headers contain the total node length.
> >
> >Sounds fine.  Yes, that appears to be the best solution.
> >
> This solution has been in my patch.
> 
> Until now we only disscussed one aspect of the problem.
> Another aspect is that what should we do when users mount new JFFS2 image
> with old JFFS2 code? Is the "big warning" in readme enough? Or we should
> let old JFFS2 code reject mounting new JFFS2 image?

Just let them mount it.  If people actually convert to virtual block
in this way, it should be relatively safe.  On their way back they
will notice.

Really, this is an unlikely case of an unlikely case.  People would
have to
1. have enough erase blocks to need virtual blocks,
2. not have MTD_NO_VIRTBLOCKS flag set,
3. decide to downgrade from newer code,
4. by all chances, previously have upgraded to the newer code and
   either not see a problem or ignore it.

Just ignore that case.

Jörn

-- 
I don't understand it. Nobody does.
-- Richard P. Feynman

Re: 1:1 mapping

[zhao forrest]

> > > Can you explain "when JFFS2 has detected that the image is an old
> > > image" in C?
> > Well, we may even do better. Scan FS. If we meet any node which crosses
> > the eraseblock boundary - bye bye. That must be easy to implement as
> > node headers contain the total node length.
>
>Sounds fine.  Yes, that appears to be the best solution.
>
This solution has been in my patch.

Until now we only disscussed one aspect of the problem.
Another aspect is that what should we do when users mount new JFFS2 image
with old JFFS2 code? Is the "big warning" in readme enough? Or we should
let old JFFS2 code reject mounting new JFFS2 image?

Thanks,
Forrest